Bug 34164 - OAuth2/OIDC should redirect to page that initiated login
Summary: OAuth2/OIDC should redirect to page that initiated login
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement with 11 votes (vote)
Assignee: Nicholas van Oudtshoorn
QA Contact: Testopia
URL:
Keywords:
: 24261 (view as bug list)
Depends on: 31378
Blocks:
  Show dependency treegraph
 
Reported: 2023-06-30 03:35 UTC by David Cook
Modified: 2024-04-18 13:06 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Return to the referring URL when signing in using GoogleOpenID (1.37 KB, patch)
2024-03-14 04:13 UTC, Nicholas van Oudtshoorn
Details | Diff | Splinter Review
Redirect to referring URL, excluding logout parameters (1.62 KB, patch)
2024-03-14 04:32 UTC, Nicholas van Oudtshoorn
Details | Diff | Splinter Review
Patch to redirect on login (2.08 KB, patch)
2024-03-14 04:38 UTC, Nicholas van Oudtshoorn
Details | Diff | Splinter Review
Redirect to referring URL on successful OAuth2 Login (2.49 KB, patch)
2024-03-20 05:52 UTC, Nicholas van Oudtshoorn
Details | Diff | Splinter Review
Redirection to referring url for the googleopenidconnect service (1.89 KB, patch)
2024-03-20 05:53 UTC, Nicholas van Oudtshoorn
Details | Diff | Splinter Review
Redirect to referring URL on successful OAuth2 Login (2.24 KB, patch)
2024-03-20 05:58 UTC, Nicholas van Oudtshoorn
Details | Diff | Splinter Review
Redirect to referring URL on successful OAuth2 Login (2.24 KB, patch)
2024-03-20 06:00 UTC, Nicholas van Oudtshoorn
Details | Diff | Splinter Review
[PATCH] Allow OAuth login to redirect to referring url on successful login (2.95 KB, patch)
2024-04-17 08:22 UTC, Nicholas van Oudtshoorn
Details | Diff | Splinter Review
[PATCH] Allow OAuth login to redirect to referring url on successful login (2.66 KB, patch)
2024-04-17 08:28 UTC, Nicholas van Oudtshoorn
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Cook 2023-06-30 03:35:23 UTC
At the moment, a successful OAuth2/OIDC login is sent to opac-user.pl/opac-main.pl or mainpage.pl instead of the page that initiated the login process.

With Shibboleth, we manage this using the "target" URL, so that the Shibboleth login is processed by whatever page initiated it, but that doesn't work with OAuth2/OIDC and the "redirect_uri" since we're using an API endpoint. 

However, all we need to do is save the current path in the user's session, and then recall it after the successful login.
Comment 1 David Cook 2023-06-30 03:52:03 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.
Comment 2 David Cook 2024-01-02 02:26:23 UTC
*** Bug 24261 has been marked as a duplicate of this bug. ***
Comment 3 Nicholas van Oudtshoorn 2024-03-14 04:13:08 UTC
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.
Comment 4 David Cook 2024-03-14 04:29:43 UTC
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.
Comment 5 Nicholas van Oudtshoorn 2024-03-14 04:32:07 UTC
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.
Comment 6 Nicholas van Oudtshoorn 2024-03-14 04:38:20 UTC
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!)
Comment 7 Nicholas van Oudtshoorn 2024-03-14 08:40:18 UTC
(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.
Comment 8 Nicholas van Oudtshoorn 2024-03-20 05:52:34 UTC
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.
Comment 9 Nicholas van Oudtshoorn 2024-03-20 05:53:46 UTC
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.
Comment 10 Nicholas van Oudtshoorn 2024-03-20 05:58:41 UTC
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!
Comment 11 Nicholas van Oudtshoorn 2024-03-20 06:00:19 UTC
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!
Comment 12 Michaela Sieber 2024-03-20 15:21:52 UTC
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>
Comment 13 David Cook 2024-04-09 05:37:45 UTC
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.
Comment 14 David Cook 2024-04-09 06:06:34 UTC
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 15 David Cook 2024-04-09 06:37:31 UTC
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.
Comment 16 Nicholas van Oudtshoorn 2024-04-17 08:22:16 UTC
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.
Comment 17 Nicholas van Oudtshoorn 2024-04-17 08:28:46 UTC
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.
Comment 18 Lukas Koszyk 2024-04-18 13:06:12 UTC
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").