Skip to content

Login Failures Map JPA implementation#11229

Merged
hmlnarik merged 1 commit intokeycloak:mainfrom
sguilhen:9664-login-failure-jpa-store
Apr 22, 2022
Merged

Login Failures Map JPA implementation#11229
hmlnarik merged 1 commit intokeycloak:mainfrom
sguilhen:9664-login-failure-jpa-store

Conversation

@sguilhen
Copy link
Copy Markdown
Contributor

@sguilhen sguilhen commented Apr 11, 2022

Closes #9664

@sguilhen sguilhen added area/storage Indicates an issue that touches storage (change in data layout or data manipulation) team/storage-sig labels Apr 11, 2022
@sguilhen
Copy link
Copy Markdown
Contributor Author

sguilhen commented Apr 12, 2022 •
edited
Loading

This will require rebasing if the realms JPA storage PR is merged first. Also, base testsuite will present failures with the user login failure JPA storage until #11230 is resolved.

@hmlnarik hmlnarik requested a review from vramik April 12, 2022 10:29
@hmlnarik
Copy link
Copy Markdown
Contributor

hmlnarik commented Apr 12, 2022

Reminder: log a warning that it would fail certain tests with reference to the GH issue if the login failure JPA store is obtained

@sguilhen
Copy link
Copy Markdown
Contributor Author

sguilhen commented Apr 12, 2022

Reminder: log a warning that it would fail certain tests with reference to the GH issue if the login failure JPA store is obtained

Ok, do you have a suggestion where exactly to put this? I thought about adding it here - basically check if the model type is the UserLoginFailureModel and log the message.

I thought about adding it to the MapUserLoginFailureProvidider, but there I can't check the type of the store (different modules, so the JPA classes are not visible) to be able to log a message. And I also don't think we should always log the message - if I understood it correctly, the intention is to warn the message only when the user login failure provider is relying on the JPA map storage.

@hmlnarik
Copy link
Copy Markdown
Contributor

hmlnarik commented Apr 14, 2022

Ok, do you have a suggestion where exactly to put this? I thought about adding it here - basically check if the model type is the UserLoginFailureModel and log the message.

The 👍🏻 above could have been missed, so let me state that I agree with this suggestion in a separate comment

@sguilhen sguilhen force-pushed the 9664-login-failure-jpa-store branch from c715762 to 55e837e Compare April 14, 2022 20:16
@sguilhen sguilhen requested a review from hmlnarik April 14, 2022 20:17
@sguilhen sguilhen force-pushed the 9664-login-failure-jpa-store branch from 55e837e to c639dab Compare April 14, 2022 20:51
vramik
vramik reviewed Apr 20, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@vramik vramik left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @sguilhen.

Comment thread .../java/org/keycloak/models/map/storage/jpa/loginFailure/entity/JpaUserLoginFailureEntity.java Outdated
Copy link
Copy Markdown
Contributor

@vramik vramik Apr 20, 2022

Choose a reason for hiding this comment

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

just a reminder: once #10901 is merged:

Suggested change
this.id = id == null ? null : UUID.fromString(id);
String validatedId = UuidValidator.validateAndConvert(id);
this.id = UUID.fromString(validatedId);

Copy link
Copy Markdown
Contributor Author

@sguilhen sguilhen Apr 20, 2022

Choose a reason for hiding this comment

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

Done, thank you @vramik

@sguilhen sguilhen force-pushed the 9664-login-failure-jpa-store branch 2 times, most recently from b93a414 to c7c43c3 Compare April 20, 2022 17:36
@sguilhen
Copy link
Copy Markdown
Contributor Author

sguilhen commented Apr 20, 2022

Rebased after #10901 was merged. Should be good to go.

@sguilhen sguilhen requested a review from vramik April 20, 2022 17:42
@sguilhen sguilhen mentioned this pull request Apr 21, 2022
Merged
@hmlnarik
Copy link
Copy Markdown
Contributor

hmlnarik commented Apr 21, 2022

#11375 has been merged, please rebase and change the foreign keys accordingly.

vramik
vramik reviewed Apr 21, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@vramik vramik left a comment

Choose a reason for hiding this comment

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

Thank you @sguilhen, LGTM! Once rebased and the foreign keys updated it's good to go.

@sguilhen sguilhen force-pushed the 9664-login-failure-jpa-store branch from c7c43c3 to d501eb0 Compare April 21, 2022 13:36
@sguilhen
Copy link
Copy Markdown
Contributor Author

sguilhen commented Apr 21, 2022

@vramik @hmlnarik Done, updated the PR!

@hmlnarik
Copy link
Copy Markdown
Contributor

hmlnarik commented Apr 21, 2022

@sguilhen Could you please rebase, seems the admin console broke the build

@sguilhen sguilhen force-pushed the 9664-login-failure-jpa-store branch from d501eb0 to 2df1985 Compare April 21, 2022 15:02
@sguilhen
Copy link
Copy Markdown
Contributor Author

sguilhen commented Apr 21, 2022

@sguilhen Could you please rebase, seems the admin console broke the build

Done, build should succeed now.

hmlnarik
hmlnarik approved these changes Apr 22, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@hmlnarik hmlnarik left a comment

Choose a reason for hiding this comment

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

Approving per @vramik 's review. Thanks for the PR!

@hmlnarik hmlnarik merged commit 0c7a8c8 into keycloak:main Apr 22, 2022
@sguilhen sguilhen deleted the 9664-login-failure-jpa-store branch April 22, 2022 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@vramik vramik vramik approved these changes

@hmlnarik hmlnarik hmlnarik approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@hmlnarik hmlnarik

Labels

area/storage Indicates an issue that touches storage (change in data layout or data manipulation)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

JPA map storage: Login failure no-downtime store

3 participants

@sguilhen @hmlnarik @vramik