[api] add Microsoft strategy to auth module (single sign-on) #3453

freemvmt · 2024-07-25T09:14:20Z

Summary

This PR adds the ability to sign into PlanX using a Microsoft account (personal or organisational), which most users (working in local government) are more likely to have to hand as compared to a Google account.

We make use of the existing passportjs implementation, adding a new microsoft-oidc strategy which leverages openid-client, which we consider reliable thanks to certification by OpenID themselves.

Since this package has some asynchronous behaviour for which the best code pattern involved using top-level awaits, this project induced some bonuses in api.planx.uk, namely adopting ESM throughout (#3464), which in turn required a migration to Vitest (#3555). We also took the opportunity to bump passportjs to latest (#3502).

See also #3221, which I used as a development/demo pizza, and has a much more granular commit structure.

TODO

Resolve various TODOs and polish
Resolve issue with Jest (see below)
Add front channel logout to OIDC setup
Further security hardening (e.g. validating ID tokens)
Add unit and integration tests
Document changes in README and as a new ADR (including Azure/Entra setup)

We will address anything not included here in further PRs.

Jest/ESM issue ✅

The stubborn issue here is that Jest (or ts-jest, the transformer we're using to transpile .ts files into .js files that Jest can read) will not treat files as ES modules (despite following docs to the letter), and hence throws errors like the following:

 FAIL  ./server.test.js
  ● Test suite failed to run

    server.ts:126:18 - error TS1378: Top-level 'await' expressions are only allowed when the 'module' option is set to 'es2022', 'esnext', 'system', 'node16', 'nodenext', or 'preserve', and the 'target' option is set to 'es2017' or higher.

    126 const passport = await getPassport();
                         ~~~~~

I examine some of the background for this in #3528, ~~onto which this PR is chained~~.

Below is a non-exhaustive list of possible approaches to this problem, which I've marked complete if they've been fully explored and not provided a solution:

Worth noting that even Jest advises that ESM support is experimental, and indeed their issue tracking this is very much still open. This could suggest we should lean to some of the latter, more climb down-y options.

🏁 In the end we abandoned Jest in favour of Vitest, which is much more ESM-friendly! See #3555 :)

api.planx.uk/modules/auth/routes.ts

api.planx.uk/server.ts

github-actions · 2024-07-25T09:38:50Z

Removed vultr server and associated DNS entries

DafyddLlyr

Thanks for bearing with me whilst I got through this. Will give it another pass through once top level await takes away a lot of the changes here 👍

Code looking great - easy to follow and clear!

api.planx.uk/modules/auth/passport.ts

api.planx.uk/modules/auth/strategy/microsoft-oidc.ts

editor.planx.uk/src/pages/Login.tsx

freemvmt · 2024-08-16T21:00:03Z

Rebased this PR onto #3528 - full explanation given in that PR description.

freemvmt · 2024-09-05T16:20:13Z

Rebased PR onto #main now that it includes #3555, which makes #3528 redundant for the purpose of fixing the Jest issue (but potentially valuable in and of itself, see comment).

clean up code: add comments, remove logs, improve naming etc add exemplar Microsoft auth environment variables add Microsoft URLs to CORS allow list

freemvmt

Some notes

freemvmt · 2024-09-06T17:12:39Z

api.planx.uk/modules/auth/passport.ts

+  const customPassport = new passport.Passport();
+
+  // instantiate Microsoft OIDC client, and use it to build the related strategy
+  const microsoftIssuer = await Issuer.discover(MICROSOFT_OPENID_CONFIG_URL);


Fetches config from Microsoft's .well-known address

freemvmt · 2024-09-06T17:13:49Z

api.planx.uk/modules/auth/routes.ts

 import * as Middleware from "./middleware.js";
 import * as Controller from "./controller.js";

-const router = Router();
+export default (passport: Authenticator): Router => {


We have to pass the passport instance through to the middleware for these routes, since they rely on it already being instantiated, which in turn is a blocking process, so instead export a factory function as default.

freemvmt · 2024-09-06T17:15:26Z

api.planx.uk/modules/auth/strategy/microsoft-oidc.ts

+  return {
+    client_id,
+    client_secret: process.env.MICROSOFT_CLIENT_SECRET!,
+    redirect_uris: [`${process.env.API_URL_EXT}/auth/microsoft/callback`],


this has to match one of the redirect URI configured in the PlanX Azure application

i.e. https://api.editor.planx.dev/auth/microsoft/callback for staging
and https://api.editor.planx.uk/auth/microsoft/callback for prod

freemvmt · 2024-09-06T18:28:19Z

api.planx.uk/server.ts

-  cb(null, obj);
-});
+// equip passport with auth strategies early on, so we can pass it to route handlers
+const passport = await getPassport();


the fabled top-level await!

freemvmt

And a query requiring some typescript tekkers

api.planx.uk/modules/auth/strategy/microsoft-oidc.ts

api.planx.uk/server.ts

DafyddLlyr · 2024-09-09T10:31:43Z

Merging to main - will pick up Pulumi vars in a follow up PR shortly, and outstanding items from the checklist in description can follow along also.

freemvmt mentioned this pull request Jul 25, 2024

[DEMO][api] add microsoft strategy to auth module #3221

Closed

github-advanced-security bot found potential problems Jul 25, 2024

View reviewed changes

freemvmt mentioned this pull request Jul 27, 2024

[api] adopt ESM throughout #3464

Merged

3 tasks

DafyddLlyr reviewed Aug 8, 2024

View reviewed changes

freemvmt force-pushed the api-add-microsoft-sso-strategy-2 branch 3 times, most recently from 8ffa211 to ac7a60e Compare August 15, 2024 19:10

freemvmt mentioned this pull request Aug 16, 2024

[api] implement verbatim module syntax in tsconfig #3528

Merged

freemvmt force-pushed the api-add-microsoft-sso-strategy-2 branch from ac7a60e to d9f76ad Compare August 16, 2024 20:58

freemvmt changed the base branch from main to implement-verbatim-module-syntax August 16, 2024 20:59

freemvmt requested a review from a team August 16, 2024 21:40

freemvmt changed the base branch from implement-verbatim-module-syntax to main September 5, 2024 15:04

freemvmt force-pushed the api-add-microsoft-sso-strategy-2 branch from 380c91c to 64b37d2 Compare September 5, 2024 15:28

freemvmt force-pushed the api-add-microsoft-sso-strategy-2 branch from d67e62f to 92013f2 Compare September 5, 2024 19:58

freemvmt added 3 commits September 6, 2024 17:35

[api] add microsoft strategy to auth module

0b836c4

[api] modularise passport code, harden security

ba98e81

clean up code: add comments, remove logs, improve naming etc add exemplar Microsoft auth environment variables add Microsoft URLs to CORS allow list

[api] introduce top-level await for getting passport in server.ts

1a89b48

freemvmt force-pushed the api-add-microsoft-sso-strategy-2 branch from 630caed to 9f08497 Compare September 6, 2024 16:38

freemvmt changed the title ~~[WIP][api] add microsoft strategy to auth module~~ [api] add microsoft strategy to auth module Sep 6, 2024

[api] address PR comments, apply minor fixes, polish

a918020

freemvmt force-pushed the api-add-microsoft-sso-strategy-2 branch from 9f08497 to a918020 Compare September 6, 2024 17:04

freemvmt commented Sep 6, 2024

View reviewed changes

[api] extract verify callback for more explicit typing

e599a44

freemvmt commented Sep 6, 2024

View reviewed changes

api.planx.uk/modules/auth/strategy/microsoft-oidc.ts Show resolved Hide resolved

api.planx.uk/server.ts Show resolved Hide resolved

freemvmt changed the title ~~[api] add microsoft strategy to auth module~~ [api] add Microsoft strategy to auth module (single sign-on) Sep 6, 2024

freemvmt requested review from DafyddLlyr and jessicamcinchak September 6, 2024 18:44

DafyddLlyr mentioned this pull request Sep 9, 2024

Production deploy #3641

Merged

DafyddLlyr approved these changes Sep 9, 2024

View reviewed changes

DafyddLlyr merged commit 2e4edc6 into main Sep 9, 2024
12 checks passed

DafyddLlyr deleted the api-add-microsoft-sso-strategy-2 branch September 9, 2024 10:32

This was referenced Sep 9, 2024

chore(iac): Add Microsoft secrets to Pulumi #3642

Merged

chore: Update CORS_ALLOWLIST on staging and production #3645

Merged

freemvmt mentioned this pull request Oct 3, 2024

[api] remove /logout route (do not implement front channel logout) #3759

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[api] add Microsoft strategy to auth module (single sign-on) #3453

[api] add Microsoft strategy to auth module (single sign-on) #3453

freemvmt commented Jul 25, 2024 •

edited

Loading

github-actions bot commented Jul 25, 2024 •

edited

Loading

DafyddLlyr left a comment

freemvmt commented Aug 16, 2024

freemvmt commented Sep 5, 2024 •

edited

Loading

freemvmt left a comment

freemvmt Sep 6, 2024

freemvmt Sep 6, 2024

freemvmt Sep 6, 2024

freemvmt Sep 6, 2024

freemvmt left a comment

DafyddLlyr commented Sep 9, 2024

[api] add Microsoft strategy to auth module (single sign-on) #3453

[api] add Microsoft strategy to auth module (single sign-on) #3453

Conversation

freemvmt commented Jul 25, 2024 • edited Loading

Summary

TODO

Jest/ESM issue ✅

github-actions bot commented Jul 25, 2024 • edited Loading

DafyddLlyr left a comment

Choose a reason for hiding this comment

freemvmt commented Aug 16, 2024

freemvmt commented Sep 5, 2024 • edited Loading

freemvmt left a comment

Choose a reason for hiding this comment

freemvmt Sep 6, 2024

Choose a reason for hiding this comment

freemvmt Sep 6, 2024

Choose a reason for hiding this comment

freemvmt Sep 6, 2024

Choose a reason for hiding this comment

freemvmt Sep 6, 2024

Choose a reason for hiding this comment

freemvmt left a comment

Choose a reason for hiding this comment

DafyddLlyr commented Sep 9, 2024

freemvmt commented Jul 25, 2024 •

edited

Loading

github-actions bot commented Jul 25, 2024 •

edited

Loading

freemvmt commented Sep 5, 2024 •

edited

Loading