Add OpenID Connect patron authentication support (PP-3473)#3038
Add OpenID Connect patron authentication support (PP-3473)#3038
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3038 +/- ##
==========================================
+ Coverage 93.04% 93.16% +0.11%
==========================================
Files 480 487 +7
Lines 43716 44787 +1071
Branches 6027 6173 +146
==========================================
+ Hits 40677 41726 +1049
- Misses 1968 1985 +17
- Partials 1071 1076 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
dbernstein
left a comment
There was a problem hiding this comment.
@tdilauro : amazing work. There are just a few suggestions here. I'm not 100% sure the lack of redirect_url validation is a real problem but something to consider. I'm giving this a thumbs up. Feel free make changes as you see fit.
|
Thanks for the speedy feedback, @dbernstein. I'll go through it tomorrow to fix and respond. There's at least one of these that I was planning to address in a future PR, but I need to review more closely. |
jonathangreen
left a comment
There was a problem hiding this comment.
Nice work on this, @tdilauro. There’s a lot of solid work here 🚀. I added a few minor comments for your consideration, plus a couple of items I think should be addressed before this is merged.
Sorry for jumping in after @dbernstein’s review. I started reviewing this yesterday before his feedback was posted, so I decided to finish my pass.
One higher-level question: was there a reason we didn’t lean more on Authlib here? We already depend on it, and some of this appears to duplicate protocol logic Authlib can handle (auth URL/state/PKCE, token exchange, ID token validation, and logout URL building). I don’t want to block the PR on this question at all, I’m mainly curious why you chose this approach.
jonathangreen
left a comment
There was a problem hiding this comment.
Just adding a couple more minor comments I noticed after my other review.
c9f4bd5 to
30afb48
Compare
30afb48 to
8c299ce
Compare
@jonathangreen I had actually started some of the work before I picked a library for this. And after I pulled authlib in, it wasn't as easy for me to debug and see what was going on, and there were some annoying issues with how it reported some errors one at a time. That said, using authlib will save us some code and maintenance, so I do think it will be worthwhile to eventually use it where we can. |
Description
This change adds OpenID Connect (OIDC) patron authentication support to Palace Manager. Where possible, this implementation uses patterns from the already-present SAML implementation.
Motivation and Context
Using OIDC instead of SAML for authentication flows offers better user experience for patrons, since session refresh can be managed within the protocol.
Follow the pattern that we already use for SAML flows allows us:
[Jira PP-3473]
How Has This Been Tested?
Checklist