Skip to content

Commit

Permalink
fix: signin の資格情報が足りないだけの場合はエラーにせず200を返すように (#14700)
Browse files Browse the repository at this point in the history 
* fix: signin の資格情報が足りないだけの場合はエラーにせず200を返すように

* run api extractor

* fix

* fix

* fix test

* /signin -> /signin-flow

* fix

* fix lint

* rename

* fix

* fix
  • Loading branch information
@kakkokari-gtyih
kakkokari-gtyih authored Oct 5, 2024
1 parent fa06c59 commit ae3c155
Show file tree
Hide file tree
Showing 13 changed files with 231 additions and 235 deletions.
2 changes: 1 addition & 1 deletion cypress/e2e/basic.cy.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ describe('After user signup', () => {
it('signin', () => {
cy.visitHome();

cy.intercept('POST', '/api/signin').as('signin');
cy.intercept('POST', '/api/signin-flow').as('signin');

cy.get('[data-cy-signin]').click();

Expand Down
2 changes: 1 addition & 1 deletion cypress/support/commands.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ Cypress.Commands.add('registerUser', (username, password, isAdmin = false) => {
Cypress.Commands.add('login', (username, password) => {
cy.visitHome();

cy.intercept('POST', '/api/signin').as('signin');
cy.intercept('POST', '/api/signin-flow').as('signin');

cy.get('[data-cy-signin]').click();
cy.get('[data-cy-signin-page-input]').should('be.visible', { timeout: 1000 });
Expand Down
2 changes: 1 addition & 1 deletion packages/backend/src/server/api/ApiServerService.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ export class ApiServerService {
'turnstile-response'?: string;
'm-captcha-response'?: string;
};
}>('/signin', (request, reply) => this.signinApiService.signin(request, reply));
}>('/signin-flow', (request, reply) => this.signinApiService.signin(request, reply));

fastify.post<{
Body: {
Expand Down
66 changes: 20 additions & 46 deletions packages/backend/src/server/api/SigninApiService.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@

import { Inject, Injectable } from '@nestjs/common';
import bcrypt from 'bcryptjs';
import * as OTPAuth from 'otpauth';
import { IsNull } from 'typeorm';
import * as Misskey from 'misskey-js';
import { DI } from '@/di-symbols.js';
import type {
MiMeta,
Expand All @@ -26,27 +26,9 @@ import { CaptchaService } from '@/core/CaptchaService.js';
import { FastifyReplyError } from '@/misc/fastify-reply-error.js';
import { RateLimiterService } from './RateLimiterService.js';
import { SigninService } from './SigninService.js';
import type { AuthenticationResponseJSON, PublicKeyCredentialRequestOptionsJSON } from '@simplewebauthn/types';
import type { AuthenticationResponseJSON } from '@simplewebauthn/types';
import type { FastifyReply, FastifyRequest } from 'fastify';

/**
* next を指定すると、次にクライアント側で行うべき処理を指定できる。
*
* - `captcha`: パスワードと、（有効になっている場合は）CAPTCHAを求める
* - `password`: パスワードを求める
* - `totp`: ワンタイムパスワードを求める
* - `passkey`: WebAuthn認証を求める（WebAuthnに対応していないブラウザの場合はワンタイムパスワード）
*/

type SigninErrorResponse = {
id: string;
next?: 'captcha' | 'password' | 'totp';
} | {
id: string;
next: 'passkey';
authRequest: PublicKeyCredentialRequestOptionsJSON;
};

@Injectable()
export class SigninApiService {
constructor(
Expand Down Expand Up @@ -101,7 +83,7 @@ export class SigninApiService {
const password = body['password'];
const token = body['token'];

function error(status: number, error: SigninErrorResponse) {
function error(status: number, error: { id: string }) {
reply.code(status);
return { error };
}
Expand Down Expand Up @@ -152,21 +134,17 @@ export class SigninApiService {
const securityKeysAvailable = await this.userSecurityKeysRepository.countBy({ userId: user.id }).then(result => result >= 1);

if (password == null) {
reply.code(403);
reply.code(200);
if (profile.twoFactorEnabled) {
return {
error: {
id: '144ff4f8-bd6c-41bc-82c3-b672eb09efbf',
next: 'password',
},
} satisfies { error: SigninErrorResponse };
finished: false,
next: 'password',
} satisfies Misskey.entities.SigninFlowResponse;
} else {
return {
error: {
id: '144ff4f8-bd6c-41bc-82c3-b672eb09efbf',
next: 'captcha',
},
} satisfies { error: SigninErrorResponse };
finished: false,
next: 'captcha',
} satisfies Misskey.entities.SigninFlowResponse;
}
}

Expand All @@ -178,7 +156,7 @@ export class SigninApiService {
// Compare password
const same = await bcrypt.compare(password, profile.password!);

const fail = async (status?: number, failure?: SigninErrorResponse) => {
const fail = async (status?: number, failure?: { id: string; }) => {
// Append signin history
await this.signinsRepository.insert({
id: this.idService.gen(),
Expand Down Expand Up @@ -268,27 +246,23 @@ export class SigninApiService {

const authRequest = await this.webAuthnService.initiateAuthentication(user.id);

reply.code(403);
reply.code(200);
return {
error: {
id: '06e661b9-8146-4ae3-bde5-47138c0ae0c4',
next: 'passkey',
authRequest,
},
} satisfies { error: SigninErrorResponse };
finished: false,
next: 'passkey',
authRequest,
} satisfies Misskey.entities.SigninFlowResponse;
} else {
if (!same || !profile.twoFactorEnabled) {
return await fail(403, {
id: '932c904e-9460-45b7-9ce6-7ed33be7eb2c',
});
} else {
reply.code(403);
reply.code(200);
return {
error: {
id: '144ff4f8-bd6c-41bc-82c3-b672eb09efbf',
next: 'totp',
},
} satisfies { error: SigninErrorResponse };
finished: false,
next: 'totp',
} satisfies Misskey.entities.SigninFlowResponse;
}
}
// never get here
Expand Down
6 changes: 4 additions & 2 deletions packages/backend/src/server/api/SigninService.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
*/

import { Inject, Injectable } from '@nestjs/common';
import * as Misskey from 'misskey-js';
import { DI } from '@/di-symbols.js';
import type { SigninsRepository, UserProfilesRepository } from '@/models/_.js';
import { IdService } from '@/core/IdService.js';
Expand Down Expand Up @@ -57,9 +58,10 @@ export class SigninService {

reply.code(200);
return {
finished: true,
id: user.id,
i: user.token,
};
i: user.token!,
} satisfies Misskey.entities.SigninFlowResponse;
}
}

73 changes: 31 additions & 42 deletions packages/backend/test/e2e/2fa.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ describe('2要素認証', () => {
keyName: string,
credentialId: Buffer,
requestOptions: PublicKeyCredentialRequestOptionsJSON,
}): misskey.entities.SigninRequest => {
}): misskey.entities.SigninFlowRequest => {
// AuthenticatorAssertionResponse.authenticatorData
// https://developer.mozilla.org/en-US/docs/Web/API/AuthenticatorAssertionResponse/authenticatorData
const authenticatorData = Buffer.concat([
Expand Down Expand Up @@ -196,22 +196,21 @@ describe('2要素認証', () => {
}, alice);
assert.strictEqual(doneResponse.status, 200);

const signinWithoutTokenResponse = await api('signin', {
const signinWithoutTokenResponse = await api('signin-flow', {
...signinParam(),
});
assert.strictEqual(signinWithoutTokenResponse.status, 403);
assert.strictEqual(signinWithoutTokenResponse.status, 200);
assert.deepStrictEqual(signinWithoutTokenResponse.body, {
error: {
id: '144ff4f8-bd6c-41bc-82c3-b672eb09efbf',
next: 'totp',
},
finished: false,
next: 'totp',
});

const signinResponse = await api('signin', {
const signinResponse = await api('signin-flow', {
...signinParam(),
token: otpToken(registerResponse.body.secret),
});
assert.strictEqual(signinResponse.status, 200);
assert.strictEqual(signinResponse.body.finished, true);
assert.notEqual(signinResponse.body.i, undefined);

// 後片付け
Expand Down Expand Up @@ -252,29 +251,23 @@ describe('2要素認証', () => {
assert.strictEqual(keyDoneResponse.body.id, credentialId.toString('base64url'));
assert.strictEqual(keyDoneResponse.body.name, keyName);

const signinResponse = await api('signin', {
const signinResponse = await api('signin-flow', {
...signinParam(),
});
const signinResponseBody = signinResponse.body as unknown as {
error: {
id: string;
next: 'passkey';
authRequest: PublicKeyCredentialRequestOptionsJSON;
};
};
assert.strictEqual(signinResponse.status, 403);
assert.strictEqual(signinResponseBody.error.id, '06e661b9-8146-4ae3-bde5-47138c0ae0c4');
assert.strictEqual(signinResponseBody.error.next, 'passkey');
assert.notEqual(signinResponseBody.error.authRequest.challenge, undefined);
assert.notEqual(signinResponseBody.error.authRequest.allowCredentials, undefined);
assert.strictEqual(signinResponseBody.error.authRequest.allowCredentials && signinResponseBody.error.authRequest.allowCredentials[0]?.id, credentialId.toString('base64url'));

const signinResponse2 = await api('signin', signinWithSecurityKeyParam({
assert.strictEqual(signinResponse.status, 200);
assert.strictEqual(signinResponse.body.finished, false);
assert.strictEqual(signinResponse.body.next, 'passkey');
assert.notEqual(signinResponse.body.authRequest.challenge, undefined);
assert.notEqual(signinResponse.body.authRequest.allowCredentials, undefined);
assert.strictEqual(signinResponse.body.authRequest.allowCredentials && signinResponse.body.authRequest.allowCredentials[0]?.id, credentialId.toString('base64url'));

const signinResponse2 = await api('signin-flow', signinWithSecurityKeyParam({
keyName,
credentialId,
requestOptions: signinResponseBody.error.authRequest,
requestOptions: signinResponse.body.authRequest,
}));
assert.strictEqual(signinResponse2.status, 200);
assert.strictEqual(signinResponse2.body.finished, true);
assert.notEqual(signinResponse2.body.i, undefined);

// 後片付け
Expand Down Expand Up @@ -320,32 +313,26 @@ describe('2要素認証', () => {
assert.strictEqual(iResponse.status, 200);
assert.strictEqual(iResponse.body.usePasswordLessLogin, true);

const signinResponse = await api('signin', {
const signinResponse = await api('signin-flow', {
...signinParam(),
password: '',
});
const signinResponseBody = signinResponse.body as unknown as {
error: {
id: string;
next: 'passkey';
authRequest: PublicKeyCredentialRequestOptionsJSON;
};
};
assert.strictEqual(signinResponse.status, 403);
assert.strictEqual(signinResponseBody.error.id, '06e661b9-8146-4ae3-bde5-47138c0ae0c4');
assert.strictEqual(signinResponseBody.error.next, 'passkey');
assert.notEqual(signinResponseBody.error.authRequest.challenge, undefined);
assert.notEqual(signinResponseBody.error.authRequest.allowCredentials, undefined);
assert.strictEqual(signinResponse.status, 200);
assert.strictEqual(signinResponse.body.finished, false);
assert.strictEqual(signinResponse.body.next, 'passkey');
assert.notEqual(signinResponse.body.authRequest.challenge, undefined);
assert.notEqual(signinResponse.body.authRequest.allowCredentials, undefined);

const signinResponse2 = await api('signin', {
const signinResponse2 = await api('signin-flow', {
...signinWithSecurityKeyParam({
keyName,
credentialId,
requestOptions: signinResponseBody.error.authRequest,
requestOptions: signinResponse.body.authRequest,
} as any),
password: '',
});
assert.strictEqual(signinResponse2.status, 200);
assert.strictEqual(signinResponse2.body.finished, true);
assert.notEqual(signinResponse2.body.i, undefined);

// 後片付け
Expand Down Expand Up @@ -450,11 +437,12 @@ describe('2要素認証', () => {
assert.strictEqual(afterIResponse.status, 200);
assert.strictEqual(afterIResponse.body.securityKeys, false);

const signinResponse = await api('signin', {
const signinResponse = await api('signin-flow', {
...signinParam(),
token: otpToken(registerResponse.body.secret),
});
assert.strictEqual(signinResponse.status, 200);
assert.strictEqual(signinResponse.body.finished, true);
assert.notEqual(signinResponse.body.i, undefined);

// 後片付け
Expand Down Expand Up @@ -485,10 +473,11 @@ describe('2要素認証', () => {
}, alice);
assert.strictEqual(unregisterResponse.status, 204);

const signinResponse = await api('signin', {
const signinResponse = await api('signin-flow', {
...signinParam(),
});
assert.strictEqual(signinResponse.status, 200);
assert.strictEqual(signinResponse.body.finished, true);
assert.notEqual(signinResponse.body.i, undefined);

// 後片付け
Expand Down
8 changes: 4 additions & 4 deletions packages/backend/test/e2e/endpoints.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,9 +66,9 @@ describe('Endpoints', () => {
});
});

describe('signin', () => {
describe('signin-flow', () => {
test('間違ったパスワードでサインインできない', async () => {
const res = await api('signin', {
const res = await api('signin-flow', {
username: 'test1',
password: 'bar',
});
Expand All @@ -77,7 +77,7 @@ describe('Endpoints', () => {
});

test('クエリをインジェクションできない', async () => {
const res = await api('signin', {
const res = await api('signin-flow', {
username: 'test1',
// @ts-expect-error password must be string
password: {
Expand All @@ -89,7 +89,7 @@ describe('Endpoints', () => {
});

test('正しい情報でサインインできる', async () => {
const res = await api('signin', {
const res = await api('signin-flow', {
username: 'test1',
password: 'test1',
});
Expand Down
Loading

0 comments on commit ae3c155

Please sign in to comment.