Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Recipe to Migrate Acegi Security to Spring Security #808

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

CodexRaunak
Copy link
Contributor

Issue #792
Added a scanning recipe to Migrate Acegi Security to Spring Security. It checks if acegi security is used as a dependency, if yes then it migrates it to Spring Security by using 2 visitors.

  1. Java visitor - Which changes the type and packages.
  2. Maven visitor - Which removes the acegi sercurity and adds spring security core and spring security config dependencies.

We need to make it a scanning recipe as the maven visitor was adding the spring security even when no acegi security is used (i.e no migration required).
Therefore we check first if acegi security is used then perform the migration or it will unnecessary add certain dependencies.

Testing done

Added test cases for the recipe, visitor and UpgradeNextMajorParentVersion.

Submitter checklist

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

@CodexRaunak
Copy link
Contributor Author

Would like a review on this.
I don't think it should be a top level recipe, it just performs migration.
Should I add it in UpgradeToRecommendCoreVersion, UpgradeToLatestJava11CoreVersion and UpgradeToLatestJava8CoreVersion?

@CodexRaunak
Copy link
Contributor Author

UpdateBomTest.shouldUpdateToLatestIncrementalsWithoutMavenConfig is failing :(

@jonesbusy
Copy link
Collaborator

This is due to incremental cleanup project: https://repo.jenkins-ci.org/artifactory/incrementals/io/jenkins/tools/bom/

I will see how to fix the test (or disable it)

@jonesbusy
Copy link
Collaborator

This looks ok for me. But my concern is when I see

<dependency>
    <groupId>org.acegisecurity</groupId>
    <artifactId>acegi-security</artifactId>
    <version>1.0.7</version>
</dependency>

No plugin should declarare such dependency (like spring security) since it's provided by core.

So normally we just need to change package name and not touch dependency

@gounthar By any chance did you follow this rather old migration?

@CodexRaunak CodexRaunak force-pushed the recipe/Migrate-Acegi-Security-to-Spring-Security branch from 7dfe347 to 89f102b Compare February 21, 2025 00:26
@CodexRaunak
Copy link
Contributor Author

So normally we just need to change package name and not touch dependency

Then it doesn't need to be a scanning recipe, I made the required changes.
Also when applying it to bitbucket-oauth.
java -jar plugin-modernizer-cli/target/jenkins-plugin-modernizer-999999-SNAPSHOT.jar dry-run --plugins bitbucket-oauth --recipe UpgradeNextMajorParentVersion

I think it needs some more migration, like loadUserByUsername should be migrated to loadUserByUsername2.
And this getAuthorities() is using the array as a return type which was used in acegi security, the newer return type of getAuthorities() in org.springframework.security.authentication.AbstractAuthenticationToken is Collection<GrantedAuthority>.
image
image

@gounthar
Copy link
Collaborator

Basil made a few of them recently if that can help.
jenkinsci/gitlab-oauth-plugin#169
jenkinsci/github-oauth-plugin#285

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants