Skip to content

FlowExecution#getAuthentication2#383

Merged
jglick merged 1 commit intojenkinsci:masterfrom
basil:acegi
Mar 17, 2025
Merged

FlowExecution#getAuthentication2#383
jglick merged 1 commit intojenkinsci:masterfrom
basil:acegi

Conversation

@basil
Copy link
Member

@basil basil commented Mar 3, 2025

@basil basil added the developer label Mar 3, 2025
@basil basil requested a review from a team as a code owner March 3, 2025 18:12
@basil
Copy link
Member Author

basil commented Mar 3, 2025

@jglick Yes, such an implementation is planned but not currently assigned to any particular person.

@jglick
Copy link
Member

jglick commented Mar 3, 2025

That is fine but this should probably stay on hold until the downstream PR has been prepared, as it is technically introducing a new API.

@basil
Copy link
Member Author

basil commented Mar 3, 2025

That is fine but this should probably stay on hold until the downstream PR has been prepared

jenkinsci/jenkins#4848 was not held back by the lack of downstream PRs (cf. jenkinsci/reverse-proxy-auth-plugin#153 (comment)), so I see no reason why this PR should stay on hold.

@jglick
Copy link
Member

jglick commented Mar 3, 2025

There were some; this method is both implemented and called in only one place (AFAIK), workflow-cps, so it only makes sense to evolve it in the context of those two sites.

(Actually workflow-cps could define and call getAuthentication2 even before it is defined in FlowExecution; it would only be the @Override annotation that would depend on this.)

@basil
Copy link
Member Author

basil commented Mar 3, 2025

so it only makes sense to evolve it in the context of those two sites

Hence the plan to update those call sites eventually. Is there a particular reason that has to be done as a prerequisite to merging this PR?

@jglick
Copy link
Member

jglick commented Mar 17, 2025

Yes: matching changes should be prepared in advance where feasible. Filed jenkinsci/workflow-cps-plugin#1002 and will merge this if it passes tests.

@jglick jglick merged commit f1fbe1a into jenkinsci:master Mar 17, 2025
17 checks passed
* @deprecated use {@link #getAuthentication2()}
*/
@Deprecated
public /* abstract */ @NonNull org.acegisecurity.Authentication getAuthentication() {
Copy link

@cyrille-leclerc cyrille-leclerc Apr 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This commit causes:

java.lang.IllegalArgumentException: The specified derived class (org.jenkinsci.plugins.workflow.cps.CpsFlowExecution) does not derive from the specified base class (org.springframework.security.core.Authentication).
	at hudson.Util.isOverridden(Util.java:1536)
	at hudson.Util.ifOverridden(Util.java:1564)
	at PluginClassLoader for workflow-api//org.jenkinsci.plugins.workflow.flow.FlowExecution.getAuthentication(FlowExecution.java:266)
	at PluginClassLoader for opentelemetry//io.jenkins.plugins.opentelemetry.job.MonitoringPipelineListener.onAtomicStep(MonitoringPipelineListener.java:209)

It's impacting the OpenTelemetry plugin:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#391

return Util.ifOverridden(
() -> org.acegisecurity.Authentication.fromSpring(getAuthentication2()),
Authentication.class,
getClass(),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getClass() will return values like org.jenkinsci.plugins.workflow.cps.CpsFlowExecution which do NOT derive from org.springframework.security.core.Authentication

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants