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

[JENKINS-75056] Upgrade pac4j to version 6.1.0 #491

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

pankajy-dev
Copy link

Testing done

image

The plugin is compiling and passing all the tests.

Jenkins issue 75056

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

@pankajy-dev pankajy-dev requested a review from a team as a code owner December 23, 2024 10:12
pom.xml Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
}

/**
* This method is needed as there seems to be a bug in pac4j and hasChanged is not able to return true
Copy link
Contributor

Choose a reason for hiding this comment

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

I am curious: Is there an official bug report link? Maybe helpful to add this here...

Copy link
Member

Choose a reason for hiding this comment

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

@@ -1376,12 +1400,10 @@ public boolean handleTokenExpiration(HttpServletRequest httpRequest, HttpServlet
}

private void redirectToLoginUrl(HttpServletRequest req, HttpServletResponse res) throws IOException {
if (req != null && (req.getSession(false) != null || Strings.isNullOrEmpty(req.getHeader("Authorization")))) {
if (req.getSession(false) != null || Strings.isNullOrEmpty(req.getHeader("Authorization"))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I added the null check on purpose (see https://github.com/jenkinsci/oic-auth-plugin/releases/tag/4.438.v6e62f6782770).

Suggestion: add @NonNull to the parameters to tighten the contract.

if (res != null) {
res.sendRedirect(Jenkins.get().getSecurityRealm().getLoginUrl());
}
res.sendRedirect(Jenkins.get().getSecurityRealm().getLoginUrl());
Copy link
Contributor

Choose a reason for hiding this comment

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

I added the null check on purpose (see https://github.com/jenkinsci/oic-auth-plugin/releases/tag/4.438.v6e62f6782770).

Suggestion: add @NonNull to the parameters to tighten the contract.

}

/**
* This method is needed as there seems to be a bug in pac4j and hasChanged is not able to return true
Copy link
Member

Choose a reason for hiding this comment

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

@fcojfernandez fcojfernandez changed the title Jenkins 75056 pac4j upgrade 6.1.0 [JENKINS-75056] Upgrade pack4j to version 6.1.0 Dec 23, 2024
… like jackson lib coming from jackson2 plugin
@pankajy-dev pankajy-dev changed the title [JENKINS-75056] Upgrade pack4j to version 6.1.0 [JENKINS-75056] Upgrade pac4j to version 6.1.0 Dec 23, 2024
…solver.java

Co-authored-by: Francisco Javier Fernandez <[email protected]>
@eva-mueller-coremedia
Copy link
Contributor

I wonder if this PR super-seeds #455?

@pankajy-dev
Copy link
Author

I wonder if this PR super-seeds #455?

Yes, seems like the same change.

Comment on lines +54 to +55
<springVersion>6.1.14</springVersion>
<jacksonVersion>2.18.1</jacksonVersion>
Copy link
Member

Choose a reason for hiding this comment

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

unused?

@pankajy-dev
Copy link
Author

CI is failing as pom uses 5.3 version of plugin-pom which needs min maven 3.9.6

I think CI needs an update to use the updated Maven version.

@michael-doubez
Copy link
Contributor

Depends on #485

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.

5 participants