Skip to content

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

Merged
juststillthinking merged 2 commits into
apache:masterfrom
markusmueller:fix-openid-session-close
Jan 17, 2024
Merged

fix(openid-connect): openid not closing session and blocking until ttl…#10788
juststillthinking merged 2 commits into
apache:masterfrom
markusmueller:fix-openid-session-close

Conversation

@markusmueller

@markusmueller markusmueller commented Jan 9, 2024

Copy link
Copy Markdown
Contributor

…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
@juststillthinking

Copy link
Copy Markdown
Contributor

We need a test case to cover this

@sheharyaar

Copy link
Copy Markdown
Contributor

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

@markusmueller
markusmueller force-pushed the fix-openid-session-close branch from c228f26 to 8996221 Compare January 11, 2024 13:30
@markusmueller

markusmueller commented Jan 11, 2024

Copy link
Copy Markdown
Contributor Author

@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)
@markusmueller
markusmueller force-pushed the fix-openid-session-close branch from 8996221 to 19a20bf Compare January 11, 2024 21:41
Comment thread apisix/plugins/openid-connect.lua Outdated
@markusmueller

Copy link
Copy Markdown
Contributor Author

@monkeyDluffy6017 applied your suggestions, thank you

@juststillthinking juststillthinking added approved and removed wait for update wait for the author's response in this issue/PR user responded labels Jan 15, 2024
@juststillthinking
juststillthinking merged commit 2d47b4b into apache:master Jan 17, 2024
@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

None yet

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