[JEP-227][JENKINS-39324] Replace Acegi Security with Spring Security APIs #490

Vlatombe · 2023-10-23T08:24:45Z

This is the implementation of
jenkinsci/jenkins#4848 for Credentials API.

cf. https://github.com/jenkinsci/jep/blob/31859bedd4594df528428ccc737117b44b786fce/jep/227/README.adoc

This will allow consumers of the credentials API to remove references to deprecated acegi APIs.

Some overloads methods were renamed in order to avoid the ambiguous signatures involving Item and ItemGroup.

For example, lookupCredentials become lookupCredentialsInItem and lookupCredentialsInItemGroup.

cc @jglick

Testing done

Submitter checklist

Give feedback

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
Options

This is the implementation of jenkinsci/jenkins#4848 for Credentials API. This will allow consumers of the credentials API to remove references to deprecated acegi APIs.

jglick

Looks good overall, and long overdue.

Is there a reason this is still in draft—are you planning to create some sample downstream PRs?

Hard to verify from review that signatures remain compatible. siom79/japicmp#266 would be helpful.

src/main/java/com/cloudbees/plugins/credentials/CredentialsProvider.java

jglick · 2023-10-23T17:00:18Z

src/main/java/com/cloudbees/plugins/credentials/CredentialsStore.java

-        if (!hasPermission(a, p)) {
-            throw new AccessDeniedException2(a, p);
+        Authentication a = Jenkins.getAuthentication2();
+        if (!hasPermission2(a, p)) {
+            throw new AccessDeniedException3(a, p);


Note that this will cause a change of behavior even without changing any clients. Not much we can do about that; one of the tricky aspects of JEP-222 was providing compatibility for exception types. I updated at least the standard handlers to accept either the Acegi or Spring Security versions, and automatically proxied from some other methods.

checkPermission2(..)?

…cation) in favor of CredentialsProvider#getCredentials2(Class, ItemGroup, Authentication, List)

rsandell

Perhaps some unit tests could be left as is/duplicated to test that the backwards compatible code works?

jglick

Fine I think. Any downstream PRs showing the new APIs off in context?

Vlatombe · 2023-10-26T07:13:23Z

Any downstream PRs showing the new APIs off in context?

Filed a few including usage and implementation of CredentialsProvider

jtnord · 2023-10-26T13:28:38Z

haven't really checked the code, but all the documentation needs to be updated too.

jglick

Nice, seems clearer and avoids the …2 naming pattern.

(The native English speaker in me wants lookup used as a verb to be replaced by lookUp but there is precedent in ExtensionList etc. Anyway I have to suppress my spelling emotions as a Java developer after Cloneable [sic]!)

jglick · 2023-10-27T12:03:25Z

docs/consumer.adoc

 ====

-There are currently two overloads, one taking `Item` as the context and the other taking `ItemGroup` as the context, the other parameters are otherwise identical.
-
-NOTE: A current RFE https://issues.jenkins-ci.org/browse/JENKINS-39324[JENKINS-39324] is looking to replace overloaded methods with a single method taking the more generic `ModelObject`.


Completely forgot I filed this seven years ago. @AncestorInPath ModelObject context would be convenient sometimes I guess, though having explicit methods for distinct types of context as you have here is probably clearer.

michelzanini · 2023-10-27T14:04:30Z

Just so you know after this PR was merged a new release was created 305.v04f5ec1f3743 my Jenkins updated and immediately no builds are working anymore. This is the error:

java.lang.AbstractMethodError: Implement getCredentialsInItemGroup
	at com.cloudbees.plugins.credentials.CredentialsProvider.getCredentialsInItemGroup(CredentialsProvider.java:1193)
	at com.cloudbees.plugins.credentials.CredentialsProvider.lookupCredentialsInItemGroup(CredentialsProvider.java:389)
	at com.cloudbees.plugins.credentials.CredentialsProvider.lookupCredentialsInItem(CredentialsProvider.java:544)
	at com.cloudbees.plugins.credentials.CredentialsProvider.lookupCredentials(CredentialsProvider.java:517)
	at org.jenkinsci.plugins.github_branch_source.Connector.lookupScanCredentials(Connector.java:300)
	at org.jenkinsci.plugins.github_branch_source.GitHubSCMSource.retrieve(GitHubSCMSource.java:1636)
	at jenkins.scm.api.SCMSource.fetch(SCMSource.java:581)
	at org.jenkinsci.plugins.workflow.multibranch.SCMBinder.create(SCMBinder.java:99)
	at org.jenkinsci.plugins.workflow.job.WorkflowRun.run(WorkflowRun.java:311)
	at hudson.model.ResourceController.execute(ResourceController.java:101)
	at hudson.model.Executor.run(Executor.java:442)
Finished: FAILURE

jglick · 2023-10-27T14:12:46Z

@michelzanini are you using a special credentials provider by any chance? (Vault, K8s Secrets, etc.) At least the error message needs to mention the implementing class. Will probably just revert this as a hotfix while we study the cause.

michelzanini · 2023-10-27T14:13:21Z

Yes @jglick , Its actually the Github app credentials from Github Source Branch plugin.

jglick · 2023-10-27T14:15:21Z

I meant the type of provider, not the type of credentials. I.e., was this App credentials item stored in global credentials, folder credentials, user credentials, Vault, etc.

michelzanini · 2023-10-27T14:16:36Z

Ah ok, its the default "System" global credentials (nothing special about it):

The part where the problem happens is on a multi-branch pipeline with Github Source for Git and then this credentials select for branch discovery.

jglick · 2023-10-27T14:27:05Z

Strange,

credentials-plugin/src/main/java/com/cloudbees/plugins/credentials/CredentialsProvider.java

Lines 1186 to 1193 in 04f5ec1

    
           public <C extends Credentials> List<C> getCredentialsInItemGroup(@NonNull Class<C> type, 
        
                                                                            @Nullable ItemGroup itemGroup, 
        
                                                                            @Nullable Authentication authentication, 
        
                                                                            @NonNull List<DomainRequirement> domainRequirements) { 
        
               if (Util.isOverridden(CredentialsProvider.class, getClass(), "getCredentials", Class.class, ItemGroup.class, org.acegisecurity.Authentication.class, List.class)) { 
        
                   return getCredentials(type, itemGroup, authentication == null ? null : org.acegisecurity.Authentication.fromSpring(authentication), domainRequirements); 
        
               } 
        
               throw new AbstractMethodError("Implement getCredentialsInItemGroup");

is indeed overridden in

credentials-plugin/src/main/java/com/cloudbees/plugins/credentials/SystemCredentialsProvider.java

Lines 425 to 428 in 04f5ec1

    
           @Override 
        
           public <C extends Credentials> List<C> getCredentialsInItemGroup(@NonNull Class<C> type, @Nullable ItemGroup itemGroup, 
        
                                                                            @Nullable Authentication authentication, 
        
                                                                            @NonNull List<DomainRequirement> domainRequirements) {

jglick · 2023-10-27T14:32:10Z

The originally abstract method lacked the List parameter, so I can imagine some other provider throwing an error?

michelzanini · 2023-10-27T14:35:57Z

org.acegisecurity.Authentication.class

Can it be that method "isOverridden" is checking for "org.acegisecurity.Authentication.class" and the impl method is using "org.springframework.security.core.Authentication" for the Authentication argument?

if (Util.isOverridden(CredentialsProvider.class, getClass(), "getCredentials", Class.class, ItemGroup.class, org.acegisecurity.Authentication.class, List.class)) {

jglick · 2023-10-27T14:38:10Z

I do not think so, the point is that the old deprecated method was using org.acegisecurity.Authentication while the new method uses org.springframework.security.core.Authentication.

https://github.com/jenkinsci/credentials-plugin/releases/tag/1305.v04f5ec1f3743 notes that a regression is being investigated as reported in jenkinsci/credentials-plugin#490 (comment) jenkinsci/credentials-plugin#492 submitted to resolve the issue

…nsci#2617)" https://github.com/jenkinsci/credentials-plugin/releases/tag/1305.v04f5ec1f3743 notes that a regression is being investigated as reported in jenkinsci/credentials-plugin#490 (comment) jenkinsci/credentials-plugin#492 submitted to resolve the issue This reverts commit 2a4afc3.

jglick · 2023-10-27T15:28:20Z

@michelzanini can you retest against https://github.com/jenkinsci/credentials-plugin/releases/tag/1307.v3757c78f17c3 please? If it is not on the update center yet (typically takes 20m or so), you can also just install https://repo.jenkins-ci.org/incrementals/org/jenkins-ci/plugins/credentials/1306.vfa_6a_01a_8028d/credentials-1306.vfa_6a_01a_8028d.hpi from the Advanced tab seems to be there.

jglick · 2023-10-27T15:48:12Z

FWIW I ran 2.414.3, installed github-branch-source and Pipeline plugins, created GH App credentials in the system store, created a multibranch project, and ran it, and it worked fine with both 1305 & 1307. So I have no hypothesis as to the trigger condition for that error.

https://github.com/jenkinsci/credentials-plugin/releases/tag/1305.v04f5ec1f3743 notes that a regression is being investigated as reported in jenkinsci/credentials-plugin#490 (comment) jenkinsci/credentials-plugin#492 submitted to resolve the issue

https://github.com/jenkinsci/credentials-plugin/releases/tag/1305.v04f5ec1f3743 notes that a regression is being investigated as reported in jenkinsci/credentials-plugin#490 (comment) jenkinsci/credentials-plugin#492 submitted to resolve the issue This reverts commit 2a4afc3.

michelzanini · 2023-10-27T16:27:11Z

This is working now with 1307.v3757c78f17c3. I didn't get the exception :) Thanks!

[JEP-227] Replace Acegi Security with Spring Security APIs

84fed07

This is the implementation of jenkinsci/jenkins#4848 for Credentials API. This will allow consumers of the credentials API to remove references to deprecated acegi APIs.

Vlatombe requested a review from a team as a code owner October 23, 2023 08:24

Vlatombe marked this pull request as draft October 23, 2023 09:10

Fix spotbugs issues

50c7c4e

jglick added the developer label Oct 23, 2023

jglick reviewed Oct 23, 2023

View reviewed changes

Fix reviews

835db74

Vlatombe marked this pull request as ready for review October 24, 2023 07:48

Vlatombe added 7 commits October 24, 2023 09:52

Rename methods appropriately

7bb95b2

Fixup some javadocs.

2955012

Remove unused method.

198946c

Forgot to remove usages.

c4c9851

Remove CredentialsProvider#getCredentials2(Class, ItemGroup, Authenti…

c2ffe29

…cation) in favor of CredentialsProvider#getCredentials2(Class, ItemGroup, Authentication, List)

Fix a few null checks while we are here.

5092da8

Restore a method I deleted by mistake.

2b986a2

rsandell approved these changes Oct 24, 2023

View reviewed changes

Add tests for new signatures

311c066

jglick approved these changes Oct 24, 2023

View reviewed changes

Rename *2 methods to *InItem/*InItemGroup to avoid ambiguous signatures

5886374

Vlatombe changed the title ~~[JEP-227] Replace Acegi Security with Spring Security APIs~~ [JEP-227][JENKINS-39324] Replace Acegi Security with Spring Security APIs Oct 26, 2023

Update docs

a2336c2

jglick approved these changes Oct 27, 2023

View reviewed changes

jglick requested a review from jtnord October 27, 2023 12:10

jglick requested a review from rsandell October 27, 2023 12:17

jtnord approved these changes Oct 27, 2023

View reviewed changes

jglick merged commit 04f5ec1 into jenkinsci:master Oct 27, 2023
13 checks passed

jglick mentioned this pull request Oct 27, 2023

Compatibility error in CredentialsProvider #492

Merged

jglick added a commit to jglick/credentials-plugin that referenced this pull request Oct 27, 2023

AbstractMethodError reported in jenkinsci#490 (comment)

fa6a01a

This was referenced Oct 27, 2023

Bump org.jenkins-ci.plugins:credentials from 1304.v5ec13eecef46 to 1305.v04f5ec1f3743 jenkinsci/jenkins#8652

Closed

Revert credentials:1305.v04f5ec1f3743 jenkinsci/bom#2618

Merged

Vlatombe mentioned this pull request Nov 8, 2023

Fix a compatibility error in CredentialsProvider #495

Merged

Vlatombe deleted the jep-227 branch January 16, 2024 14:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JEP-227][JENKINS-39324] Replace Acegi Security with Spring Security APIs #490

[JEP-227][JENKINS-39324] Replace Acegi Security with Spring Security APIs #490

Vlatombe commented Oct 23, 2023 •

edited

Loading

Submitter checklist

jglick left a comment

jglick Oct 23, 2023

rsandell Oct 24, 2023

rsandell left a comment

jglick left a comment

Vlatombe commented Oct 26, 2023

jtnord commented Oct 26, 2023

jglick left a comment

jglick Oct 27, 2023

michelzanini commented Oct 27, 2023

jglick commented Oct 27, 2023

michelzanini commented Oct 27, 2023

jglick commented Oct 27, 2023

michelzanini commented Oct 27, 2023 •

edited

Loading

jglick commented Oct 27, 2023

jglick commented Oct 27, 2023

michelzanini commented Oct 27, 2023 •

edited

Loading

jglick commented Oct 27, 2023

jglick commented Oct 27, 2023 •

edited

Loading

jglick commented Oct 27, 2023

michelzanini commented Oct 27, 2023

[JEP-227][JENKINS-39324] Replace Acegi Security with Spring Security APIs #490

[JEP-227][JENKINS-39324] Replace Acegi Security with Spring Security APIs #490

Conversation

Vlatombe commented Oct 23, 2023 • edited Loading

Testing done

Submitter checklist

jglick left a comment

Choose a reason for hiding this comment

jglick Oct 23, 2023

Choose a reason for hiding this comment

rsandell Oct 24, 2023

Choose a reason for hiding this comment

rsandell left a comment

Choose a reason for hiding this comment

jglick left a comment

Choose a reason for hiding this comment

Vlatombe commented Oct 26, 2023

jtnord commented Oct 26, 2023

jglick left a comment

Choose a reason for hiding this comment

jglick Oct 27, 2023

Choose a reason for hiding this comment

michelzanini commented Oct 27, 2023

jglick commented Oct 27, 2023

michelzanini commented Oct 27, 2023

jglick commented Oct 27, 2023

michelzanini commented Oct 27, 2023 • edited Loading

jglick commented Oct 27, 2023

jglick commented Oct 27, 2023

michelzanini commented Oct 27, 2023 • edited Loading

jglick commented Oct 27, 2023

jglick commented Oct 27, 2023 • edited Loading

jglick commented Oct 27, 2023

michelzanini commented Oct 27, 2023

Vlatombe commented Oct 23, 2023 •

edited

Loading

michelzanini commented Oct 27, 2023 •

edited

Loading

michelzanini commented Oct 27, 2023 •

edited

Loading

jglick commented Oct 27, 2023 •

edited

Loading