Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support nonce and acr with OIDC + Tests #883

Merged
merged 45 commits into from
Aug 8, 2024
Merged

Support nonce and acr with OIDC + Tests #883

merged 45 commits into from
Aug 8, 2024

Conversation

fflorent
Copy link
Collaborator

@fflorent fflorent commented Mar 6, 2024 •
edited
Loading

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 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
Copy link
Collaborator Author

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

@fflorent fflorent marked this pull request as ready for review March 19, 2024 11:23
@fflorent fflorent changed the title Support nonce and acr with OIDC + Tests [Draft] Support nonce and acr with OIDC + Tests Mar 19, 2024
@fflorent 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 fflorent changed the title [Draft] Support nonce and acr with OIDC + Tests Support nonce and acr with OIDC + Tests Mar 21, 2024
@fflorent fflorent requested a review from CamilleLegeron March 21, 2024 18:24
@fflorent fflorent force-pushed the support-nonce-and-acr-with-oidc branch from 3603767 to bb6445b Compare July 25, 2024 07:25
fflorent
fflorent commented Jul 25, 2024
View reviewed changes
Copy link
Collaborator Author

@fflorent fflorent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @dsagal for your review! 🙏

I made the changes requested.

app/server/lib/OIDCConfig.ts
@@ -167,33 +224,43 @@ export class OIDCConfig {
profile,
}));

delete mreq.session.oidc;
mreq.session.oidc = {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is what I wrote good to you?

app/server/lib/OIDCConfig.ts Outdated
res.redirect(targetUrl ?? '/');
} catch (err) {
log.error(`OIDC callback failed: ${err.stack}`);
// Delete the session data even if the login failed.
if (Object.prototype.hasOwnProperty.call(err, 'response')) {
log.error(`Response received: ${err.response?.body ?? err.response}`);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realized that the error may be multiple things (especially the body may be a buffer).

I made some changes, with the hope it will be fine for you. If that's too tricky or you feel a bit uncomfortable, I may remove this log and reintroduce it a bit later or in a separate PR.

@fflorent fflorent force-pushed the support-nonce-and-acr-with-oidc branch from 561dc45 to 15732bc Compare July 25, 2024 12:39
@fflorent fflorent requested a review from dsagal July 25, 2024 13:10
dsagal
dsagal approved these changes Jul 29, 2024
View reviewed changes
Copy link
Member

@dsagal dsagal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SleepyLeslie, would you be able to run your tests against Authentik again, since quite a lot of code changed since your review? All code looks good to me now, so approving from my end.

app/server/lib/BrowserSession.ts Outdated Show resolved Hide resolved
Spoffy
Spoffy approved these changes Aug 7, 2024
View reviewed changes
Copy link
Contributor

@Spoffy Spoffy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing this against our OIDC example that uses Authelia, and State, Nonce and PKCE all seem to be working as expected! 🎉

I've left a few minor nitpick comments, I don't think any of them are necessary from my perspective 🙂

app/common/StringUnion.ts Outdated Show resolved Hide resolved
app/server/lib/OIDCConfig.ts Outdated Show resolved Hide resolved
app/server/lib/sendAppPage.ts Outdated Show resolved Hide resolved
app/server/lib/OIDCConfig.ts Outdated Show resolved Hide resolved
app/server/lib/OIDCConfig.ts Outdated Show resolved Hide resolved
@fflorent
Copy link
Collaborator Author

fflorent commented Aug 8, 2024

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

dsagal
dsagal approved these changes Aug 8, 2024
View reviewed changes
Copy link
Member

@dsagal dsagal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the bits of cleanup!

app/server/lib/OIDCConfig.ts Outdated Show resolved Hide resolved
@fflorent fflorent mentioned this pull request Aug 8, 2024
Closed
@dsagal dsagal dismissed SleepyLeslie’s stale review August 8, 2024 19:26

All the changes requested have been resolved, or are no longer relevant, and latest version has been reviewed.

@dsagal dsagal merged commit fde6c81 into gristlabs:main Aug 8, 2024
9 of 11 checks passed
@fflorent fflorent deleted the support-nonce-and-acr-with-oidc branch August 14, 2024 15:23
fflorent added a commit to fflorent/grist-help that referenced this pull request Aug 16, 2024
@fflorent
Document new OIDC variables after gristlabs/grist-core#883
1d1d8b0 
New variables have been introduced in this PR:
  - GRIST_OIDC_IDP_ENABLED_PROTECTIONS
  - GRIST_OIDC_IDP_ACR_VALUES
  - GRIST_OIDC_IDP_EXTRA_CLIENT_METADATA
dsagal pushed a commit to gristlabs/grist-help that referenced this pull request Aug 16, 2024
@fflorent
Document new OIDC variables after gristlabs/grist-core#883 (#387)
6fb6e4d 
New variables have been introduced in this PR:
  - GRIST_OIDC_IDP_ENABLED_PROTECTIONS
  - GRIST_OIDC_IDP_ACR_VALUES
  - GRIST_OIDC_IDP_EXTRA_CLIENT_METADATA
@fflorent fflorent mentioned this pull request Aug 24, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CamilleLegeron CamilleLegeron CamilleLegeron left review comments

@dsagal dsagal dsagal approved these changes

@Spoffy Spoffy Spoffy approved these changes

@SleepyLeslie SleepyLeslie Awaiting requested review from SleepyLeslie

Assignees
No one assigned
Labels
anct
Projects
French administration Board
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@fflorent @CamilleLegeron @paulfitz @SleepyLeslie @dsagal @Spoffy