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

Add constant cookie name for the OIDC filter #3234

Merged

Conversation

czhou-brex
Copy link
Contributor

@czhou-brex czhou-brex commented Sep 18, 2024

In order to support the logout filter for OIDC, we are proposing a parameter to make the cookie name a constant, so it can be read accordingly.

This is one of the PRs for #3235

Thanks to @Matheus-Michels for the code.

Signed-off-by: Carl Zhou <czhou@brex.com>
Signed-off-by: Carl Zhou <czhou@brex.com>
Signed-off-by: Carl Zhou <czhou@brex.com>
Signed-off-by: Carl Zhou <czhou@brex.com>
Signed-off-by: Carl Zhou <czhou@brex.com>
Signed-off-by: Carl Zhou <czhou@brex.com>
Signed-off-by: Carl Zhou <czhou@brex.com>
Signed-off-by: Carl Zhou <czhou@brex.com>
@czhou-brex czhou-brex changed the title Add constant OIDC cookie name Add constant cookie name for the OIDC filter Sep 18, 2024
Signed-off-by: Carl Zhou <czhou@brex.com>
@czhou-brex czhou-brex marked this pull request as ready for review September 18, 2024 20:58
Signed-off-by: Carl Zhou <czhou@brex.com>
@szuecs
Copy link
Member

szuecs commented Sep 19, 2024

I just want to be clear:
Do you know that this cookie name is only a prefix of the name, because we will split the content into multiple cookies because the size of the data we need in the cookie is often too big for browsers to accept?
So you will only match by "prefix" the cookie names. The number of cookies can change and we have the feature to split into more cookies and merge into less cookies based on data needed.

@szuecs szuecs added the major moderate risk, for example new API, small filter changes that have no risk like refactoring or logs label Sep 19, 2024
@szuecs
Copy link
Member

szuecs commented Sep 19, 2024

👍

1 similar comment
@AlexanderYastrebov
Copy link
Member

👍

@czhou-brex
Copy link
Contributor Author

I just want to be clear: Do you know that this cookie name is only a prefix of the name, because we will split the content into multiple cookies because the size of the data we need in the cookie is often too big for browsers to accept? So you will only match by "prefix" the cookie names. The number of cookies can change and we have the feature to split into more cookies and merge into less cookies based on data needed.

Thanks for explaining this. So far we were only able to observe one skipper cookie for the OIDC session. Can you elaborate on cases where there may be multiple cookies? Are these cases only within auth or outside of auth?

@AlexanderYastrebov AlexanderYastrebov merged commit 3c2a615 into zalando:master Sep 19, 2024
10 checks passed
@AlexanderYastrebov
Copy link
Member

@czhou-brex Thank you for contribution.

Next time please rebase and squash your PR on top of the latest master so we do not have github confused and its easier to write release notes as they are composed from all commit messages

Screenshot from 2024-09-19 18-21-48

Pushpalanka pushed a commit that referenced this pull request Oct 11, 2024
Allow specifying cookie name prefix for OIDC filters

Signed-off-by: Carl Zhou <czhou@brex.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major moderate risk, for example new API, small filter changes that have no risk like refactoring or logs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants