Move JWT from cookie store to Authorization header #2158

radavis · 2020-04-06T14:35:13Z

Resolves #2100

This pull request...

removes JWT from cookies
stores JWT on front end in localStorage
sends JWT in Authorization header with each request

This pull request is ready to merge when...

Tests have been updated (and all tests are passing)
This code has been reviewed by someone other than the original author
~~The experience passes a basic manual accessibility audit (keyboard nav, screenreader, text scaling) OR an exemption is documented~~ no UI changes
The change has been documented
- Associated OpenAPI documentation has been updated
Changelog is updated as appropriate

This feature is done when...

Design has approved the experience
Product has approved the experience

radavis · 2020-04-06T14:36:54Z

api/auth/session.js

-              expiresIn: `${process.env.SESSION_LIFETIME_MINUTES}m`
-            }
-          ),
+          signWebToken({ payload: session.content }),


Extracted common functionality in this file into functions (signWebToken, verifyWebToken) within api/auth/jwtUtils.js

docker-compose.yml

cms-eapd-bot · 2020-04-07T16:01:37Z

This deploy was cleaned up.

api/auth/index.js

radavis · 2020-05-01T20:42:58Z

Sorry this is such a massive change! 😬

api/routes/auth/login.endpoint.js

docker-compose.yml

mgwalker

Only a few minor changes. I prefixed those comments with:

change

to make them easier to find. There's also one that's ripe for discussion about whether /auth/logout should ever return error statuses. On the whole, though, this PR looks really good. It's a nice, clean implementation!

mgwalker · 2020-05-04T16:45:32Z

api/auth/index.js


-const defaultStrategies = [new LocalStrategy(authenticate())];
+const passportLocalStrategy = new LocalStrategy(authenticate());


This is a sensible change. The reason it was originally an array is we thought we would soon need to support more than one strategy - local for dev, and CMS's central auth service in production. That will eventually still be the case, but I like this being simpler in the meantime. 👍

api/auth/index.js

mgwalker · 2020-05-04T18:22:53Z

api/auth/index.js

+    '/auth/login',
+    passport.authenticate('local', { session: false }),
+    (req, res) => {
+      serializeUser(req.user, (err, sessionId) => {


Since serializeUser is only used by our custom code now instead of being used by the Passport session library, it doesn't have to conform to that API anymore. We should look into how we can refactor it to be easier to use/maintain. (Added issue #2206 to track that as future work.)

Thanks for documenting that!

mgwalker · 2020-05-04T18:26:14Z

api/auth/index.js

+  // login routes.
+  const jwtExcludedRoutes = ['/auth/login/nonce', '/auth/login'];
+  app.use((req, res, next) =>
+    jwtExcludedRoutes.includes(req.originalUrl)


🤔 I wonder how this is affected by proxying. It's probably fine, but we should double-check in a preview deployment because those create a proxy from /api to /. If originalUrl is /api/auth/login/nonce, that could break this. But again... I bet it's fine. I think Express generally ignores proxies.

So, I was thinking about this yesterday evening. If I rely on the jwtMiddleware to only deserialize the user if the Authorization header is present (e.g. remove the res.send(400).end()), and rely on can and loggedIn to manage sending 401 and 403 responses, I can get rid of this funky excluded routes thing.

mgwalker · 2020-05-04T18:34:21Z

api/auth/jwtUtils.js

+  jwtExtractor,
+  signWebToken,
+  verifyWebToken
+};


Putting all of the JWT-related stuff in one place makes me so happy. 👍 👍

mgwalker · 2020-05-05T21:25:53Z

api/routes/openAPI/auth.js

+        400: {
+          description: 'Not logged in'


Same thing with the error on logging out

mgwalker · 2020-05-05T21:26:19Z

api/routes/openAPI/helpers.js

+      description: 'Not logged in'
    },
    403: {
-      description: 'Not logged in'
+      description: 'Does not have permission to this activity'


😂 Thank you!

mgwalker · 2020-05-05T21:26:36Z

api/routes/openAPI/index.js

+      bearerAuth: {
+        type: 'http',
+        scheme: 'bearer',
+        bearerFormat: 'Bearer xxx.yyy.zzz'


docker-compose.yml

mgwalker · 2020-05-05T21:33:50Z

web/src/actions/auth.test.js

      const store = mockStore({});
      fetchMock
        .onPost('/auth/login/nonce', { username: 'name' })
        .reply(200, { nonce: '123abc' });
      fetchMock
        .onPost('/auth/login', { username: '123abc', password: 'secret' })
-        .reply(200, { moop: 'moop', activities: [] });
+        .reply(200, { user: { moop: 'moop', activities: [] }, token: 'xxx.yyy.zzz' });


Good ol' moop moop

radavis · 2020-05-07T14:33:17Z

@mgwalker I'm pretty sure I've addressed all your requested changes. Please, take a look...

I have a few questions for you:

What do you think about the change to the jwtMiddleware?
Do you think there is any issue with the way I am handling logging out? Should logging out of one session end all of that user's sessions?
At what point in this process should I push my changes to the Wiki?

mgwalker

Looks good to me!

mgwalker · 2020-05-07T19:43:42Z

api/auth/jwtUtils.js

-  return token;
+  const token = req.get('Authorization');
+  if (!token || !token.toLowerCase().match(/^bearer\s.+\..+\..+/)) return null;
+  const [temp, result] = token.split(' ');  // eslint-disable-line no-unused-vars


As a future note, one convention for throwaway destructured values would be something like:

const [_, result] = ...

And then the linter won't complain. Only trouble is you can't do that if you have more than one throwaway variable. 😛

mgwalker · 2020-05-07T19:45:30Z

api/auth/jwtUtils.test.js

+    const scenarios = [
+      ['Bearer xxx.yyy.zzz', 'xxx.yyy.zzz', 'returns the JWT'],
+      ['bearer xxx.yyy.zzz', 'xxx.yyy.zzz', 'returns the JWT'],
+      ['bearer', null, 'returns null'],
+      ['bearer ', null, 'returns null'],
+      ['', null, 'returns null'],
+      ['Elephanter xxx.yyy.zzz', null, 'returns null']
+    ];


radavis · 2020-05-08T16:44:47Z

Notes to Design and Product: This is a back-end change, so you shouldn't see any changes in UI. Verify that this is functional by signing in and out. That's it!

beparticular

Logged in and logged out. :)

codecov-io · 2020-05-08T16:53:11Z

Codecov Report

Merging #2158 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2158   +/-   ##
=======================================
  Coverage   92.99%   92.99%           
=======================================
  Files         219      219           
  Lines        4214     4214           
  Branches      658      658           
=======================================
  Hits         3919     3919           
  Misses        273      273           
  Partials       22       22

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 66952d4...66952d4. Read the comment docs.

jeromeleecms

I can only verify this in Firefox -- but it looks like the rest of the team is good. Gtg.

radavis commented Apr 6, 2020

View reviewed changes

docker-compose.yml Outdated Show resolved Hide resolved

radavis commented Apr 15, 2020

View reviewed changes

api/auth/index.js Show resolved Hide resolved

radavis added 3 commits May 1, 2020 11:45

remove 'request' node module, refactor tests

7c8a108

manually tested & working!

007c934

fix some tests

f74b416

radavis force-pushed the 2100-operation-cookie-crumble branch from cf9fc59 to f74b416 Compare May 1, 2020 20:42

radavis added 6 commits May 1, 2020 17:41

add 'authenticate' endpoint test utility

0fe1ecd

another option

45c17ec

update test

048ad0e

update tests 👀

0022fe0

fix stuff

0c63227

fix 401/403 mixup

e55e290

radavis commented May 4, 2020

View reviewed changes

api/routes/auth/login.endpoint.js Outdated Show resolved Hide resolved

radavis changed the title ~~[WIP] Move JWT from cookie store to Authorization header~~ Move JWT from cookie store to Authorization header May 4, 2020

radavis requested review from mgwalker and pphillips-fearless May 4, 2020 15:06

radavis added 4 commits May 4, 2020 11:07

changes

1b4f815

now with fewer changes!

0fa2725

don't change files out of scope of this task

6077e01

remove 'withCredentials' from axios config, remove outdated headers test

e5be16c

mgwalker mentioned this pull request May 4, 2020

[Tech Debt] Refactor API user serialization methods #2206

Closed

1 task

radavis added 2 commits May 4, 2020 15:04

remove DISABLE_SECURE_COOKIE env var

bd918ab

don't change files out of scope of this task

06897a7

radavis commented May 4, 2020

View reviewed changes

docker-compose.yml Show resolved Hide resolved

radavis added 3 commits May 4, 2020 16:06

update tests after removing 'withCredentials' setting from api wrapper

e51c2a0

add jwtMiddleware tests

f6511f0

update OpenAPI specs

68dc06f

radavis added 2 commits May 5, 2020 13:33

lint it

36736f7

ignore options requests

9aebd31

mgwalker suggested changes May 5, 2020

View reviewed changes

radavis added 9 commits May 6, 2020 11:27

refactor auth setup and test

446107a

refactor jwtUtils based on feedback

a1012e1

corrections

5619075

updates

8c2fe85

clear the database 🤦

817120b

fix test

ecbb955

merge master

85171ab

lint fixes

d19e267

linter and api docs

06426d4

radavis requested a review from mgwalker May 7, 2020 14:22

Merge branch 'master' into 2100-operation-cookie-crumble

fbf2844

mgwalker approved these changes May 7, 2020

View reviewed changes

radavis requested review from beparticular and jeromeleecms May 8, 2020 16:42

update changelog

66952d4

radavis force-pushed the 2100-operation-cookie-crumble branch from 725a735 to 66952d4 Compare May 8, 2020 16:46

beparticular approved these changes May 8, 2020

View reviewed changes

jeromeleecms approved these changes May 8, 2020

View reviewed changes

radavis merged commit 0e1a350 into master May 8, 2020

radavis deleted the 2100-operation-cookie-crumble branch May 8, 2020 19:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move JWT from cookie store to Authorization header #2158

Move JWT from cookie store to Authorization header #2158

radavis commented Apr 6, 2020 •

edited

radavis Apr 6, 2020

cms-eapd-bot commented Apr 7, 2020 •

edited

radavis commented May 1, 2020

mgwalker left a comment

mgwalker May 4, 2020

mgwalker May 4, 2020

radavis May 6, 2020

mgwalker May 4, 2020

radavis May 6, 2020

mgwalker May 4, 2020

mgwalker May 5, 2020

mgwalker May 5, 2020

mgwalker May 5, 2020

mgwalker May 5, 2020

radavis commented May 7, 2020 •

edited

mgwalker left a comment

mgwalker May 7, 2020

mgwalker May 7, 2020

radavis commented May 8, 2020

beparticular left a comment

codecov-io commented May 8, 2020 •

edited

jeromeleecms left a comment


		const defaultStrategies = [new LocalStrategy(authenticate())];
		const passportLocalStrategy = new LocalStrategy(authenticate());

Move JWT from cookie store to Authorization header #2158

Move JWT from cookie store to Authorization header #2158

Conversation

radavis commented Apr 6, 2020 • edited

This pull request...

This pull request is ready to merge when...

This feature is done when...

Choose a reason for hiding this comment

cms-eapd-bot commented Apr 7, 2020 • edited

radavis commented May 1, 2020

mgwalker left a comment

Choose a reason for hiding this comment

change

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

radavis commented May 7, 2020 • edited

mgwalker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radavis commented May 8, 2020

beparticular left a comment

Choose a reason for hiding this comment

codecov-io commented May 8, 2020 • edited

Codecov Report

jeromeleecms left a comment

Choose a reason for hiding this comment

radavis commented Apr 6, 2020 •

edited

cms-eapd-bot commented Apr 7, 2020 •

edited

radavis commented May 7, 2020 •

edited

codecov-io commented May 8, 2020 •

edited