From deb4429e3afacfb33292e935558d521a3957755a Mon Sep 17 00:00:00 2001 From: Kagami Sascha Rosylight Date: Sun, 9 Apr 2023 21:21:10 +0200 Subject: [PATCH] return scope in token response --- .../src/server/oauth/OAuth2ProviderService.ts | 12 +-- packages/backend/test/e2e/oauth.ts | 98 +++++++++++++++++-- 2 files changed, 98 insertions(+), 12 deletions(-) diff --git a/packages/backend/src/server/oauth/OAuth2ProviderService.ts b/packages/backend/src/server/oauth/OAuth2ProviderService.ts index 677a85473f..a18b3519a5 100644 --- a/packages/backend/src/server/oauth/OAuth2ProviderService.ts +++ b/packages/backend/src/server/oauth/OAuth2ProviderService.ts @@ -384,7 +384,6 @@ export class OAuth2ProviderService { if (!body.code_verifier || pkceS256(body.code_verifier) !== granted.codeChallenge) return [false]; const accessToken = secureRndstr(128, true); - const refreshToken = secureRndstr(128, true); const now = new Date(); @@ -400,7 +399,7 @@ export class OAuth2ProviderService { permission: granted.scopes, }); - return [accessToken, refreshToken]; + return [accessToken, { scope: granted.scopes.join(' ') }]; })().then(args => done(null, ...args), err => done(err)); })); this.#server.serializeClient((client, done) => done(null, client)); @@ -441,7 +440,7 @@ export class OAuth2ProviderService { // this feature for some time, given that this is security related. fastify.get<{ Querystring: OAuthRequestQuery }>('/oauth/authorize', async (request, reply) => { console.log('HIT /oauth/authorize', request.query); - const oauth2 = (request.raw as any).oauth2 as (OAuth2 | undefined); + const oauth2 = (request.raw as any).oauth2 as OAuth2; console.log(oauth2, request.raw.session); if (request.query.response_type !== 'code') { @@ -454,15 +453,16 @@ export class OAuth2ProviderService { throw new Error('`code_challenge_method` parameter must be set as S256'); } - const scopes = [...new Set(oauth2?.req.scope)].filter(s => kinds.includes(s)); + const scopes = [...new Set(oauth2.req.scope)].filter(s => kinds.includes(s)); if (!scopes.length) { throw new Error('`scope` parameter has no known scope'); } + oauth2.req.scope = scopes; reply.header('Cache-Control', 'no-store'); return await reply.view('oauth', { - transactionId: oauth2?.transactionID, - clientId: oauth2?.client, + transactionId: oauth2.transactionID, + clientId: oauth2.client, scope: scopes.join(' '), }); }); diff --git a/packages/backend/test/e2e/oauth.ts b/packages/backend/test/e2e/oauth.ts index 5dd0d7c39f..17fcea9e9d 100644 --- a/packages/backend/test/e2e/oauth.ts +++ b/packages/backend/test/e2e/oauth.ts @@ -104,8 +104,8 @@ describe('OAuth', () => { code_verifier, }); assert.strictEqual(typeof token.token.access_token, 'string'); - assert.strictEqual(typeof token.token.refresh_token, 'string'); assert.strictEqual(token.token.token_type, 'Bearer'); + assert.strictEqual(token.token.scope, 'write:notes'); const createResponse = await relativeFetch('api/notes/create', { method: 'POST', @@ -278,19 +278,39 @@ describe('OAuth', () => { }); test('Partially known scopes', async () => { + const { code_challenge, code_verifier } = pkceChallenge.default(128); + const client = getClient(); const response = await fetch(client.authorizeURL({ redirect_uri, scope: 'write:notes test:unknown test:unknown2', state: 'state', - code_challenge: 'code', + code_challenge, code_challenge_method: 'S256', })); // Just get the known scope for this case for backward compatibility assert.strictEqual(response.status, 200); - // TODO: OAuth2 requires returning `scope` in the token response in this case but oauth2orize seemingly doesn't support this + + const decisionResponse = await fetchDecisionFromResponse(response, alice); + assert.strictEqual(decisionResponse.status, 302); + + const location = new URL(decisionResponse.headers.get('location')!); + assert.ok(location.searchParams.has('code')); + + const code = new URL(decisionResponse.headers.get('location')!).searchParams.get('code')!; + assert.ok(!!code); + + const token = await client.getToken({ + code, + redirect_uri, + code_verifier, + }); + + // OAuth2 requires returning `scope` in the token response if the resulting scope is different than the requested one + // (Although Misskey always return scope, which is also fine) + assert.strictEqual(token.token.scope, 'write:notes'); }); test('Known scopes', async () => { @@ -304,13 +324,79 @@ describe('OAuth', () => { code_challenge_method: 'S256', })); - // Just get the known scope for this case for backward compatibility assert.strictEqual(response.status, 200); }); - // TODO: duplicate scopes test (currently token response doesn't return final scopes, although it must) + test('Duplicated scopes', async () => { + const { code_challenge, code_verifier } = pkceChallenge.default(128); - // TODO: write failure when no scope + const client = getClient(); + + const response = await fetch(client.authorizeURL({ + redirect_uri, + scope: 'write:notes write:notes read:account read:account', + state: 'state', + code_challenge, + code_challenge_method: 'S256', + })); + + assert.strictEqual(response.status, 200); + + const decisionResponse = await fetchDecisionFromResponse(response, alice); + assert.strictEqual(decisionResponse.status, 302); + + const location = new URL(decisionResponse.headers.get('location')!); + assert.ok(location.searchParams.has('code')); + + const code = new URL(decisionResponse.headers.get('location')!).searchParams.get('code')!; + assert.ok(!!code); + + const token = await client.getToken({ + code, + redirect_uri, + code_verifier, + }); + assert.strictEqual(token.token.scope, 'write:notes read:account'); + }); + + test('Scope check by API', async () => { + const { code_challenge, code_verifier } = pkceChallenge.default(128); + + const client = getClient(); + + const response = await fetch(client.authorizeURL({ + redirect_uri, + scope: 'read:account', + state: 'state', + code_challenge, + code_challenge_method: 'S256', + })); + assert.strictEqual(response.status, 200); + + const decisionResponse = await fetchDecisionFromResponse(response, alice); + assert.strictEqual(decisionResponse.status, 302); + + const location = new URL(decisionResponse.headers.get('location')!); + assert.ok(location.searchParams.has('code')); + + const token = await client.getToken({ + code: location.searchParams.get('code')!, + redirect_uri, + code_verifier, + }); + assert.strictEqual(typeof token.token.access_token, 'string'); + + const createResponse = await relativeFetch('api/notes/create', { + method: 'POST', + headers: { + Authorization: `Bearer ${token.token.access_token}`, + 'Content-Type': 'application/json', + }, + body: JSON.stringify({ text: 'test' }), + }); + // XXX: PERMISSION_DENIED is not using kind: 'permission' and gives 400 instead of 403 + assert.strictEqual(createResponse.status, 400); + }); }); test('Authorization header', async () => {