grist-core icon indicating copy to clipboard operation
grist-core copied to clipboard

Support nonce and acr with OIDC + Tests

Open fflorent opened this issue 1 year ago • 9 comments

Context

  • Some Identity provider don't support PKCE and instead impose Nonce;
  • They also may impose passing some ACR values;
  • And require to pass the state and the idToken in the logout

Proposed solution

  • Introduce the GRIST_OIDC_IDP_ENABLED_PROTECTIONS variable who can contain comma-separated values with either: STATE, NONCE and PKCE, and defaults to STATE,PKCE;
  • Introduce the GRIST_OIDC_IDP_ACR_VALUES variable with space separated values;
  • Once logged in (after the callback), store the state and the idToken for the logout, and clear any other values;
  • Introduce unit tests with mocks;
  • Also redirect to the error page when something went wrong while signing in;

fflorent avatar Mar 06 '24 10:03 fflorent

Opening the PR for checking whether the tests work in the CI

fflorent avatar Mar 19 '24 11:03 fflorent

Looks good to me

CamilleLegeron avatar Mar 27 '24 10:03 CamilleLegeron

Hi @fflorent, sorry for the delay in reviewing this! Dmitry just hasn't had time, and likely won't for a while. @SleepyLeslie has offered to take it on.

paulfitz avatar May 23 '24 15:05 paulfitz

* Some Identity provider don't support PKCE and instead impose Nonce;
* They also may impose passing some ACR values;
* And require to pass the state and the idToken in the logout

By the way, can you provide some examples for these IdPs?

SleepyLeslie avatar May 24 '24 18:05 SleepyLeslie

@SleepyLeslie

By the way, can you provide some examples for these IdPs?

I just have one example of such IdP, AgentConnect, which requires all of that and is used by the French Administration.

Otherwise, my point was that OIDC allows to ask the IdP to meet these requirements.

Do you ask this question for testing purpose?

fflorent avatar Jun 04 '24 12:06 fflorent

I just have one example of such IdP, AgentConnect, which requires all of that and is used by the French Administration. Otherwise, my point was that OIDC allows to ask the IdP to meet these requirements. Do you ask this question for testing purpose?

@fflorent Yeah it would be helpful for me if I can fiddle around, see it in action and do some tests. I assume that's not feasible with AgentConnect but it's totally okay. Compatibility improvements are always welcome!

SleepyLeslie avatar Jun 04 '24 16:06 SleepyLeslie

@SleepyLeslie I sent you in private everything you need to try my PR using Agent Connect. Please keep me informed otherwise.

fflorent avatar Jun 05 '24 16:06 fflorent

@SleepyLeslie Hmm, I have given the env variables to use, but using your own email won't work. There are credentials to use and some simple questions to answer, but asked in French.

I suggest we take some time to take a look at that together if you want to, that will be probably simpler and faster. It may take 10 / 15 minutes in case testing is just what you need, or more if you have questions regarding my developments.

I send you an email to see what slot we could find.

fflorent avatar Jun 12 '24 07:06 fflorent

@dsagal Thanks a lot for your thorough review! I'll take a look next week (working for another client than the French administration today)

fflorent avatar Jun 14 '24 13:06 fflorent

Didn't have time to review all, but sharing what I have so far.

dsagal avatar Jul 12 '24 06:07 dsagal

@dsagal Thanks for your remarks! I should have taken them into account

fflorent avatar Jul 18 '24 11:07 fflorent

I took into account your remarks @Spoffy, thank you!

fflorent avatar Aug 08 '24 11:08 fflorent