Support nonce and acr with OIDC + Tests #883

fflorent · 2024-03-06T10:49:43Z

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 · 2024-03-19T09:03:30Z

app/server/lib/OIDCConfig.ts

+ post_logout_redirect_uri: redirectUrl.href,
+ state: mreq.session.oidc?.state,
+ id_token_hint: mreq.session.oidc?.idToken,
+ });


Note to the reviewers: I don't clear either the state or the idToken from the session in this method, as I feel like it's more secure to secure the logout if the first attempt fails (the following attempts may succeed).

This makes sense, but I can't see where this info is cleaned up. Can you point me to that?

fflorent · 2024-03-19T11:23:40Z

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

app/server/lib/OIDCConfig.ts

test/server/lib/OIDCConfig.ts

CamilleLegeron · 2024-03-27T10:43:32Z

Looks good to me

fflorent

Thanks @CamilleLegeron!

app/server/lib/OIDCConfig.ts

paulfitz · 2024-05-23T15:43:17Z

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.

SleepyLeslie

Thanks @fflorent for the work. I left some thoughts. I am new to these security features so I had to learn before commenting - sorry for the delay!
Also, feel free to point out any mistakes I make and any misunderstandings I have - I've just joined Grist Labs last week and I'm still largely unfamiliar with this codebase.

SleepyLeslie · 2024-05-23T18:31:40Z

app/client/ui/errorPages.ts

+ document.title = t("Signin failed{{suffix}}", {suffix: getPageTitleSuffix(getGristConfig())});
+ return pagePanelsError(appModel, t("Signin failed{{suffix}}", {suffix: ''}), [
+ cssErrorText(message ??
+ t("Failed to login.{{separator}}Please try again or contact the support.", {


"contact support" - no need for "the".

SleepyLeslie · 2024-05-23T18:35:47Z

app/server/lib/FlexServer.ts

@@ -1849,7 +1849,9 @@ export class FlexServer implements GristServer {
 }

 public resolveLoginSystem() {
- return process.env.GRIST_TEST_LOGIN ? getTestLoginSystem() : (this._getLoginSystem?.() || getLoginSystem());
+ return isAffirmative(process.env.GRIST_TEST_LOGIN) ?


Why not call allowTestLogin()?

SleepyLeslie · 2024-05-23T18:48:34Z

app/server/lib/OIDCConfig.ts

+ .omitBy(_.isFunction)
+ .mapValues((value, key) => {
+ const showValueInClear = ['token_type', 'expires_in', 'expires_at', 'scope'].includes(key);
+ return showValueInClear ? value : 'REDACTED';


Why not just show these 4 values?

SleepyLeslie · 2024-05-24T01:19:17Z

app/server/lib/OIDCConfig.ts

 res.redirect(targetUrl ?? '/');
 } catch (err) {
 log.error(`OIDC callback failed: ${err.stack}`);
+ if (Object.prototype.hasOwnProperty.call(err, 'response')) {
+ log.error(`Response received: ${err.response?.body ?? err.response}`);
+ }
 // Delete the session data even if the login failed.
 // This way, we prevent several login attempts.
 //
 // Also session deletion must be done before sending the response.
 delete mreq.session.oidc;


Comments here should be updated. It's no longer "even if the login failed" - if I understand correctly, OIDC-related session data now needs to be preserved until logout if login succeeds.
I don't quite understand the "we prevent several login attempts" part - can you explain that a bit more?

SleepyLeslie · 2024-05-24T01:20:29Z

app/server/lib/OIDCConfig.ts

 // Delete the session data even if the login failed.
 // This way, we prevent several login attempts.
 //
 // Also session deletion must be done before sending the response.
 delete mreq.session.oidc;
- res.status(500).send(`OIDC callback failed.`);
+ await this._sendAppPage(req, res, {


Nice UX improvement.

SleepyLeslie · 2024-05-24T15:06:54Z

test/server/lib/OIDCConfig.ts

+ {
+ itMsg: 'should fulfill when the end_session_endpoint is not known ' +
+ 'and GRIST_OIDC_IDP_SKIP_END_SESSION_ENDPOINT=true',
+ end_session_endpoint: undefined,
+ env: {
+ GRIST_OIDC_IDP_SKIP_END_SESSION_ENDPOINT: 'true'
+ }
+ },


Identical tests?

SleepyLeslie · 2024-05-24T15:20:42Z

test/server/lib/OIDCConfig.ts

+ const promise = OIDCConfigStubbed.buildWithStub(client.asClient());
+ if (ctx.errorMsg) {
+ await assert.isRejected(promise, ctx.errorMsg);
+ assert.isFalse(logInfoStub.calledOnce);


Is this test making sure log.info(`OIDCConfig: initialized with issuer ${issuerUrl}`) wasn't called?

SleepyLeslie · 2024-05-24T15:26:38Z

test/server/lib/OIDCConfig.ts

+ assert.isFalse(config.supportsProtection("STATE"));
+ });
+
+ it('if omitted, should defaults to "STATE,PKCE"', async function () {


Typo: remove "s" - "should default to"

SleepyLeslie · 2024-05-24T15:47:40Z

test/server/lib/OIDCConfig.ts

+ }
+ },
+ expectedErrorMsg: /Login is stale/,
+ },


Maybe it's helpful for long-term maintenance to explicitly specify GRIST_OIDC_IDP_ENABLED_PROTECTIONS here?

SleepyLeslie · 2024-05-24T17:51:10Z

test/server/lib/OIDCConfig.ts

+ });
+ });
+ });
+});


Very comprehensive testing. Thanks!

SleepyLeslie · 2024-05-24T18:49:43Z

* 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?

fflorent force-pushed the support-nonce-and-acr-with-oidc branch 3 times, most recently from e78e2f3 to 8b90c90 Compare March 7, 2024 10:59

fflorent commented Mar 19, 2024

View reviewed changes

fflorent marked this pull request as ready for review March 19, 2024 11:23

fflorent changed the title ~~Support nonce and acr with OIDC + Tests~~ [Draft] Support nonce and acr with OIDC + Tests Mar 19, 2024

fflorent force-pushed the support-nonce-and-acr-with-oidc branch 20 times, most recently from b3ca7fe to 56706b3 Compare March 21, 2024 06:49

fflorent added 3 commits March 21, 2024 07:49

First unit test for OIDCConfig

3f3066c

WIP

0eba178

WIP

6c95af3

fflorent added 5 commits March 21, 2024 07:49

Tests with keycloak are not in test:server

63f05e5

Fix CI ? + Skip in LoginWithOIDC

1dedcc1

Remove health-checks for keycloak, curl being not avaible in container

4cef071

Attempt fix using docker-run-action

a87578c

Remove integration tests (for now)

1bf114a

fflorent force-pushed the support-nonce-and-acr-with-oidc branch from 56706b3 to 1bf114a Compare March 21, 2024 06:49

fflorent changed the title ~~[Draft] Support nonce and acr with OIDC + Tests~~ Support nonce and acr with OIDC + Tests Mar 21, 2024

fflorent requested a review from CamilleLegeron March 21, 2024 18:24

CamilleLegeron reviewed Mar 26, 2024

View reviewed changes

app/server/lib/OIDCConfig.ts Show resolved Hide resolved

CamilleLegeron reviewed Mar 26, 2024

View reviewed changes

app/server/lib/OIDCConfig.ts Show resolved Hide resolved

CamilleLegeron reviewed Mar 26, 2024

View reviewed changes

app/server/lib/OIDCConfig.ts Show resolved Hide resolved

Add friendly page for login failure

0115474

fflorent force-pushed the support-nonce-and-acr-with-oidc branch from eb8331b to 0115474 Compare March 26, 2024 13:45

fflorent requested a review from CamilleLegeron March 26, 2024 14:25

CamilleLegeron reviewed Mar 27, 2024

View reviewed changes

test/server/lib/OIDCConfig.ts Outdated Show resolved Hide resolved

Fix bad indentation

280dc17

CamilleLegeron reviewed Mar 27, 2024

View reviewed changes

test/server/lib/OIDCConfig.ts Show resolved Hide resolved

PR remark

a992efa

fflorent force-pushed the support-nonce-and-acr-with-oidc branch from 763caf6 to a992efa Compare March 27, 2024 11:09

fflorent commented Mar 27, 2024

View reviewed changes

app/server/lib/OIDCConfig.ts Show resolved Hide resolved

fflorent requested a review from dsagal March 28, 2024 16:38

fflorent added the anct label Apr 11, 2024

paulfitz removed the request for review from dsagal May 15, 2024 20:23

fflorent requested a review from dsagal May 22, 2024 10:27

paulfitz requested review from SleepyLeslie and removed request for dsagal May 23, 2024 15:41

SleepyLeslie requested changes May 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support nonce and acr with OIDC + Tests #883

Support nonce and acr with OIDC + Tests #883

fflorent commented Mar 6, 2024 •

edited

fflorent Mar 19, 2024 •

edited

SleepyLeslie May 24, 2024

fflorent commented Mar 19, 2024

CamilleLegeron commented Mar 27, 2024 •

edited

fflorent left a comment

paulfitz commented May 23, 2024

SleepyLeslie left a comment

SleepyLeslie May 23, 2024

SleepyLeslie May 23, 2024

SleepyLeslie May 23, 2024

SleepyLeslie May 24, 2024

SleepyLeslie May 24, 2024

SleepyLeslie May 24, 2024

SleepyLeslie May 24, 2024

SleepyLeslie May 24, 2024

SleepyLeslie May 24, 2024

SleepyLeslie May 24, 2024

SleepyLeslie commented May 24, 2024 •

edited

Support nonce and acr with OIDC + Tests #883

Are you sure you want to change the base?

Support nonce and acr with OIDC + Tests #883

Conversation

fflorent commented Mar 6, 2024 • edited

Context

Proposed solution

fflorent Mar 19, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fflorent commented Mar 19, 2024

CamilleLegeron commented Mar 27, 2024 • edited

fflorent left a comment

Choose a reason for hiding this comment

paulfitz commented May 23, 2024

SleepyLeslie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SleepyLeslie commented May 24, 2024 • edited

fflorent commented Mar 6, 2024 •

edited

fflorent Mar 19, 2024 •

edited

CamilleLegeron commented Mar 27, 2024 •

edited

SleepyLeslie commented May 24, 2024 •

edited