Change session and token handling #42

joshcanhelp · 2019-12-10T23:13:36Z

Description

This is a major breaking change to functionality. Please read the below along with the EXAMPLES.md document for how to handle your application's use case.

This PR remove the requirement for a session middleware and, instead, stores the user identity in an encrypted and signed cookie. This PR will also remove automatic storage of the TokenSet received from the authorization server.

See API.md in this PR for added configuration options and EXAMPLES.md in this PR for how to handle token storage and user sessions with server-side sessions.

Breaking Changes:

A session middleware is no longer required and will not be used automatically
The returned TokenSet is not longer stored automatically
Remove req.openid.tokens setter and getter
Change getUser function to pull from configured req property rather than req.session

References

Testing

This change adds test coverage for new/changed/fixed functionality

Checklist

I have added documentation for new/changed functionality in this PR or in auth0.com/docs
All active GitHub checks for tests, formatting, and security are passing
The correct base branch is being used, if not master

joshcanhelp · 2019-12-18T00:27:02Z

lib/context.js

  get isAuthenticated() {
    return !!this.user;
  }

+  makeTokenSet(tokenSet) {


This lets developers create a TokenSet without importing the library directly.

joshcanhelp · 2019-12-18T00:30:01Z

lib/config.js

@@ -36,6 +36,10 @@ const paramsSchema = Joi.object().keys({
  loginPath: Joi.string().optional().default('/login'),
  logoutPath: Joi.string().optional().default('/logout'),
  legacySameSiteCookie: Joi.boolean().optional().default(true),
+  sessionName: Joi.string().token().optional().default('identity'),


This is the actual cookie name for the internal session.

joshcanhelp · 2019-12-18T00:31:47Z

lib/loadEnvs.js

@@ -3,21 +3,14 @@ const fieldsEnvMap = {
  'baseURL': 'BASE_URL',
  'clientID': 'CLIENT_ID',
  'clientSecret': 'CLIENT_SECRET',
+  'sessionSecret': 'SESSION_SECRET',


SESSION_SECRET is the env name for the secret that encrypts the cookie. If this env exists then the config value does not need to be passed to the SDK config.

joshcanhelp · 2019-12-18T00:32:12Z

lib/loadEnvs.js

 };

 module.exports = function(params) {
  Object.keys(fieldsEnvMap).forEach(k => {
-    if (params[k]) {


This was treating false as not set

joshcanhelp · 2019-12-18T00:32:31Z

lib/loadEnvs.js

 };

 module.exports = function(params) {
  Object.keys(fieldsEnvMap).forEach(k => {
-    if (params[k]) {
-      return;
+    if (typeof params[k] === 'undefined') {


If the config key is not present then use the env var for specific keys.

joshcanhelp · 2019-12-18T00:33:00Z

lib/loadEnvs.js

  });
-
-  if (!params.baseURL &&


A lot of dancing around to allow a very specific case. Also: sets the URL without TLS.

joshcanhelp · 2019-12-18T00:33:54Z

middleware/auth.js

-      req.session.openidTokens = tokenSet;
+      req.openIdTokens = tokenSet;
+
+      if (config.sessionSecret) {


If there is a session secret, then we store the claims in a cookie.

gkwang · 2019-12-23T20:35:32Z

middleware/auth.js

+      if (config.sessionSecret) {
+        let identityClaims = tokenSet.claims();
+        // Remove validation claims to reduce stored size.
+        ['aud', 'iss', 'exp', 'nonce', 'azp', 'auth_time'].forEach(claim => delete identityClaims[claim]);


Would c_hash/at_hash also be included?

@gkwang - When are those properties present?

Switch to use sessionLength instead. Setting the length to 0 will indicate an ephemeral session, reducing the need for an additional key.

joshcanhelp · 2019-12-30T20:47:34Z

lib/session.js

@@ -0,0 +1,100 @@
+const { strict: assert } = require('assert');


Encryption and signing was added and reviewed here:

https://github.com/joshcanhelp/express-openid-connect/pull/1

Appending `app` to the session-realted config keys to make it more clear what they are used for.

Added a config key for the application session cookie to allow cookie options to be passed to the session cookie.

joshcanhelp · 2019-12-31T21:38:37Z

Coverage report from nyc (updated)

-----------------------------------|----------|----------|----------|----------|-------------------|
File                               |  % Stmts | % Branch |  % Funcs |  % Lines | Uncovered Line #s |
-----------------------------------|----------|----------|----------|----------|-------------------|
All files                          |    92.49 |    83.75 |    86.05 |    93.06 |                   |
 express-openid-connect            |      100 |      100 |      100 |      100 |                   |
  index.js                         |      100 |      100 |      100 |      100 |                   |
 express-openid-connect/lib        |    95.48 |       88 |    92.86 |    95.43 |                   |
  ResponseMode.js                  |      100 |      100 |      100 |      100 |                   |
  appSession.js                    |    95.74 |    78.79 |      100 |    95.56 |             46,47 |
  client.js                        |    96.88 |    84.62 |      100 |    96.88 |                63 |
  config.js                        |     97.3 |    92.31 |      100 |     97.3 |                84 |
  context.js                       |     90.2 |     87.5 |    77.78 |     90.2 |   16,20,68,83,107 |
  loadEnvs.js                      |      100 |      100 |      100 |      100 |                   |
  transientHandler.js              |      100 |      100 |      100 |      100 |                   |
 express-openid-connect/lib/hooks  |      100 |      100 |      100 |      100 |                   |
  getUser.js                       |      100 |      100 |      100 |      100 |                   |
  handleCallback.js                |      100 |      100 |      100 |      100 |                   |
 express-openid-connect/middleware |    84.34 |    63.33 |    69.23 |    86.25 |                   |
  auth.js                          |    88.41 |    57.89 |    77.78 |    90.91 |... 87,138,139,140 |
  requiresAuth.js                  |    88.89 |    88.89 |      100 |    88.89 |                20 |
  unauthorizedHandler.js           |       20 |        0 |        0 |       20 |        9,10,11,13 |
-----------------------------------|----------|----------|----------|----------|-------------------|

joshcanhelp · 2019-12-31T21:39:50Z

lib/appSession.js

@@ -0,0 +1,88 @@
+const { strict: assert } = require('assert');


Encryption added and reviewed by security here:

https://github.com/joshcanhelp/express-openid-connect/pull/1

damieng

A few questions still, none blockers.

EXAMPLES.md

damieng · 2020-01-03T06:09:32Z

EXAMPLES.md

 });
 ```

-## 5. Calling userinfo
+## 7. Calling userinfo


Maybe a better title to match the access token one?

I struggled with a title here because you only need to call userinfo in specific cases and I'm not sure if I could shorten that down to something universally-clear in the title. I added a bit more color in the description, though.

damieng · 2020-01-03T06:10:46Z

EXAMPLES.md

      next();
    } catch(e) {
      next(e);
    }
  },
-  getUser: function(tokenSet) {


Do we not need this now? The descriptive text indicates we do but it looks like line 221 is mapping claims directly now. One of these seems out of step with the other.

Good catch. That example was not working properly with this new system.

joshcanhelp · 2020-01-03T15:30:47Z

@damieng - Last change blew away your approval. Ready again!

joshcanhelp added 5 commits December 13, 2019 07:09

Remove session use, switch to cookie session for identity

8e5b708

Documentation changes and add req.makeTokenSet()

dc78ac4

Adjust internal session defaults; add docs

4e28342

Allow internal session handling to be turned off

d952cff

Move identity claim filtering to callback; examples

348c0a7

joshcanhelp commented Dec 18, 2019

View reviewed changes

gkwang reviewed Dec 23, 2019

View reviewed changes

panva and others added 3 commits December 30, 2019 07:05

JWE based session cookie mechanism

0978b1f

use conform hkdf

fa628bf

Remove sessionEphemeral

c67777d

Switch to use sessionLength instead. Setting the length to 0 will indicate an ephemeral session, reducing the need for an additional key.

joshcanhelp commented Dec 30, 2019

View reviewed changes

joshcanhelp added 6 commits December 30, 2019 13:29

Rename session config keys

d0b4399

Appending `app` to the session-realted config keys to make it more clear what they are used for.

Use single prop for property and cookie names

01a35c2

Add appSessionCookie config key

0d37cb7

Added a config key for the application session cookie to allow cookie options to be passed to the session cookie.

Fix existing tests

bdda90b

Fix session cookie config and add tests

048e259

Rename appSession, light refacotring, and tests

ef888df

joshcanhelp commented Dec 31, 2019

View reviewed changes

joshcanhelp requested a review from gkwang December 31, 2019 21:41

joshcanhelp added CH: Changed review:large Large review labels Dec 31, 2019

joshcanhelp added this to the v0.6.0 milestone Dec 31, 2019

joshcanhelp marked this pull request as ready for review December 31, 2019 21:41

joshcanhelp added 4 commits January 2, 2020 21:48

Improve examples wording for clarity

4908210

Rename appSessionLength to appSessionDuration

b33476c

Add filter to config for identity claims

29fda77

Keep httpOnly setting if session cooie opts are passed

76f8377

joshcanhelp requested a review from damieng January 3, 2020 06:03

damieng previously approved these changes Jan 3, 2020

View reviewed changes

Update example for userinfo

370cf78

joshcanhelp dismissed damieng’s stale review via 370cf78 January 3, 2020 15:28

joshcanhelp requested a review from damieng January 3, 2020 15:30

damieng approved these changes Jan 3, 2020

View reviewed changes

joshcanhelp merged commit 69572c6 into auth0:master Jan 3, 2020

joshcanhelp deleted the add-internal-session-for-claims branch January 3, 2020 22:20

joshcanhelp mentioned this pull request Jan 6, 2020

Add typescript defs new config #46

Merged

joshcanhelp changed the title ~~Add internal session for claims~~ Change session and token handling Jan 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change session and token handling #42

Change session and token handling #42

joshcanhelp commented Dec 10, 2019 •

edited

Loading

joshcanhelp Dec 18, 2019

joshcanhelp Dec 18, 2019

joshcanhelp Dec 18, 2019

joshcanhelp Dec 18, 2019

joshcanhelp Dec 18, 2019

joshcanhelp Dec 18, 2019

joshcanhelp Dec 18, 2019

gkwang Dec 23, 2019

joshcanhelp Dec 31, 2019

joshcanhelp Dec 30, 2019

joshcanhelp commented Dec 31, 2019 •

edited

Loading

joshcanhelp Dec 31, 2019

damieng left a comment

damieng Jan 3, 2020

joshcanhelp Jan 3, 2020

damieng Jan 3, 2020

joshcanhelp Jan 3, 2020

joshcanhelp commented Jan 3, 2020

		@@ -0,0 +1,100 @@
		const { strict: assert } = require('assert');

		@@ -0,0 +1,88 @@
		const { strict: assert } = require('assert');

Change session and token handling #42

Change session and token handling #42

Conversation

joshcanhelp commented Dec 10, 2019 • edited Loading

Description

References

Testing

Checklist

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

joshcanhelp commented Dec 31, 2019 • edited Loading

Choose a reason for hiding this comment

damieng 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

joshcanhelp commented Jan 3, 2020

joshcanhelp commented Dec 10, 2019 •

edited

Loading

joshcanhelp commented Dec 31, 2019 •

edited

Loading