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

Make new Cookie handling extendable / CookieTypeBuilder public for custom cookies #28235

Closed
joeri-jansen opened this issue Mar 27, 2024 · 5 comments
Assignees
Labels
kind/enhancement Categorizes a PR related to an enhancement

Comments

@joeri-jansen
Copy link

Description

Since Keycloak 24, the cookie handling is refactored base on these issues:
#26500
#26847

However, the CookieType class is not extendible / usable in other parts of the code. We have some custom login components (e.g. automated identity brokering when you have multiple IDPs) that require additional cookies to be set. In the new setup, it is not possible to use the CookieProvider and CookieType for other cookies, e.g. a cookie to store a username or previously selected IDP.

Is it possible to make the CookieTypeBuilder visible / public, so it can be used to create other cookies as well?

Discussion

No response

Motivation

It is possible to create a complete new cookie SPI for our own extensions, however, this is a bit of an overkill for simple extension of the cookies and reduces the maintainabillity of our loginflow extensions.

Making the CookieTypeBuilder public is not a risk.

Details

No response

@joeri-jansen joeri-jansen added kind/enhancement Categorizes a PR related to an enhancement status/triage labels Mar 27, 2024
@jonkoops
Copy link
Contributor

I don't think we actually want to make this publicly extendable. What is preventing you from setting cookies using regular JAX-RS calls?

@joeri-jansen
Copy link
Author

@jonkoops It is indeed possible to read/set custom cookies in our own code. However, this includes duplication of code that is also present in the DefaultCookieProvider and was previously present in the CookieHelper util. Duplication of code introduces the risk of bugs and lowers maintainabillity of our extensions, thats why we created this ticket. Not using the new refactored code of the cookie handling for other purposes just seems to be a waste.

Not making the CookieTypeBuilder publicly available is a descission we can live with, but it was worth asking...

@jonkoops
Copy link
Contributor

jonkoops commented Apr 3, 2024

@stianst what is your take on this?

@stianst
Copy link
Contributor

stianst commented Apr 5, 2024

There is no plans to extend on CookieProvider to support custom cookies, instead just use:

NewCookie newCookie = new NewCookie.Builder("mycookie")
                    .maxAge(1232)
                    .value("myvalue")
                    .path(session.getContext().getUri().getRequestUri().getRawPath())
                    .build();
            session.getContext().getHttpResponse().setCookieIfAbsent(newCookie);

I'm reluctant to support custom cookies in the cookie provider for two reasons; one the APIs there may change quite a bit, and secondly it is intended to manage internal cookies, and not custom cookies.

@jonkoops
Copy link
Contributor

jonkoops commented Apr 5, 2024

Closing this issue for the reasons above.

@jonkoops jonkoops closed this as not planned Won't fix, can't repro, duplicate, stale Apr 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement Categorizes a PR related to an enhancement
Projects
None yet
Development

No branches or pull requests

3 participants