fix: principal-change triggered logout fixes

This fixes the adapter.revokeByGrantId being occasionally called with
`undefined` as well as an unintended
`interaction session and authentication session mismatch` throw
caused by the principal-change triggered logout's underlying session
being removed.

fixes #628
fixes #600

lib/actions/authorization/interactions.js

@@ -108,13 +108,7 @@ module.exports = async function interactions(resumeRouteName, ctx, next) {
     uid: oidc.uid,
     params: oidc.params.toPlainObject(),
     signed: oidc.signed,
-    session: oidc.session.accountId() ? {
-      accountId: oidc.session.accountId(),
-      ...(oidc.session.uid ? { uid: oidc.session.uid } : undefined),
-      ...(oidc.session.jti ? { cookie: oidc.session.jti } : undefined),
-      ...(oidc.session.acr ? { acr: oidc.session.acr } : undefined),
-      ...(oidc.session.amr ? { amr: oidc.session.amr } : undefined),
-    } : undefined,
+    session: oidc.session,
   });
 
   await interactionSession.save(cookieOptions.maxAge / 1000);

lib/actions/authorization/resume.js

@@ -10,6 +10,7 @@ const instance = require('../../helpers/weak_cache');
 const Params = require('../../helpers/params');
 const formPost = require('../../response_modes/form_post');
 const ssHandler = require('../../helpers/samesite_handler');
+const epochTime = require('../../helpers/epoch_time');
 
 module.exports = async function resumeAction(whitelist, resumeRouteName, ctx, next) {
   const { maxAge, expires, ...cookieOptions } = instance(ctx.oidc.provider).configuration('cookies.short');
@@ -48,6 +49,11 @@ module.exports = async function resumeAction(whitelist, resumeRouteName, ctx, ne
     && session.account
     && session.account !== result.login.account
   ) {
+    if (interactionSession.session && interactionSession.session.uid) {
+      delete interactionSession.session.uid;
+      await interactionSession.save(interactionSession.exp - epochTime());
+    }
+
     session.state = {
       secret: nanoid(),
       clientId: storedParams.client_id,

lib/actions/end_session.js

@@ -176,8 +176,11 @@ module.exports = {
               // Note: tokens that don't get dropped due to offline_access having being added
               // later will still not work, as such they will be orphaned until their TTL hits
               if (
-                clientId === state.clientId
-                || !session.authorizationFor(clientId).persistsLogout
+                grantId
+                && (
+                  clientId === state.clientId
+                  || !session.authorizationFor(clientId).persistsLogout
+                )
               ) {
                 const client = await ctx.oidc.provider.Client.find(clientId).catch(() => {});
                 await revokeGrant(ctx.oidc.provider, client, grantId);

lib/models/interaction.js

@@ -5,6 +5,20 @@ const hasFormat = require('./mixins/has_format');
 module.exports = (provider) => class Interaction extends hasFormat(provider, 'Interaction', instance(provider).BaseModel) {
   constructor(id, payload) {
     if (arguments.length === 2) {
+      if (payload.session instanceof instance(provider).BaseModel) {
+        const { session } = payload;
+        const accountId = session.accountId();
+        Object.assign(payload, accountId ? {
+          session: {
+            accountId,
+            ...(session.uid ? { uid: session.uid } : undefined),
+            ...(session.jti ? { cookie: session.jti } : undefined),
+            ...(session.acr ? { acr: session.acr } : undefined),
+            ...(session.amr ? { amr: session.amr } : undefined),
+          },
+        } : { session: undefined });
+      }
+
       super({ ...payload, jti: id });
     } else {
       super(id);

test/device_code/device_resume.test.js

@@ -38,8 +38,8 @@ describe('device interaction resume /device/:user_code/:uid/', () => {
       ...auth,
     };
 
-    const interaction = new this.provider.Interaction(grantId, {});
     const session = new this.provider.Session({ jti: 'sess', ...sessionData });
+    const interaction = new this.provider.Interaction(grantId, { session });
     const keys = new KeyGrip(i(this.provider).configuration('cookies.keys'));
     const code = new this.provider.DeviceCode({
       params,
@@ -370,20 +370,34 @@ describe('device interaction resume /device/:user_code/:uid/', () => {
           account: nanoid(),
         });
 
+        let state;
+
         await this.agent.get(path)
           .expect(200)
           .expect('content-type', 'text/html; charset=utf-8')
           .expect(/<body onload="javascript:document\.forms\[0]\.submit\(\)"/)
           .expect(/<input type="hidden" name="logout" value="yes"\/>/)
           .expect(({ text }) => {
-            const { state } = this.getSession();
+            ({ state } = this.getSession());
             expect(state).to.have.property('clientId', 'client');
             expect(state).to.have.property('postLogoutRedirectUri').that.matches(new RegExp(`${path}$`));
             expect(text).to.match(new RegExp(`input type="hidden" name="xsrf" value="${state.secret}"`));
           })
           .expect(/<form method="post" action=".+\/session\/end\/confirm">/);
 
         expect(await this.provider.Interaction.find(grantId)).to.be.ok;
+
+        await this.agent.post('/session/end/confirm')
+          .send({
+            xsrf: state.secret,
+            logout: 'yes',
+          })
+          .type('form')
+          .expect(302)
+          .expect('location', state.postLogoutRedirectUri);
+
+        await this.agent.get(state.postLogoutRedirectUri.replace(this.provider.issuer, ''))
+          .expect(200);
       });
     });
 

test/end_session/end_session.test.js

@@ -2,6 +2,7 @@ const { parse: parseUrl } = require('url');
 
 const sinon = require('sinon').createSandbox();
 const { expect } = require('chai');
+const timekeeper = require('timekeeper');
 
 const bootstrap = require('../test_helper');
 const JWT = require('../../lib/helpers/jwt');
@@ -11,6 +12,7 @@ const route = '/session/end';
 
 describe('logout endpoint', () => {
   before(bootstrap(__dirname));
+  afterEach(() => timekeeper.reset());
 
   describe('when logged out', () => {
     ['get', 'post'].forEach((verb) => {
@@ -63,6 +65,21 @@ describe('logout endpoint', () => {
             (await this.provider.Client.find('client')).postLogoutRedirectUris = [];
           });
 
+          it('even when expired', function () {
+            timekeeper.travel(Date.now() + ((3600 + 10) * 1000));
+            const params = {
+              id_token_hint: this.idToken,
+              post_logout_redirect_uri: 'https://client.example.com/logout/cb',
+            };
+
+            return this.wrap({ route, verb, params })
+              .expect(200)
+              .expect(() => {
+                const { state: { postLogoutRedirectUri } } = this.getSession();
+                expect(postLogoutRedirectUri).to.equal('https://client.example.com/logout/cb');
+              });
+          });
+
           it('allows to redirect there', function () {
             const params = {
               id_token_hint: this.idToken,

test/interaction/interaction.test.js

@@ -321,8 +321,11 @@ describe('resume after consent', () => {
   function setup(grant, result, sessionData) {
     const cookies = [];
 
-    const interaction = new this.provider.Interaction('resume', {});
     const session = new this.provider.Session({ jti: 'sess', ...sessionData });
+    const interaction = new this.provider.Interaction('resume', {
+      params: grant,
+      session,
+    });
     const keys = new KeyGrip(i(this.provider).configuration('cookies.keys'));
 
     expect(grant).to.be.ok;
@@ -331,7 +334,6 @@ describe('resume after consent', () => {
     cookies.push(cookie);
     let [pre, ...post] = cookie.split(';');
     cookies.push([`_interaction_resume.sig=${keys.sign(pre)}`, ...post].join(';'));
-    Object.assign(interaction, { params: grant });
 
     const sessionCookie = `_session=sess; path=/; expires=${expire.toGMTString()}; httponly`;
     cookies.push(sessionCookie);
@@ -475,20 +477,36 @@ describe('resume after consent', () => {
         account: nanoid(),
       });
 
+      let state;
+
       await this.agent.get('/auth/resume')
         .expect(200)
         .expect('content-type', 'text/html; charset=utf-8')
         .expect(/<body onload="javascript:document\.forms\[0]\.submit\(\)"/)
         .expect(/<input type="hidden" name="logout" value="yes"\/>/)
         .expect(({ text }) => {
-          const { state } = this.getSession();
+          ({ state } = this.getSession());
           expect(state).to.have.property('clientId', 'client');
           expect(state).to.have.property('postLogoutRedirectUri').that.matches(/\/auth\/resume$/);
           expect(text).to.match(new RegExp(`input type="hidden" name="xsrf" value="${state.secret}"`));
         })
         .expect(/<form method="post" action=".+\/session\/end\/confirm">/);
 
       expect(await this.provider.Interaction.find('resume')).to.be.ok;
+
+      await this.agent.post('/session/end/confirm')
+        .send({
+          xsrf: state.secret,
+          logout: 'yes',
+        })
+        .type('form')
+        .expect(302)
+        .expect('location', /\/auth\/resume$/);
+
+      await this.agent.get('/auth/resume')
+        .expect(302)
+        .expect(auth.validateState)
+        .expect(auth.validateClientLocation);
     });
   });
 

test/revocation/revocation.test.js

@@ -409,6 +409,7 @@ describe('revocation features', () => {
         (async () => {
           const at = new this.provider.AccessToken({
             accountId: 'accountId',
+            grantId: 'foo',
             clientId: 'client',
             scope: 'scope',
           });
@@ -431,6 +432,7 @@ describe('revocation features', () => {
         (async () => {
           const rt = new this.provider.RefreshToken({
             accountId: 'accountId',
+            grantId: 'foo',
             clientId: 'client',
             scope: 'scope',
           });

types/index.d.ts

@@ -176,7 +176,7 @@ declare class Interaction extends BaseModel {
   readonly kind: 'Interaction';
   iat: number;
   exp: number;
-  session?: {
+  session?: Session | {
     accountId: string;
     cookie: string;
     jti?: string;