8000 Enable MapUserProvider storing username with the letter case signific… by vramik · Pull Request #13183 · keycloak/keycloak · GitHub
[go: up one dir, main page]
Skip to content

Enable MapUserProvider storing username with the letter case signific… - #13183

Merged
hmlnarik merged 1 commit into
keycloak:mainfrom
vramik:10245-username-lettercase
Sep 9, 2022
Merged

Enable MapUserProvider storing username with the letter case signific…#13183
hmlnarik merged 1 commit into
keycloak:mainfrom
vramik:10245-username-lettercase

Conversation

@vramik
@vramik vramik commented Jul 18, 2022
Copy link
Copy Markdown
Contributor

…ance

Closes #10245

@vramik vramik mentioned this pull request Jul 18, 2022
Closed
@vramik vramik added kind/enhancement Categorizes a PR related to an enhancement area/storage Storage layer — JPA entities, database schemas, Liquibase migrations, caching team/storage-sig labels Jul 18, 2022
@vramik
vramik requested a review from hmlnarik July 18, 2022 13:50
sguilhen
sguilhen reviewed Jul 20, 2022
View reviewed changes
@sguilhen sguilhen left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@vramik Thanks for getting into this. I've reviewed the whole thing and I must say I find it a bit confusing for the user provider to check a property set in the realm storage config. I say this because the storages themselves (JPA, hotrod, etc) never actually use this property and it feels like this switch has been put in the wrong place.

Perhaps I'm failing to see how this is supposed to work with realm-specific stores, but I have to say that this switch is really confusing to me.

@vramik
vramik force-pushed the 10245-username-lettercase branch 2 times, most recently from 9c4bfeb to a60c0dc Compare July 27, 2022 15:52
@vramik
vramik marked this pull request as ready for review July 27, 2022 21:56
@vramik
vramik force-pushed the 10245-username-lettercase branch from a60c0dc to 31f86e6 Compare July 27, 2022 22:00
@vramik
vramik requested review from mhajas and sguilhen July 28, 2022 07:40
@vramik
vramik commented Jul 28, 2022 •
edited
Loading
Copy link
Copy Markdown
Contributor Author

@vramik Thanks for getting into this. I've reviewed the whole thing and I must say I find it a bit confusing for the user provider to check a property set in the realm storage config. I say this because the storages themselves (JPA, hotrod, etc) never actually use this property and it feels like this switch has been put in the wrong place.

You're right it was misplaced at that stage of the draft PR. The setting now is taken from realm attribute (if the realm is available) upon creation of user provider. Default setting is case-insensitive.

The implementation follows #10198 (comment) and also incorporates #10198 (comment) and should be ready for a review

@sguilhen @mhajas @hmlnarik could you please take a look?

mhajas
mhajas reviewed Jul 28, 2022
View reviewed changes
@mhajas mhajas left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you for the PR @vramik, I added some comments

Comment thread model/map/src/main/java/org/keycloak/models/map/user/MapUserEntity.java Resolved
Comment thread model/map/src/main/java/org/keycloak/models/map/user/MapUserProviderFactory.java Resolved
sguilhen
sguilhen reviewed Aug 1, 2022
View reviewed changes
@sguilhen sguilhen left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I know this is still pending some feedback from @hmlnarik , but I have to say that overall this looks a lot better and I like it that the config is now coming from a realm attribute.

hmlnarik
hmlnarik suggested changes Aug 11, 2022
View reviewed changes
@hmlnarik hmlnarik left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you @vramik, this PR is going in the right direction.

Please see a few inline comments. After addressing the two main comments re the session attribute and lowercase variant of the username, it should be good to be merged

Comment thread ...-jpa/src/main/java/org/keycloak/models/map/storage/jpa/user/JpaUserModelCriteriaBuilder.java Resolved
Comment thread model/map/src/main/java/org/keycloak/models/map/user/MapUserProvider.java Resolved
Comment thread model/map/src/main/java/org/keycloak/models/map/user/MapUserProviderFactory.java Resolved
Comment thread model/map/src/main/java/org/keycloak/models/map/user/MapUserEntity.java Resolved
@vramik
vramik force-pushed the 10245-username-lettercase branch 2 times, most recently from f922672 to 0c20e31 Compare August 29, 2022 17:03
hmlnarik
hmlnarik reviewed Aug 29, 2022
View reviewed changes
@hmlnarik hmlnarik left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would it be possible to add integration tests which scenarios where

  • realm with case sensitivity search is (a) enabled, (b) disabled
  • create a user
  • search for the user by (a) uppercase, (b) original letter case, (c) lowercase username
  • switch the case sensitivity flag to the other state
  • search as previously

Comment thread model/map-jpa/src/main/resources/META-INF/users/jpa-users-changelog-1.xml Resolved
Comment thread model/map/src/main/java/org/keycloak/models/map/user/MapUserProviderFactory.java Resolved
Comment thread server-spi-private/src/main/java/org/keycloak/userprofile/DefaultAttributes.java Resolved
Comment thread server-spi/src/main/java/org/keycloak/models/UserModel.java Resolved
Comment thread model/map/src/main/java/org/keycloak/models/map/user/MapUserProvider.java Resolved
@vramik
vramik force-pushed the 10245-username-lettercase branch 3 times, most recently from 31472f6 to b7eccc6 Compare August 30, 2022 11:55
@vramik
vramik requested a review from hmlnarik August 30, 2022 14:39
mhajas
mhajas previously approved these changes Sep 5, 2022
View reviewed changes
@mhajas mhajas left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @vramik! Great work. I like it this way. There is one thing that caught my attention, but it is mostly my curiosity because we are doing quite the opposite in HotRod.

Comment thread model/map/src/main/java/org/keycloak/models/map/storage/chm/MapFieldPredicates.java Resolved
@vramik
vramik force-pushed the 10245-username-lettercase branch 2 times, most recently from 0a007d5 to 12fe932 Compare September 7, 2022 13:52
@vramik
vramik requested a review from hmlnarik September 7, 2022 17:34
@vramik
vramik force-pushed the 10245-username-lettercase branch from 12fe932 to 90feeb2 Compare September 9, 2022 07:01
@vramik
vramik force-pushed the 10245-username-lettercase branch from 90feeb2 to 0dd7db9 Compare September 9, 2022 07:11
@hmlnarik
hmlnarik commented Sep 9, 2022
Copy link
Copy Markdown
Contributor

undertow-map-jpa, group1 failed due to #14270

hmlnarik
hmlnarik approved these changes Sep 9, 2022
View reviewed changes
@hmlnarik hmlnarik left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the PR!

@hmlnarik
hmlnarik merged commit 869ccc8 into keycloak:main Sep 9, 2022
@vramik
vramik deleted the 10245-username-lettercase branch September 13, 2022 19:18
@ahus1 ahus1 mentioned this pull request Sep 30, 2022
Closed
@vramik vramik mentioned this pull request Oct 7, 2022
Merged
@codespearhead codespearhead mentioned this pull request Oct 17, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

3 more reviewers
@sguilhen sguilhen sguilhen left review comments
@hmlnarik hmlnarik hmlnarik approved these changes
@mhajas mhajas mhajas left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@hmlnarik hmlnarik

Labels

area/storage Storage layer — JPA entities, database schemas, Liquibase migrations, caching kind/enhancement Categorizes a PR related to an enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Enable MapUserProvider storing username with the letter case significance

4 participants

@vramik @hmlnarik @sguilhen @mhajas
0