|
Description
David Cook
2023-06-30 03:35:23 UTC
(In reply to David Cook from comment #0) > However, all we need to do is save the current path in the user's session, > and then recall it after the successful login. No, not the current path, because that's the path to the API. We either need to save the path of the previous request, or we need to use the HTTP_REFERER, which is problematic since it's a user-provided HTTP header. We can validate the URL in the HTTP_REFERER, although if you initiated a login from "/cgi-bin/koha/opac-main.pl?logout.x=1" then you'd be redirected to a logout page. But we can't leave off the query string because sometimes you'd want to be redirected to "/cgi-bin/koha/opac-detail.pl?biblionumber=29" for instance. *** Bug 24261 has been marked as a duplicate of this bug. *** Created attachment 163127 [details] [review] Return to the referring URL when signing in using GoogleOpenID Just threw together a quick patch which enables this. This also strips out the logout parameter from the referring URL, so that we don't end up in a loop. It would be good to also check that the referring URI is part of the koha installation, and if not go to the main library page. opac/cgi-bin/opac/svc/auth/googleopenidconnect is not a valid Koha path in the git. It also looks like this is based off a different version of ./opac/svc/auth/googleopenidconnect than what exists in Koha's master branch. I think too the goal is to deprecate and remove ./opac/svc/auth/googleopenidconnect in favour of the new generic OIDC functionality. Created attachment 163128 [details] [review] Redirect to referring URL, excluding logout parameters A quick patch which enables this. This also strips out the logout parameter from the referring URI, so that we don't end up in a loop. It also checks that the referring URI is part of the koha installation, and if not goes to either the main page or the user page. Created attachment 163129 [details] [review] Patch to redirect on login A quick patch which enables this. This also strips out the logout parameter from the referring URI, so that we don't end up in a loop. It also checks that the referring URI is part of the koha installation, and if not goes to either the main page or the user page. (Obsoleting old patch which I managed to mangle!) (In reply to David Cook from comment #4) > opac/cgi-bin/opac/svc/auth/googleopenidconnect is not a valid Koha path in > the git. > > It also looks like this is based off a different version of > ./opac/svc/auth/googleopenidconnect than what exists in Koha's master branch. > > I think too the goal is to deprecate and remove > ./opac/svc/auth/googleopenidconnect in favour of the new generic OIDC > functionality. Thanks David. I didn't realise that. I've had a quick look at the new OIDC stuff... and it's a LOT less user friendly. I'll get around to having a look at it in the next week or so. Not sure why the googleopenidconnect file is different; it might be because it's based off the 23.11 branch. (I made the patch off a recently upgraded package install.) I can see about fixing that soon too. Created attachment 163493 [details] [review] Redirect to referring URL on successful OAuth2 Login This allows the new OAuth2/OIDC identity providers to redirect to the opac page the user was on when requesting a login. It ensures that the referrer is on the opac server, and strips out the logout parameters from the URI to prevent a login/logout loop. Created attachment 163494 [details] [review] Redirection to referring url for the googleopenidconnect service A secondary patch that enables redirecting to the referring page when using the old style googleopenidconnect service rather than the new identity providers. Created attachment 163495 [details] [review] Redirect to referring URL on successful OAuth2 Login This allows the new OAuth2/OIDC identity providers to redirect to the opac page the user was on when requesting a login. It ensures that the referrer is on the opac server, and strips out the logout parameters from the URI to prevent a login/logout loop. This patch removes some debugging code that should never have been included in the previous patch! Created attachment 163496 [details] [review] Redirect to referring URL on successful OAuth2 Login This allows the new OAuth2/OIDC identity providers to redirect to the opac page the user was on when requesting a login. It ensures that the referrer is on the opac server, and strips out the logout parameters from the URI to prevent a login/logout loop. This patch removes some debugging code that should never have been included in the previous patch! successfully tested the new OAuth2/OIDC identity providers! Please note that we were only able to test the second patch for oidc identity provider. We therefore ask for a second sign off for the old mimic for authentication via google oidc Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu> There's a few things here: 1. We do need the referrer for the staff interface too, since people might be using bookmarks, have expired sessions when clicking on an old search result, manually going to a URL written by hand, etc. 2. The session variable is different between the googleopenidconnect and the REST client. I think it'd be best to use the same meaningful name. (I also think referer as used in the REST client is too generic. That could cause problems down the line if we started tracking refer for other reasons. I think the one you used for googleopenidconnect could work (ie "auth-refer-uri"), so I think just harmonize those. 3. It would make more sense to validate the referrer input before setting it as a session variable. 4. Instead of "if (defined $referrer_url) && ($referrer_url ne '')", I think we could just do (if $referrer_url) in Perl. This syntax reminds me more of Python/Ruby. -- Overall, it's looking good, and I'm really looking forward to having this feature in Koha. I know this might be annoying, but it might be worth splitting this into 2 issues with 1 patch on each issue. Just because a lot of people will be able to test the REST client using Keycloak, but fewer people will be able to test the googleopenidconnect service. Comment on attachment 163496 [details] [review] Redirect to referring URL on successful OAuth2 Login Review of attachment 163496 [details] [review]: ----------------------------------------------------------------- ::: Koha/REST/V1/OAuth/Client.pm @@ +56,4 @@ > if ( $interface eq 'opac' ) { > + my $host = C4::Context->preference('OPACBaseURL'); > + $redirect_url = $host . '/api/v1/public/oauth/login/'; > + if ((defined $referrer_url) && ($referrer_url ne '') && ($referrer_url =~ /^$host/ )) { Since OPACBaseURL is a user input, I think it would be good to change the regexp to /^\Q$host\E/, so that we're escaping any metacharacters that might intentionally or unintentionally come through in that system preference. Created attachment 164979 [details] [review] [PATCH] Allow OAuth login to redirect to referring url on successful login Redirection to referring URL on successful OAuth2 Login. Updated following David's comments: 1. Enables the service for both the staff and the opac interfaces. 2. Uses "auth-refer-ui" as the session variable name. 3. Validates the referrer input before setting it as a session variable. 4. Uses perl syntax a bit better; perl isn't my preferred language, and it shows! Thanks David! 5. Only deals with OIDC/OAuth2, bug 36617 has been created to deal with the googleopenidconnect service. 6. Changes the regexp to /^\Q$host\E as a security measure. Created attachment 164980 [details] [review] [PATCH] Allow OAuth login to redirect to referring url on successful login Redirection to referring URL on successful OAuth2 Login. Updated following David's comments: 0. Removes some ugly debug statement that snuck into the patch! 1. Enables the service for both the staff and the opac interfaces. 2. Uses "auth-refer-ui" as the session variable name. 3. Validates the referrer input before setting it as a session variable. 4. Uses perl syntax a bit better; perl isn't my preferred language, and it shows! Thanks David! 5. Only deals with OIDC/OAuth2, bug 36617 has been created to deal with the googleopenidconnect service. 6. Changes the regexp to /^\Q$host\E as a security measure. Redirecting to the previous page after logging in generally works but unfortunately there are a few problems:
1. If there was an error in authentication or the user does not have permissions, the user will also be redirected without notice. The error message cannot be read because only on the page "/cgi-bin/koha/opac-user.pl" (in OPAC) errors are displayed.
2. After logging out (the "logout.x" parameter in the URL is present) the user cannot log back in.
3. After logging in from the OPAC main page, the user should be redirected to the account page ("/cgi-bin/koha/opac-user.pl").
|