Replies: 7 comments 17 replies
-
|
I think that:
is completely fair, as it will step away from a number of real and possible issues. |
Beta Was this translation helpful? Give feedback.
-
|
If you decide to do that, please document migration path for clients that already have space in their realm. Looks like manual hacking in database would be required |
Beta Was this translation helpful? Give feedback.
-
|
Hi @djotanov and thank you for your feedback! This would not most probably affect existing realms. However, we might hit validation issues when modifying such Realm. @DGuhr Realm renaming was always a bit buggy. This will have to be properly tested. |
Beta Was this translation helpful? Give feedback.
-
|
Realm updates is what I am talking about. We have automated process of realm upgrades, where we update realm JSON, then export old realm with users, replace old realm JSON with updated one, and reimport it back with overwrite. I'm sure many keycloak users are doing that, as there is no better way to do it at the moment (this might be simpler in the future with new store). |
Beta Was this translation helpful? Give feedback.
-
|
That's a good point. However, as far as I can tell, we never supported whitespaces properly. It is broken at multiple places (incl. token verification as Dominik stated). The suggestion is to improve the UX around it to make it clear whitespaces are not supported. |
Beta Was this translation helpful? Give feedback.
-
|
Thanks for creating this discussion @DGuhr. We discussed this subject some time ago in a couple of Jiras. The amount of breaking changes and the effort required to support whitespace, from my perspective, does not worth it. If we take URLs as an example, they are supported, but considered a bad practice at the same time. cc @keycloak/kc-developers |
Beta Was this translation helpful? Give feedback.
-
|
ok, so it is pretty clear that we do not want to support this, makes total sense from my side, I just did not want to bias anyone by putting my opinion out first. @mposolda actually there is already a regression to older keycloak versions it seems, so a deprecation would not bring much imo, as it is unusable as is. See this video: https://youtu.be/3OhurnqPsLk Even when you set the encoded whitespace manually, it does not work to access the account console ootb, or am I missing something? @djotanov mentions here, that it works for him. This seems a bit weird for me, as the video shows it does not, so -i guess I am missing sth here. If not, it is just broken imo, and even some checks are in place to avoid adding whitespaces when creating a client (see video), just not the check for when we create / import a realm. So if I dont miss sth we don't need to deprecate it, but could just add the proper validation to the appropriate points. wdyt? |
Beta Was this translation helpful? Give feedback.
-
|
see also e.g. stians answer here: #5375 (comment) and pedros answer here and his referenced JIRA tickets: #11921 (comment) - imo we should just remove the capability to create realms with whitespaces in it by adding proper validation, but I am fine when someone tells me we have to deprecate it first, I just need to know what I did wrong, because it seems completely broken to me, so no need to deprecate a broken feature ;) |
Beta Was this translation helpful? Give feedback.
-
|
@DGuhr I've just tried to create keycloak realm called "foo bar" and it works fine. I am not able to login to account console by default, but after changing "Valid redirect URL" of the That is why I propose the "deprecation period" to be on the safe side regarding backwards compatibility. |
Beta Was this translation helpful? Give feedback.
-
|
+1 for deprecation. We are currently able to upgrade our realm with whitespace to the latest keycloak version, and everything works fine (we don't use account pages). |
Beta Was this translation helpful? Give feedback.
-
|
@mposolda @djotanov thanks for clarifying, then i'd also say we need a deprecation period. Forgot about the wildcards, also it seems to work under some circumstances, so perhaps "account" console is not the best example. So @keycloak/kc-developers @keycloak/kc-maintainers question is how to proceed:
|
Beta Was this translation helpful? Give feedback.
-
|
I think whitespaces is not good and if developers create realm with 2 words with whitespace then replace whitespace with dash symbol or underscore symbol. |
Beta Was this translation helpful? Give feedback.
-
|
I believe that replacing whitespaces with underscores by default is not a way to go. I can imagine situations where caller could be surprised by this replacement. I think there should be rather validation done during the creation of the realm. |
Beta Was this translation helpful? Give feedback.
-
|
One question related to this. Should we use realm names in URLs at all? Shouldn't we aim for referencing realms only by IDs and use names only in UI? In my opinion, switching to IDs would also help with performance. For example, for some storage implementations, searching by ID can be better optimized than searching by some first level entity field (in this case realm name). WDYT? cc. @keycloak/storage-x |
Beta Was this translation helpful? Give feedback.
-
|
Good point. I think the Realm name and ID is the same thing in most cases. AFAIK we don't use UUID for Realm IDs(?). |
Beta Was this translation helpful? Give feedback.
-
|
If I am not mistaken (@sguilhen could you please confirm), starting with #10901 we use UUIDs in the new storage JPA implementation. |
Beta Was this translation helpful? Give feedback.
-
|
Realm names could be used in the URLs as long as they are unique - which should be the case - and as long as the lookup is done by realm name - which is the case (see That said, no format of ID is guaranteed in the store - ID is store's responsibility, and store gives no guarantee apart from that it can be converted to and from String.
|
Beta Was this translation helpful? Give feedback.
-
I think this is the answer to the question whether we should use IDs in the URLs. Since the IDs generation depends on the store implementation, we should use something more consistent – i.e. keep using the name. |
Beta Was this translation helpful? Give feedback.
-
|
Removing support for spaces in realm names is all fine, as long as you provide instructions for users with existing realms with whitespace how to update their realm name. Updating realm name through the admin UI currently does not work. The realm name gets updated, but the base URLs and redirect URLs remain the same in database. We would either need instructions for what manual database changes we would need to do, or fix the realm name update functionality. |
Beta Was this translation helpful? Give feedback.
-
|
Actually update of realm name through the UI works as expected in latest version of keycloak, so that's fine |
Beta Was this translation helpful? Give feedback.
-
|
White-spaces (and other special characters) should not be permitted in Realm name by default. However, as there is a number of places where we should be more restrictive in what is permitted when adding more validation we need to consider backwards compatibility. For any new and more restrictive validation we should support disabling the validation. I don't want to add more bespoke validation though, and would like to address this as a part of better/unified validation in general. |
Beta Was this translation helpful? Give feedback.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
-
Hi everyone,
before going in the details, here's the tl;dr version:
Situation
createa realm where the name/id has a whitespace. This seemed to work in previous Versions of Keycloak (6.0 at least, see Validation for realm name not containing space OLD DESCRIPTION: Realm import fails with "Base URL is not a valid URL; A redirect URI is not a valid URI" #11921 )baseUrlandredirectUriof the client, and also on theissfield in the access token, as this does not get properly encoded/decoded.Conclusion
Today it is possible to create a non-usable realm. This is not wanted.
So two options I can see:
iss) of the OIDC Token.Question(s)
What option should we go with? Should we say "it is not supported"? Then we'd need to have client- and serverside checks when creating / importing a realm, with proper error messages to guide ppl. Also, this seems to be a regression (see #11921, answers from @djotanov ). OR: Should we add support for realms with whitespaces. This is a bigger change, and touches some sensible areas like the
issfield parsing for the tokens in order to work correctly.Long version
While investigating a supposedly small problem with the new "Import at startup" feature in Quarkus based Keycloak in #11921, I discovered that this issue is also happening in Wildfly.
Initial investigation in this Draft PR: #11964 showed that the problem was in the validation of the
ClientModelthat is imported, so first fix version was to use a simpleuri.replaceAll("\\s+","%20")in the import.The realm could get imported correctly.
Not sure about the sideeffects here, I changed the draft PR #11964 to the current state, where it for now sanitizes the whitespaces to %20 before importing.
Then I tested creating a user and logging in and discovered, that while logging in to the account console worked, logging in to the admin console led to a 401 error.
You can see it in action in the following video:
https://youtu.be/PrEzdw28j0Y
I also tested what happens with an OOTB Keycloak 18.0 installation, when you create such a realm with a whitespace in the UI. Result is you are not able to log in due to a wrong redirectUri set. You can see this in action in the following videos:
https://youtu.be/K9CbedIdgS0 / https://youtu.be/p5oOJOd_2ek
Long story short
At the moment we have inconsistent and not functional behaviour when a realm with whitespace is created. Allowing this needs a bigger change, touching some more sensitive areas, so I wanted to reach out to ask you if we should go this path or if we should add the proper checks to not allow this at all instead.
@vmuzikar Feel free to add sth, and thanks for help investigating this :)
Beta Was this translation helpful? Give feedback.
All reactions