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

KNOX-3077 - Add pac4j.cookie.max.age param #972

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

Conversation

moresandeep
Copy link
Contributor

What changes were proposed in this pull request?

This PR introduces a new parameter pac4j.cookie.max.age for the Pac4J provider that enforces cookie age on the cookies created by the pac4j provider.
e.g.

         <provider>
                  <role>federation</role>
                  <name>pac4j</name>
                  <enabled>true</enabled>
                  <param>
                      <name>pac4j.callbackUrl</name>
                      <value>https://www.local.com:8443/gateway/knoxsso/api/v1/websso</value>
                  </param>

                 <!-- Unit is seconds -->
                  <param>
                      <name>pac4j.cookie.max.age</name>
                      <value>180</value>
                  </param>
            .........

Note that these value needs to be same as knoxsso.cookie.max.age if you want Knox session to expire at the same time.

How was this patch tested?

This patch was tested locally.

@moresandeep moresandeep self-assigned this Dec 12, 2024
@moresandeep moresandeep added the SSO Knox SSO label Dec 12, 2024
public static final String PAC4J_COOKIE_MAX_AGE = "pac4j.cookie.max.age";

/* default value is same is KNOXSSO token ttl default */
public static final int PAC4J_COOKIE_MAX_AGE_DEFAULT = -1;
Copy link
Contributor

Choose a reason for hiding this comment

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

This could - should - be private (not used anywhere outside of this class) and String instead of int (i.e. "-1", to avoid unnecessary conversion below)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed! thanks for the review!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@smolnar82 actually PAC4J_COOKIE_MAX_AGE is used outside of this class in KnoxSessionStore which needs this to be public.
I did update the int to String thanks for pointing it out!

Copy link
Contributor

Choose a reason for hiding this comment

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

@moresandeep - yes, I know the parameter name is used elsewhere, that's why I added a comment on the default value only 😊
Thanks for applying the changes so quickly.

Copy link
Contributor

@smolnar82 smolnar82 left a comment

Choose a reason for hiding this comment

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

LGTM

@smolnar82
Copy link
Contributor

Please don't forget to file a Doc Jira with this new config.

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

Successfully merging this pull request may close these issues.

2 participants