Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: signin の資格情報が足りないだけの場合はエラーにせず200を返すように #14700

Merged
merged 13 commits into from
Oct 5, 2024
2 changes: 1 addition & 1 deletion cypress/e2e/basic.cy.ts
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
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
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
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
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
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
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
Loading