Remove Google OAuth admin authentication provider #802
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #802 +/- ##
==========================================
- Coverage 89.36% 89.31% -0.06%
==========================================
Files 183 181 -2
Lines 32492 32275 -217
Branches 7244 7196 -48
==========================================
- Hits 29037 28825 -212
Misses 2282 2282
+ Partials 1173 1168 -5
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
I see there is significant drop in the code coverage in EDITED: This is due to the fact that we removed only external auth admin provider so there is no way to test that lines currently. |
|
@jonathangreen I left the option (existing code) for other external admin auth providers in the future, is that OK? Should I just completely remove option for other auth providers except classic username/password? |
|
@pcustic I would completely remove the code for external admin auth providers. That should help the code coverage. We can revist it if we need to in the future, but having dead code around that is never called just slows down our development. |
Sure, makes sense 👍 |
| @@ -393,22 +387,6 @@ def collection_library_registrations(): | |||
| ) | |||
|
|
|||
|
|
|||
| @app.route("/admin/admin_auth_services", methods=["GET", "POST"]) | |||
There was a problem hiding this comment.
This will require update to the admin UI, we will need to remove the Admin Auth from the settings so users on the CM cannot (try) to access these endpoints.
There was a problem hiding this comment.
|
@jonathangreen (I think) I removed all the external admin auth stuff. I'll create PR from removing admin auth views in admin UI. I don't quite understand this codecov/patch thing, I'll need to look into that a bit more. |
|
@pcustic now that ThePalaceProject/circulation-admin#75 is merged, and I made a release that includes it https://github.com/ThePalaceProject/circulation-admin/releases/tag/v1.5.1 can you update the package version here: https://github.com/ThePalaceProject/circulation/blob/main/api/admin/config.py#L16 Once that is updated, this should be good to go. |
@jonathangreen updated 0845185! |
Description
This PR removes Google OAuth admin authentication provider (
GoogleOAuthAdminAuthenticationProvider) and all the code related to id. However, it leaves supporting code for having another external authentication provider even though Google OAuth was the only one currently implemented.Motivation and Context
The circulation manager has an admin authentication integration meant to allow admins to authenticate via Google to log into the circulation manager. We are not using this integration anywhere, and the library that it is built on oauth2client is deprecated and not meant to be used anymore.
Notion
How Has This Been Tested?
Removed tests that used Google OAuth, all other tests passed. Also, manually tested sign-in functionality.
Checklist