Bug 32354 - Handle session_state param given by OAuth identity provider
Summary: Handle session_state param given by OAuth identity provider
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: Main
Hardware: All All
: P5 - low blocker (vote)
Assignee: Aleisha Amohia
QA Contact: Testopia
URL: https://openid.net/specs/openid-conne...
Keywords: rel_22_11_candidate
Depends on: 31378
Blocks:
  Show dependency treegraph
 
Reported: 2022-11-27 21:24 UTC by Aleisha Amohia
Modified: 2023-12-28 20:43 UTC (History)
5 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This patch ensures Koha doesn't throw an error if the IdP hands back a session_state parameter.
Version(s) released in:
23.05.00,22.11.01


Attachments
Bug 32354: Accept session_state param given by OAuth IdP (1.74 KB, patch)
2022-11-27 21:27 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 32354: Accept session_state param given by OAuth IdP (1.79 KB, patch)
2022-12-02 01:16 UTC, David Cook
Details | Diff | Splinter Review
Bug 32354: Accept session_state param given by OAuth IdP (1.84 KB, patch)
2022-12-07 17:19 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Aleisha Amohia 2022-11-27 21:24:09 UTC
We're trying Bug 31378 in production with OpenID Connect and Azure AD.

Azure is passing back a parameter session_state which causes Koha to throw a 'malformed query string' error

[{"message":"Malformed query string","path":"\/query\/session_state"}]

Believe this means we need to add session_state to the list of parameters Koha can expect from the IdP.
Comment 1 Aleisha Amohia 2022-11-27 21:27:53 UTC
Created attachment 144274 [details] [review]
Bug 32354: Accept session_state param given by OAuth IdP

This patch ensures Koha doesn't throw an error if the IdP hands back a
session_state parameter.

To test:

1) Set up an identity provider
2) On the IdP's configuration, make it hand back a session_state
3) Confirm authentication using OIDC works with and without the
session_state

Sponsored-by: Plant & Food Research
Comment 2 David Cook 2022-11-27 23:26:32 UTC
I noticed this during testing as well. I was able to turn off session management in Keycloak, but that might not be possible in all identity providers. 

I'll come back to this one.
Comment 3 Tomás Cohen Arazi 2022-11-29 00:24:45 UTC
We need to document it better. Understand if the param name is standard, if it needs to be carried around in responses as well, etc.
Comment 4 David Cook 2022-11-29 01:19:33 UTC
(In reply to Tomás Cohen Arazi from comment #3)
> We need to document it better. Understand if the param name is standard, if
> it needs to be carried around in responses as well, etc.

I'm not 100% sure what you mean, but I think I agree. I've got a deadline tomorrow, but I'm hoping to look at this in December. I'll be testing with Keycloak. 

That said, from what I've read so far at https://openid.net/specs/openid-connect-session-1_0.html, it looks like the session_state is just used by optional client side iframes for checking the user's session status with the IdP. 

So I think we can accept session_state in the IdP's AuthN response without actually supporting OIDC session management ourselves.
Comment 5 David Cook 2022-12-02 01:07:43 UTC
Currently testing this and using the following to point to the Keycloak provided by my customized docker-compose.sso.yml

http://host.docker.internal:8082/realms/test/.well-known/openid-configuration
Comment 6 David Cook 2022-12-02 01:10:50 UTC
(In reply to David Cook from comment #5)
> Currently testing this and using the following to point to the Keycloak
> provided by my customized docker-compose.sso.yml
> 
> http://host.docker.internal:8082/realms/test/.well-known/openid-configuration

Note that this is me using Docker Desktop on Windows, so your mileage may vary...
Comment 7 David Cook 2022-12-02 01:16:51 UTC
Created attachment 144388 [details] [review]
Bug 32354: Accept session_state param given by OAuth IdP

This patch ensures Koha doesn't throw an error if the IdP hands back a
session_state parameter.

To test:

1) Set up an identity provider
2) On the IdP's configuration, make it hand back a session_state
3) Confirm authentication using OIDC works with and without the
session_state

Sponsored-by: Plant & Food Research

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 8 Tomás Cohen Arazi 2022-12-02 01:18:30 UTC
(In reply to David Cook from comment #5)
> Currently testing this and using the following to point to the Keycloak
> provided by my customized docker-compose.sso.yml
> 
> http://host.docker.internal:8082/realms/test/.well-known/openid-configuration

The one in KTD, documented in the README should be usable as well.
Comment 9 David Cook 2022-12-02 01:22:57 UTC
I'm happy with this patch. If anyone needs help QAing this or wants to add their signoff and count mine towards QA, just let me know...

--

Out of scope side note:

It doesn't look like /oauth/login/{provider_code}/{interface} actually gets used. It looks like the staff interface uses the /public/oauth/login/{provider_code}/{interface} endpoint same as the OPAC. I don't know whether or not this is by design. Agustin and Tomas might be able to clarify that one...
Comment 10 David Cook 2022-12-02 01:25:30 UTC
(In reply to Tomás Cohen Arazi from comment #8)
> The one in KTD, documented in the README should be usable as well.

That's true. I don't like meddling with my /etc/hosts too much but maybe I should just for the sake of consistency with other KTD users...
Comment 11 David Cook 2022-12-02 01:29:57 UTC
(In reply to Tomás Cohen Arazi from comment #8)
> The one in KTD, documented in the README should be usable as well.

Hmmm that doesn't seem to work, since on the workstation it's sso:8082 but in koha-testing-docker it's sso:8080
Comment 12 David Cook 2022-12-02 01:31:02 UTC
(In reply to David Cook from comment #11)
> Hmmm that doesn't seem to work, since on the workstation it's sso:8082 but
> in koha-testing-docker it's sso:8080

Ah, I'm using an old docker-compose.sso.yml. Nevermind me ;)
Comment 13 David Cook 2022-12-02 02:00:34 UTC
Pulled the latest koha-testing-docker and the http://sso:8082/auth/ URL works well, so a "test" realm would be http://sso:8082/auth/realms/test/.well-known/openid-configuration
Comment 14 Tomás Cohen Arazi 2022-12-07 17:19:44 UTC
Created attachment 144477 [details] [review]
Bug 32354: Accept session_state param given by OAuth IdP

This patch ensures Koha doesn't throw an error if the IdP hands back a
session_state parameter.

To test:

1) Set up an identity provider
2) On the IdP's configuration, make it hand back a session_state
3) Confirm authentication using OIDC works with and without the
session_state

Sponsored-by: Plant & Food Research

Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 15 Tomás Cohen Arazi 2022-12-07 17:30:59 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 16 Jacob O'Mara 2022-12-08 11:11:40 UTC
Nice work, thanks everyone!

Pushed to 22.11.x for the next release.
Comment 17 Lucas Gass 2023-01-18 21:25:24 UTC
Not needed in 22.05.x