Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(openid-connect): openid not closing session and blocking until ttl… #10788

Merged

Conversation

markusmueller
Copy link
Contributor

@markusmueller markusmueller commented Jan 9, 2024

…l expired when using lockable session storage backend (#8017)

Description

Fixes #8017

I'm not a Lua developer but I'd really like to see this fixed. It might be appropriate to wrap with a pcall and have the session closed in any case (try-catch-finally). Also, I couldn't quickly figure out if the testing would cover configuring and spinning up a locking session storage (memcache for example) to test this. If so, happy to add a test case.

Thank you for your feedback.

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

@markusmueller markusmueller changed the title [WIP] fix(openid-connect): openid not closing session and blocking until ttl… fix(openid-connect): openid not closing session and blocking until ttl… Jan 9, 2024
@markusmueller markusmueller marked this pull request as draft January 9, 2024 09:00
@markusmueller markusmueller marked this pull request as ready for review January 9, 2024 09:01
@monkeyDluffy6017
Copy link
Contributor

We need a test case to cover this

@sheharyaar
Copy link
Contributor

@markusmueller , can you please add a test case for the PR??

@markusmueller
Copy link
Contributor Author

markusmueller commented Jan 11, 2024

@sheharyaar I have added a test verifying the session lock is not blocking subsequent requests anymore as the session is closed properly (test will fail without the change in the plugin).

…l expired when using lockable session storage backend (apache#8017)
Co-authored-by: Liu Wei <375636559@qq.com>
@markusmueller
Copy link
Contributor Author

@monkeyDluffy6017 applied your suggestions, thank you

@monkeyDluffy6017 monkeyDluffy6017 added approved and removed wait for update wait for the author's response in this issue/PR user responded labels Jan 15, 2024
@monkeyDluffy6017 monkeyDluffy6017 merged commit 2d47b4b into apache:master Jan 17, 2024
44 checks passed
@markusmueller markusmueller deleted the fix-openid-session-close branch January 17, 2024 08:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

openid-connect plugin leaving server-side sessions locked by not calling explicit session:close()
4 participants