8000 Unexpected invalid_grant error on offline session refresh when client… by martin-kanis · Pull Request #17490 · keycloak/keycloak · GitHub
[go: up one dir, main page]
Skip to content

Unexpected invalid_grant error on offline session refresh when client… - #17490

Merged
mhajas merged 1 commit into
keycloak:mainfrom
martin-kanis:9959-offline-client-session-loading
Mar 15, 2023
Merged

Unexpected invalid_grant error on offline session refresh when client…#17490
mhajas merged 1 commit into
keycloak:mainfrom
martin-kanis:9959-offline-client-session-loading

Conversation

@martin-kanis
@martin-kanis martin-kanis commented Mar 7, 2023
Copy link
Copy Markdown
Member

… session is not in the cache

Closes #9959

@martin-kanis
martin-kanis requested a review from a team March 7, 2023 11:16
@ghost ghost added the flaky-test label Mar 7, 2023
This was referenced Mar 7, 2023
Closed
Closed
@martin-kanis
martin-kanis force-pushed the 9959-offline-client-session-loading branch from d54dda6 to b5a59b0 Compare March 7, 2023 14:07
@martin-kanis martin-kanis added the area/storage Storage layer — JPA entities, database schemas, Liquibase migrations, caching label Mar 7, 2023
@martin-kanis
martin-kanis force-pushed the 9959-offline-client-session-loading branch from b5a59b0 to 1f681fa Compare March 7, 2023 14:49
@ghost ghost mentioned this pull request Mar 7, 2023
Closed
@hmlnarik
hmlnarik requested review from a team and vramik March 8, 2023 09:52
vramik
vramik suggested changes Mar 8, 2023
View reviewed changes
@vramik vramik 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 @martin-kanis for the PR! LGTM, I have just one nitpick and one question, Please see inline.

Comment thread ...l/legacy-private/src/main/java/org/keycloak/models/session/UserSessionPersisterProvider.java Outdated
Comment on lines 114 to 118
@vramik vramik Mar 8, 2023
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.

nitpick: missing description of @params and @return. If it's not needed I'd suggest to remove them.

@martin-kanis martin-kanis Mar 14, 2023
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Added

Comment thread model/jpa/src/main/java/org/keycloak/models/jpa/session/JpaUserSessionPersisterProvider.java Resolved
@stianst
stianst requested a review from mposolda March 10, 2023 08:17
@stianst
stianst commented Mar 10, 2023
Copy link
Copy Markdown
Contributor

I think there should never be more than one client session per offline session, as it's really a bug that you can end up with multiple clients sharing an offline session. Would it be better to change the behaviour to that, which should also fix this issue?

@mposolda @sschu FIY

@hmlnarik
hmlnarik commented Mar 13, 2023
Copy link
Copy Markdown
Contributor

I think there should never be more than one client session per offline session, as it's really a bug that you can end up with multiple clients sharing an offline session. Would it be better to change the behaviour to that, which should also fix this issue?

While I agree that the original intention should be improved, this can be done independently, and I'm still in favour of merging this PR (after resolving all the comments).

@sschu
sschu commented Mar 13, 2023 •
edited
Loading
Copy link
Copy Markdown
Contributor

@hmlnarik @stianst Just for completeness, there is also a PR from us (in this case @danielFesenmeyer) who implemented the alternative solution with only one client session per offline user session. #9962

@martin-kanis
martin-kanis force-pushed the 9959-offline-client-session-loading branch from 1f681fa to a33a885 Compare March 14, 2023 12:47
@martin-kanis @lexcao
Unexpected invalid_grant error on offline session refresh when client…
ff288cc 
… session is not in the cache

Closes keycloak#9959

Co-authored-by: Martin Kanis <mkanis@redhat.com>
Co-authored-by: Lex Cao <lexcao@foxmail.com>
@martin-kanis
martin-kanis force-pushed the 9959-offline-client-session-loading branch from a33a885 to ff288cc Compare March 14, 2023 12:49
@martin-kanis martin-kanis mentioned this pull request Mar 14, 2023
Closed
@ghost ghost mentioned this pull request Mar 14, 2023
Closed
vramik
vramik approved these changes Mar 14, 2023
View reviewed changes
@vramik vramik 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 @martin-kanis. From my point of view LGTM!

mhajas
mhajas approved these changes Mar 15, 2023
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 @martin-kanis, LGTM

@mhajas
mhajas merged commit 5e7793b into keycloak:main Mar 15, 2023
@danielFesenmeyer danielFesenmeyer mentioned this pull request May 16, 2023
Closed
@sschu sschu mentioned this pull request Jul 3, 2023
Closed
2 tasks done
@lexcao lexcao mentioned this pull request Sep 20, 2023
Closed
2 tasks done
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mposolda mposolda Awaiting requested review from mposolda
3 more reviewers
@hmlnarik hmlnarik hmlnarik left review comments
@vramik vramik vramik approved these changes
@mhajas mhajas mhajas approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area/storage Storage layer — JPA entities, database schemas, Liquibase migrations, caching flaky-test

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Unexpected invalid_grant error on offline session refresh when maximum number of offline sessions is configured

6 participants

@martin-kanis @stianst @hmlnarik @sschu @vramik @mhajas
0