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;

app/server/lib/OIDCConfig.ts

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

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

fflorent · 2024-06-05T16:15:48Z

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

SleepyLeslie

Thanks @fflorent for the updates and Agent Connect info. I'm waiting for an approval to sign up for Agent Connect. I'll get back to you as soon as I finish testing. Otherwise this looks good to me now, and should be able to get merged soon!

fflorent · 2024-06-12T07:47:36Z

@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.

SleepyLeslie

@fflorent Thanks for working with me on testing. I ran your PR successfully against Agent Connect locally. Found a small issue when running with my Authentik server and left a comment. Otherwise this PR looks good to me.

Please disregard my previous comment (deleted). I messed up. Everything seems in good shape now.

SleepyLeslie · 2024-06-13T20:39:41Z

app/server/lib/OIDCConfig.ts


      // The callback function will compare the state present in the params and the one we retrieved from the session.
      // If they don't match, it will throw an error.
-      const tokenSet = await this._client.callback(
-        this._redirectUrl,
-        params,
-        { state, code_verifier: codeVerifier }
-      );
+      const tokenSet = await this._client.callback(this._redirectUrl, params, checks);


During my test against my own Authentik server, excluding STATE from GRIST_OIDC_IDP_ENABLED_PROTECTIONS caused login to fail. This was because Authentik sent an empty state while Grist expected it to not exist at all. Could you handle this and similar situations? I could imagine stripping empty fields from params before calling this._client.callback.

Thanks for your feedback! I'll take a look at this next week (I am not working for the ANCT today).

The last time I took a look at Authentik, I failed to setup an instance (at least, it seemed to me it's not that easy). I'll take a look and see if a free account on their SAAS offer would be enough.

dsagal

This is great! Adds a lot of features to OIDC support, and it's great to see a comprehensive test! Sorry for having lots of comments -- but most are superficial. Overall it looks very solid, thank you!

dsagal · 2024-06-14T07:28:21Z

app/server/lib/OIDCConfig.ts

+      client_secret: clientSecret,
+      redirect_uris: [this._redirectUrl],
+      response_types: ['code'],
+      ...extraMetadata,


This allows GRIST_OIDC_IDP_EXTRA_CLIENT_METADATA to override the values set earlier. Is that intentional? If so, maybe that's worth mentioning in the description of that variable?

dsagal · 2024-06-14T07:37:39Z

app/server/lib/OIDCConfig.ts


 const CALLBACK_URL = '/oauth2/callback';

+function formatTokenForLogs(token: TokenSet) {
+  return _.chain(token)


Total nitpick, but after being a heavy lodash user myself, I've since found that it makes things a bit harder to understand (e.g. I'd have to refer to docs often), and native JS is more readable and almost as concise. E.g. I think this would be equivalent:

const showValueInClear = ['token_type', 'expires_in', 'expires_at', 'scope']; const result = {}; for (const [key, value] of Object.entries(token)) { if (typeof value !== 'function') { result[key] = showValueInClear.includes(key) ? value : 'REDACTED'; } } return result;

dsagal · 2024-06-14T07:45:24Z

app/server/lib/OIDCConfig.ts

+import { SessionObj } from './BrowserSession';
+import { SendAppPage } from './sendAppPage';
+
+enum ENABLED_PROTECTIONS {


We have a handy StringUnion class in app/common/StringUnion, which is great for things like this. More convenient than an enum, I think. It does typings, and has a method checkAll to check a list of values easily.

dsagal · 2024-06-14T07:48:16Z

app/server/lib/OIDCConfig.ts

+  "Something went wrong while logging, please try again or contact your administrator if the problem persists";
+
+class ErrorWithUserFriendlyMessage extends Error {
+  constructor(errMessage: string, public readonly userFriendlyMessage: string = DEFAULT_USER_FRIENDLY_MESSAGE) {


I am confused by the purpose of this default message. When would you want to omit the second argument?

dsagal · 2024-06-14T07:52:41Z

app/server/lib/OIDCConfig.ts

-    const issuer = await Issuer.discover(issuerUrl);
+    const extraMetadata: Partial<ClientMetadata> = JSON.parse(section.flag('extraClientMetadata').readString({
+      envVar: 'GRIST_OIDC_IDP_EXTRA_CLIENT_METADATA',
+      defaultValue: '{}'


If I recall correctly, the default won't come into play if the variable is set but is an empty string. I think it's easy to handle that case with JSON.parse(section.flag(...).readString(....) || "{}")?

dsagal · 2024-06-14T08:39:15Z

test/nbrowser/testServer.ts

-      env,
+      env: {
+        ...env,
+        ...(process.env.SERVER_NODE_OPTIONS ? {NODE_OPTIONS: process.env.SERVER_NODE_OPTIONS} : {})


I see SERVER_NODE_OPTIONS here and NODE_SERVER_OPTIONS in develop.md documentation.

dsagal · 2024-06-14T08:44:19Z

app/server/lib/BrowserSession.ts

@@ -74,6 +74,8 @@ export interface SessionObj {
    codeVerifier?: string;
    state?: string;
    targetUrl?: string;
+    nonce?: string;


Could you add a brief explanation of the oidc state (currently only codeVerifier is explained). E.g. I know, after reading the code, that some of state, codeVerifier, and nonce (depending on GRIST_OIDC_IDP_ENABLED_PROTECTIONS) will be set as protections during OIDC authentication, and it would be a helpful reminder to anyone else reading this code.

dsagal · 2024-06-14T12:45:54Z

app/server/lib/OIDCConfig.ts

      targetUrl
    };
+    if (this.supportsProtection('PKCE')) {


I have no problem with this code, but would just like to share an idea. Each protection implements the same interface, so you could organize the code so that the configuration is translated to an array of checks that you can then scan through without re-checking the configuration each time. Something like this:

for (const protection of enabledProtections) { this._protections.push(Protections[protection]) } const Protections: {[key: EnabledProtection]: ...} = { NONCE: { setProtection(sessionInfo, urlParams) { sessionInfo.nonce = urlParams.nonce = generator.nonce(); }, checkProtection(sessionInfo, checks) { if (!sessionInfo.nonce) { throw new Error("Login is stale"); } checks.nonce = sessionInfo.nonce; } }, ... }

dsagal · 2024-06-14T13:02:43Z

test/server/lib/OIDCConfig.ts

+          end_session_endpoint: undefined,
+        }
+      ].forEach((ctx) => {
+        it(ctx.itMsg, async () => {


My recommendation for repetitive tests is to factor out the common part into a function, but keep tests as individual manually written blocks. E.g.

it('should reject when ...', async () => { await assert.isRejected(doEndSessionTest({end_session_endpoint: undefined}), /...please set.../) });

It's not too important (so ok not to refactor if you prefer to keep as is), but my reason for suggesting this is past experience when a bug crept into some logic that generated test cases, and then no one noticed for a while that some tests weren't running at all.

dsagal · 2024-06-14T13:06:42Z

test/server/lib/OIDCConfig.ts

+
+    it('should successfully accept an empty string', async function () {
+      setEnvVars();
+      process.env.GRIST_OIDC_IDP_ENABLED_PROTECTIONS = '';


This is the case that worries me. Do we not want this to default to the recommended options in this case? Maybe if we need to support "no checks" mode, we should require the user to set this variable to "UNPROTECTED", so they can't miss the danger?

fflorent · 2024-06-14T13:27:49Z

@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 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

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

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 21 times, most recently 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

fflorent added 20 commits June 5, 2024 12:41

WIP: LoginWithOIDC

f326aff

Log err.response if present

d95cfae

Add support for extra OIDC client metadata

6425b7a

Log userinfo in debug and check passed info in endSessionUrl

778493c

Many other stuffs + cleanup + working version with Agent Connect

4ce7eef

WIP: integration tests

a2ea3bd

Improve formatTokenForLogs + format file

cac4176

Selenium tests working

17cf5c3

Fix CI?

8494ec7

Tests with keycloak are not in test:server

2ab6ac2

Fix CI ? + Skip in LoginWithOIDC

7068a31

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

28d0297

Attempt fix using docker-run-action

b5dd211

Remove integration tests (for now)

a6709cd

Add friendly page for login failure

4e9eef9

Fix bad indentation

498d937

PR remark

0121132

Fix after rebase

dcf376f

PR remarks

f89a96b

Adapt comment

d83df2e

fflorent force-pushed the support-nonce-and-acr-with-oidc branch from bb81a2a to d83df2e Compare June 5, 2024 12:50

Smarter way to test whether an info log has been called

660ce77

fflorent requested a review from SleepyLeslie June 5, 2024 16:16

SleepyLeslie reviewed Jun 10, 2024

View reviewed changes

SleepyLeslie requested a review from dsagal June 12, 2024 18:01

SleepyLeslie reviewed Jun 13, 2024

View reviewed changes

dsagal reviewed Jun 14, 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 commented Mar 19, 2024

fflorent commented Jun 5, 2024

SleepyLeslie left a comment

fflorent commented Jun 12, 2024

SleepyLeslie left a comment

SleepyLeslie Jun 13, 2024

fflorent Jun 14, 2024

dsagal left a comment

dsagal Jun 14, 2024

dsagal Jun 14, 2024

dsagal Jun 14, 2024

dsagal Jun 14, 2024

dsagal Jun 14, 2024

dsagal Jun 14, 2024

dsagal Jun 14, 2024

dsagal Jun 14, 2024

dsagal Jun 14, 2024

dsagal Jun 14, 2024

fflorent commented Jun 14, 2024

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 commented Mar 19, 2024

fflorent commented Jun 5, 2024

SleepyLeslie left a comment

Choose a reason for hiding this comment

fflorent commented Jun 12, 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

dsagal 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

fflorent commented Jun 14, 2024

fflorent commented Mar 6, 2024 •

edited