Repository navigation
KEYCLOAK-18051 Add regex redirect-uri client policy - #8236
patrick-primesign wants to merge 1 commit into
Conversation
7d933d0 to
31c49a7
Compare
|
Sorry for the late reply to this one. Wondering if there's still interest in this PR if so perhaps we can discuss how to proceed. If so could you provide a bit more context behind the approach here? It seems like the idea is that clients can register '*' as redirect-uris, then the client-policy is used to configure a regex-pattern that can check the redirect-uri used by the authorization request. That seems a bit the wrong way around to me. I would have rather expected to have a client-policy that is used to decide what redirect-uris a client can register, so would check on client create/update rather than on authz requests. Wondering if it also might be nice to convert this into a redirect-uri policy and have some more options than just regex to incorporate some of the stuff we've discussed in #9278. |
|
One more thing is that it would be good to separate the client-ids condition into a separate PR. |
|
Added some more thoughts to #9278 (comment) |
|
Hi @stianst. There is still interest in bringing this pull request upstream. We are already using this change in production (since a year) and it works pretty well. The idea behind this PR is to allow (next to wildcards) RegEx for the allowed redirect-uris. However, the specs very clearly state that redirect-uris should be a strict string comparison. Therefore, we decided that an realm admin has to explicitly configure the RegEx by a client policy. Configuration steps:
The RegEx pattern is used by the authorization request to validate the redirect-uri. To understand your suggestion: Your approach would be to register a client policy that decides what kind of valid-redirect uris (wildcard, RegEx) a client may have. Furthermore, adapt the client settings to also allow RegEx (if a client-policy is set for the specific client). And change this method to also support RegEx validation: Your approach also seems justified to me, but it is a deeper change in Keycloak than the current change. A question to your approach: At the client settings, can we query if this client has set the regex client policy? What should happen, if we remove this client at the client-policies? Thanks, |
It is no proplem to seperate the condition into a sperate PR. |
|
There's a more extensive executor in #14659. Perhaps we would want to keep that one, and in this one only keep the client-id condition? |
|
Few points to this:
|
@mposolda great, thanks for the information! I will have a look at the already merged solution (#25637) in the next few days. If our solution is outdated by the solution already merged, then this pull request offers no added value and can be closed. |
|
@CorneliaLahnsteiner Cool, Thanks! Please comment here or close the PR if you are already comfortable with existing approach. Thanks for your contribution regardless! |
|
@CorneliaLahnsteiner @patrick-primesign No further feedback, so I am closing this for now. Just to summary:
But feel free to re-open if you think that some parts of this PR still should be added (or maybe rather create new GH issue and/or PR if something doesn't work as expected in Keycloak 24, which is going to be released soon). |
Documentation-PR: keycloak/keycloak-documentation#1223