Skip to content

Commit

Permalink
fix: keep grants that persist if logged out by that grant's client
Browse files Browse the repository at this point in the history
closes #857
  • Loading branch information
panva committed Jan 19, 2021
1 parent 31f6332 commit 26449f5
Show file tree
Hide file tree
Showing 3 changed files with 32 additions and 21 deletions.
13 changes: 3 additions & 10 deletions lib/actions/end_session.js
Original file line number Diff line number Diff line change
Expand Up @@ -180,17 +180,10 @@ module.exports = {
if (session.authorizations) {
await Promise.all(
Object.entries(session.authorizations).map(async ([clientId, { grantId }]) => {
// 1) drop the grants for the client that requested a logout
// 2) drop the grants without offline_access
// Drop the grants without offline_access
// 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 (
grantId
&& (
clientId === state.clientId
|| !session.authorizationFor(clientId).persistsLogout
)
) {
if (grantId && !session.authorizationFor(clientId).persistsLogout) {
const client = await ctx.oidc.provider.Client.find(clientId).catch(() => {});
await revokeGrant(ctx.oidc.provider, client, grantId);
ctx.oidc.provider.emit('grant.revoked', ctx, grantId);
Expand Down Expand Up @@ -228,7 +221,7 @@ module.exports = {
);
} else if (state.clientId) {
const grantId = session.grantIdFor(state.clientId);
if (grantId) {
if (grantId && !session.authorizationFor(state.clientId).persistsLogout) {
const client = await ctx.oidc.provider.Client.find(state.clientId).catch(() => {});
await revokeGrant(ctx.oidc.provider, client, grantId);
ctx.oidc.provider.emit('grant.revoked', ctx, grantId);
Expand Down
8 changes: 4 additions & 4 deletions test/end_session/end_session.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,16 @@ module.exports = {
clients: [
{
client_id: 'client',
response_types: ['id_token'],
grant_types: ['implicit'],
response_types: ['id_token', 'code'],
grant_types: ['implicit', 'authorization_code'],
redirect_uris: ['https://client.example.com/cb'],
token_endpoint_auth_method: 'none',
},
{
client_id: 'client-hmac',
client_secret: 'secret',
response_types: ['id_token'],
grant_types: ['implicit'],
response_types: ['id_token', 'code'],
grant_types: ['implicit', 'authorization_code'],
redirect_uris: ['https://client.example.com/cb'],
token_endpoint_auth_method: 'none',
id_token_signed_response_alg: 'HS256',
Expand Down
32 changes: 25 additions & 7 deletions test/end_session/end_session.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -439,30 +439,46 @@ describe('logout endpoint', () => {

it('destroys complete session if user wants to', function () {
const sessionId = this.getSessionId();
const adapter = this.TestAdapter.for('Session');
sinon.spy(adapter, 'destroy');
sinon.spy(adapter, 'upsert');
const sessionAdapter = this.TestAdapter.for('Session');
sinon.spy(sessionAdapter, 'destroy');
sinon.spy(sessionAdapter, 'upsert');
const authorizationCodeAdapter = this.TestAdapter.for('AuthorizationCode');
sinon.spy(authorizationCodeAdapter, 'revokeByGrantId');
const session = this.getSession();

this.getSession().state = { secret: '123', postLogoutRedirectUri: '/', clientId: 'client' };
session.state = { secret: '123', postLogoutRedirectUri: '/', clientId: 'client' };
session.authorizations.client.persistsLogout = true;

const [firstGrant, secondGrant] = Object.keys(session.authorizations)
.map((x) => session.authorizations[x].grantId);

return this.agent.post('/session/end/confirm')
.send({ xsrf: '123', logout: 'yes' })
.type('form')
.expect(302)
.expect((response) => {
expect(adapter.destroy.called).to.be.true;
expect(adapter.upsert.called).not.to.be.true;
expect(adapter.destroy.withArgs(sessionId).calledOnce).to.be.true;
expect(sessionAdapter.destroy.called).to.be.true;
expect(sessionAdapter.upsert.called).not.to.be.true;
expect(sessionAdapter.destroy.withArgs(sessionId).calledOnce).to.be.true;
expect(parseUrl(response.headers.location, true).query).not.to.have.property('client_id');
expect(authorizationCodeAdapter
.revokeByGrantId.calledOnce).to.be.true;
expect(authorizationCodeAdapter
.revokeByGrantId.withArgs(firstGrant).calledOnce).to.be.false;
expect(authorizationCodeAdapter
.revokeByGrantId.withArgs(secondGrant).calledOnce).to.be.true;
});
});

it('only clears one clients session if user doesnt wanna log out (using post_logout_redirect_uri)', function () {
const adapter = this.TestAdapter.for('Session');
sinon.spy(adapter, 'destroy');
const authorizationCodeAdapter = this.TestAdapter.for('AuthorizationCode');
sinon.spy(authorizationCodeAdapter, 'revokeByGrantId');
let session = this.getSession();
const oldId = this.getSessionId();
session.state = { secret: '123', postLogoutRedirectUri: 'https://rp.example.com/logout/cb', clientId: 'client' };
session.authorizations.client.persistsLogout = true;

expect(session.authorizations.client).to.be.ok;

Expand All @@ -477,6 +493,8 @@ describe('logout endpoint', () => {
expect(this.getSessionId()).not.to.eql(oldId);
expect(adapter.destroy.calledOnceWith(oldId)).to.be.true;
expect(parseUrl(response.headers.location, true).query).not.to.have.key('client_id');
expect(authorizationCodeAdapter
.revokeByGrantId.called).to.be.false;
});
});

Expand Down

0 comments on commit 26449f5

Please sign in to comment.