Open ID Connect authentication implementation#1230
Open ID Connect authentication implementation#1230demiankatz merged 49 commits intovufind-org:devfrom
Conversation
Merge vufind-org/vufind master
Merge vufind-org/vufind master
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
Thanks for this. I've left a few (mostly nitpicky) comments based on a quick browse of the code. Is there a particular aspect of this you are looking to have reviewed more closely? I'm not familiar with the underlying protocol, so I'd have to do some research to give a more substantive review.
|
@xmorave2, thanks for the progress; please let me know when you're ready for me to take another look, and if there's anything more specific I can offer advice on. |
|
@xmorave2, I see you've done some recent work on this. Is it still in progress, or is it ready for potential review and merging? If it's still not ready, it might be worth adding some TODO checkboxes to the top description so we can keep track of progress and know when it is ready (assuming there are clear tasks that can be reduced to a checklist). |
# Conflicts: # module/VuFind/src/VuFind/Auth/Factory.php # module/VuFind/src/VuFind/Auth/PluginManager.php
|
@xmorave2, some of my refactoring for VuFind 6.0 caused conflicts here, so I have resolved them and brought your factory configuration up to date. I'm not able to test this, though, so please double-check that I haven't broken anything, and let me know if you need help with any fixes. Updates aside, what is the status of this project? |
|
@xmorave2, I noticed this had a minor conflict with master; I have resolved it. Let me know if you'd like me to do any further testing/review at this stage. |
|
@xmorave2, just checking on the status of this PR. It's going to need to be brought up to date with Laminas following the migration from Zend. Would you like me to help with that? Is this any closer to being mergeable? No rush, in any case -- just wanted to check where things stand. |
|
@demiankatz I updated this pull request to use Laminas. My collegue is going to test this feature in near future hopefully, so we possibly could make it done... |
|
Thanks, @xmorave2; I'll await further news! |
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
Thanks, @xmorave2 -- see below for a couple of small suggestions. I'll do some hands-on testing once you've made these adjustments (assuming you agree with them).
module/VuFind/tests/unit-tests/src/VuFindTest/Auth/OpenIDConnectTest.php
Outdated
Show resolved
Hide resolved
|
Thanks, @xmorave2, this is looking good to me from the code perspective. I haven't had time to do hands-on testing yet, though, as it seems non-trivial to set up auth0.com. Do you think it might be feasible to set up Facebook login using this mechanism? Since the actual Facebook auth module has been broken since at least 2017 (see VUFIND-1237), it would be good to remove it if we could replace it with this. (Honestly, it would be good to remove it either way, since I doubt anyone needs it -- just wondering if it might be worth my time to try to come up with a sample configuration here that could serve as a replacement for proof-of-concept purposes). |
EreMaijala
left a comment
EreMaijala
left a comment
There was a problem hiding this comment.
HTTP request results need error checking before trying json_decode. Otherwise we may try to pass a plain-text error to json_decode and crash without providing any meaningful error message.
|
@demiankatz I took a quick look and this PR is not able to handle Facebook login without some changes, and you will need a proper configuration as .well-known endpoint of Facebook does not return all the needed information @EreMaijala I tried to ensure there is valid response before trying to call json_decode. In case of not successful response, the AuthException is raised as there is not a way how to continue normally if we do net get needed data... |
|
I've tested the latest commits from this PR and can confirm that onelogin and Auth0 both work without any issues. |
|
@Owen-Fitz: Thank you for good news ;) |
EreMaijala
left a comment
EreMaijala
left a comment
There was a problem hiding this comment.
In troubleshooting it might be useful to log the message in addition to the status code, but I suppose those situations would be rare.
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
Thanks, everyone! This PR has been open for about seven years -- very happy to see it finally completed! :-)
demiankatz
changed the title
|
Thanks all for your testing and reviewing and for your patience! |
|
@xmorave2 I think we need to add some documentation regarding the configuration to the wiki. The settings are quite clear as is, but e.g. the redirect URL should be documented somewhere because it's needed for server-side configuration for servers that are not open to anyone. |
Maybe a good URL for such a wiki page would be https://vufind.org/wiki/configuration:oidc. Feel free to create a page there if you like... and let me know if you need my help setting things up. |
(cherry picked from commit 78e9051)
|
I've created a wiki page at https://vufind.org/wiki/configuration:authentication:oidc but it needs more details filled in by somebody with greater knowledge than me. :-) |
| { | ||
| // Adding the auth_method setting makes it possible to handle logins when | ||
| // using an auth method that proxies others (e.g. ChoiceAuth) | ||
| $targetUri = $target . (str_contains($target, '?') ? '&' : '?') . 'auth_method=oidc'; |
There was a problem hiding this comment.
OpenIDConnect works for me after fixing auth_method here. Should I send a pull request with a trivial fix (see stweil@f0b18b1), or did I miss something?
There was a problem hiding this comment.
Please do! If something is not working for you, and a code change makes it work, that seems worth a PR -- we can discuss the particulars once the PR is open, should further adjustment be needed. (I'm not an expert on this code -- @xmorave2 and @EreMaijala might know more -- but in any case, I see no reason not to get the ball rolling, as it were). Thanks!
5 participants
This is not done, but I would like to have some review on this.
TODO: