From bd942342b0c7cc2fd9808dd3e66bdd7dd4f0c84e Mon Sep 17 00:00:00 2001 From: YANG QIA <2013xile@gmail.com> Date: Fri, 6 Sep 2024 14:43:14 +0800 Subject: [PATCH] fix(auth): should set user token as invalid when changing password (#5212) * fix(auth): should log user out when changing password * fix: add passwordChangeTZ * fix: clear local token when token is invalid * fix: test * fix: field name --- .../auth/src/__tests__/middleware.test.ts | 2 +- packages/core/auth/src/auth-manager.ts | 5 +++- packages/core/auth/src/base/auth.ts | 11 +++++-- .../core/client/src/api-client/APIClient.ts | 5 +++- .../core/client/src/user/ChangePassword.tsx | 3 ++ packages/core/logger/src/request-logger.ts | 11 +++++-- packages/core/server/src/locale/locale.ts | 1 + packages/core/test/src/server/mock-server.ts | 1 + .../@nocobase/plugin-acl/src/server/server.ts | 2 +- .../src/server/__tests__/actions.test.ts | 24 +++++++++++++++ .../plugin-auth/src/server/plugin.ts | 12 ++------ .../src/server/collections/users.ts | 4 +++ .../plugin-users/src/server/server.ts | 29 ++++++++----------- 13 files changed, 74 insertions(+), 36 deletions(-) diff --git a/packages/core/auth/src/__tests__/middleware.test.ts b/packages/core/auth/src/__tests__/middleware.test.ts index 3f7333f07c..0f0bd09d50 100644 --- a/packages/core/auth/src/__tests__/middleware.test.ts +++ b/packages/core/auth/src/__tests__/middleware.test.ts @@ -71,7 +71,7 @@ describe('middleware', () => { hasFn.mockImplementation(() => true); const res = await agent.resource('auth').check(); expect(res.status).toBe(401); - expect(res.text).toContain('token is not available'); + expect(res.text).toContain('Token is invalid'); }); }); }); diff --git a/packages/core/auth/src/auth-manager.ts b/packages/core/auth/src/auth-manager.ts index f7bd79d899..f0c625a1e5 100644 --- a/packages/core/auth/src/auth-manager.ts +++ b/packages/core/auth/src/auth-manager.ts @@ -109,7 +109,10 @@ export class AuthManager { return async (ctx: Context & { auth: Auth }, next: Next) => { const token = ctx.getBearerToken(); if (token && (await ctx.app.authManager.jwt.blacklist?.has(token))) { - return ctx.throw(401, ctx.t('token is not available')); + return ctx.throw(401, { + code: 'TOKEN_INVALID', + message: ctx.t('Token is invalid'), + }); } const name = ctx.get(this.options.authKey) || this.options.default; diff --git a/packages/core/auth/src/base/auth.ts b/packages/core/auth/src/base/auth.ts index 4c8659f63c..bab49103fe 100644 --- a/packages/core/auth/src/base/auth.ts +++ b/packages/core/auth/src/base/auth.ts @@ -69,14 +69,14 @@ export class BaseAuth extends Auth { return null; } try { - const { userId, roleName } = await this.jwt.decode(token); + const { userId, roleName, iat, temp } = await this.jwt.decode(token); if (roleName) { this.ctx.headers['x-role'] = roleName; } const cache = this.ctx.cache as Cache; - return await cache.wrap(this.getCacheKey(userId), () => + const user = await cache.wrap(this.getCacheKey(userId), () => this.userRepository.findOne({ filter: { id: userId, @@ -84,6 +84,10 @@ export class BaseAuth extends Auth { raw: true, }), ); + if (temp && user.passwordChangeTz && iat * 1000 < user.passwordChangeTz) { + throw new Error('Token is invalid'); + } + return user; } catch (err) { this.ctx.logger.error(err, { method: 'check' }); return null; @@ -106,6 +110,7 @@ export class BaseAuth extends Auth { } const token = this.jwt.sign({ userId: user.id, + temp: true, }); return { user, @@ -119,7 +124,7 @@ export class BaseAuth extends Auth { return; } const { userId } = await this.jwt.decode(token); - await this.ctx.app.emitAsync('beforeSignOut', { userId }); + await this.ctx.app.emitAsync('cache:del:roles', { userId }); await this.ctx.cache.del(this.getCacheKey(userId)); return await this.jwt.block(token); } diff --git a/packages/core/client/src/api-client/APIClient.ts b/packages/core/client/src/api-client/APIClient.ts index 5c6993aa3f..23d11e64f5 100644 --- a/packages/core/client/src/api-client/APIClient.ts +++ b/packages/core/client/src/api-client/APIClient.ts @@ -98,6 +98,9 @@ export class APIClient extends APIClientSDK { if (errs.find((error: { code?: string }) => error.code === 'ROLE_NOT_FOUND_ERR')) { this.auth.setRole(null); } + if (errs.find((error: { code?: string }) => error.code === 'TOKEN_INVALID')) { + this.auth.setToken(null); + } throw error; }, ); @@ -119,7 +122,7 @@ export class APIClient extends APIClientSDK { } return response; }, - (error) => { + async (error) => { if (this.silence) { throw error; } diff --git a/packages/core/client/src/user/ChangePassword.tsx b/packages/core/client/src/user/ChangePassword.tsx index 226d9d9eb2..c6837ff799 100644 --- a/packages/core/client/src/user/ChangePassword.tsx +++ b/packages/core/client/src/user/ChangePassword.tsx @@ -14,6 +14,7 @@ import React, { useContext, useMemo, useState } from 'react'; import { useTranslation } from 'react-i18next'; import { ActionContextProvider, DropdownVisibleContext, SchemaComponent, useActionContext } from '../'; import { useAPIClient } from '../api-client'; +import { useNavigate } from 'react-router-dom'; const useCloseAction = () => { const { setVisible } = useActionContext(); @@ -29,6 +30,7 @@ const useCloseAction = () => { }; const useSaveCurrentUserValues = () => { + const navigate = useNavigate(); const { setVisible } = useActionContext(); const form = useForm(); const api = useAPIClient(); @@ -40,6 +42,7 @@ const useSaveCurrentUserValues = () => { }); await form.reset(); setVisible(false); + navigate('/signin'); }, }; }; diff --git a/packages/core/logger/src/request-logger.ts b/packages/core/logger/src/request-logger.ts index 8875f4c53e..fc6e78a401 100644 --- a/packages/core/logger/src/request-logger.ts +++ b/packages/core/logger/src/request-logger.ts @@ -7,9 +7,8 @@ * For more information, please refer to: https://www.nocobase.com/agreement. */ -import { getLoggerFilePath } from './config'; import { Logger, LoggerOptions } from './logger'; -import { pick } from 'lodash'; +import { pick, omit } from 'lodash'; const defaultRequestWhitelist = [ 'action', 'header.x-role', @@ -21,6 +20,12 @@ const defaultRequestWhitelist = [ 'referer', ]; const defaultResponseWhitelist = ['status']; +const defaultActionBlackList = [ + 'params.values.password', + 'params.values.confirmPassword', + 'params.values.oldPassword', + 'params.values.newPassword', +]; export interface RequestLoggerOptions extends LoggerOptions { skip?: (ctx?: any) => Promise; @@ -60,7 +65,7 @@ export const requestLogger = (appName: string, requestLogger: Logger, options?: message: `response ${ctx.url}`, ...requestInfo, res: pick(ctx.response.toJSON(), options?.responseWhitelist || defaultResponseWhitelist), - action: ctx.action?.toJSON?.(), + action: omit(ctx.action?.toJSON?.(), defaultActionBlackList), userId: ctx.auth?.user?.id, status: ctx.status, cost, diff --git a/packages/core/server/src/locale/locale.ts b/packages/core/server/src/locale/locale.ts index f04a5ecad3..4ec017eb00 100644 --- a/packages/core/server/src/locale/locale.ts +++ b/packages/core/server/src/locale/locale.ts @@ -45,6 +45,7 @@ export class Locale { name: 'locale', prefix: 'locale', store: 'memory', + max: 2000 }); await this.get(this.defaultLang); diff --git a/packages/core/test/src/server/mock-server.ts b/packages/core/test/src/server/mock-server.ts index 91dec44647..9f58cd5b45 100644 --- a/packages/core/test/src/server/mock-server.ts +++ b/packages/core/test/src/server/mock-server.ts @@ -123,6 +123,7 @@ export class MockServer extends Application { jwt.sign( { userId: typeof userOrId === 'number' ? userOrId : userOrId?.id, + temp: true, }, process.env.APP_KEY, { diff --git a/packages/plugins/@nocobase/plugin-acl/src/server/server.ts b/packages/plugins/@nocobase/plugin-acl/src/server/server.ts index 37936e0a9e..73fcfe9302 100644 --- a/packages/plugins/@nocobase/plugin-acl/src/server/server.ts +++ b/packages/plugins/@nocobase/plugin-acl/src/server/server.ts @@ -408,7 +408,7 @@ export class PluginACLServer extends Plugin { }); }); - this.app.on('beforeSignOut', ({ userId }) => { + this.app.on('cache:del:roles', ({ userId }) => { this.app.cache.del(`roles:${userId}`); }); this.app.resourcer.use(setCurrentRole, { tag: 'setCurrentRole', before: 'acl', after: 'auth' }); diff --git a/packages/plugins/@nocobase/plugin-auth/src/server/__tests__/actions.test.ts b/packages/plugins/@nocobase/plugin-auth/src/server/__tests__/actions.test.ts index c5f87ca261..27f3a96b25 100644 --- a/packages/plugins/@nocobase/plugin-auth/src/server/__tests__/actions.test.ts +++ b/packages/plugins/@nocobase/plugin-auth/src/server/__tests__/actions.test.ts @@ -102,6 +102,7 @@ describe('actions', () => { }); afterEach(async () => { + await app.db.clean({ drop: true }); await app.destroy(); }); @@ -273,5 +274,28 @@ describe('actions', () => { expect(res.statusCode).toEqual(400); expect(res.error.text).toBe('Please enter a password'); }); + + it('should sign user out when changing password', async () => { + const userRepo = db.getRepository('users'); + const user = await userRepo.create({ + values: { + username: 'test', + password: '12345', + }, + }); + const userAgent = await agent.login(user); + const res = await userAgent.post('/auth:check').set({ 'X-Authenticator': 'basic' }).send(); + expect(res.statusCode).toEqual(200); + expect(res.body.data.id).toBeDefined(); + const res2 = await userAgent.post('/auth:changePassword').set({ 'X-Authenticator': 'basic' }).send({ + oldPassword: '12345', + newPassword: '123456', + confirmPassword: '123456', + }); + expect(res2.statusCode).toEqual(200); + const res3 = await userAgent.post('/auth:check').set({ 'X-Authenticator': 'basic' }).send(); + expect(res3.statusCode).toEqual(200); + expect(res3.body.data.id).toBeUndefined(); + }); }); }); diff --git a/packages/plugins/@nocobase/plugin-auth/src/server/plugin.ts b/packages/plugins/@nocobase/plugin-auth/src/server/plugin.ts index 730d43b736..98e7c66175 100644 --- a/packages/plugins/@nocobase/plugin-auth/src/server/plugin.ts +++ b/packages/plugins/@nocobase/plugin-auth/src/server/plugin.ts @@ -29,15 +29,6 @@ export class PluginAuthServer extends Plugin { } async load() { - // Set up database - await this.importCollections(resolve(__dirname, 'collections')); - this.db.addMigrations({ - namespace: 'auth', - directory: resolve(__dirname, 'migrations'), - context: { - plugin: this, - }, - }); this.cache = await this.app.cacheManager.createCache({ name: 'auth', prefix: 'auth', @@ -85,6 +76,9 @@ export class PluginAuthServer extends Plugin { const cache = this.app.cache as Cache; await cache.del(`auth:${user.id}`); }); + this.app.on('cache:del:auth', async ({ userId }) => { + await this.cache.del(`auth:${userId}`); + }); } async install(options?: InstallOptions) { diff --git a/packages/plugins/@nocobase/plugin-users/src/server/collections/users.ts b/packages/plugins/@nocobase/plugin-users/src/server/collections/users.ts index 5eccf34a0c..fdfaf654cf 100644 --- a/packages/plugins/@nocobase/plugin-users/src/server/collections/users.ts +++ b/packages/plugins/@nocobase/plugin-users/src/server/collections/users.ts @@ -91,6 +91,10 @@ export default defineCollection({ 'x-component': 'Password', }, }, + { + name: 'passwordChangeTz', + type: 'bigInt', + }, { type: 'string', name: 'appLang', diff --git a/packages/plugins/@nocobase/plugin-users/src/server/server.ts b/packages/plugins/@nocobase/plugin-users/src/server/server.ts index 1034775186..b1c00d914b 100644 --- a/packages/plugins/@nocobase/plugin-users/src/server/server.ts +++ b/packages/plugins/@nocobase/plugin-users/src/server/server.ts @@ -7,11 +7,9 @@ * For more information, please refer to: https://www.nocobase.com/agreement. */ -import { Collection, Op } from '@nocobase/database'; +import { Collection, Model, Op } from '@nocobase/database'; import { Plugin } from '@nocobase/server'; import { parse } from '@nocobase/utils'; -import { resolve } from 'path'; -import { Cache } from '@nocobase/cache'; import * as actions from './actions/users'; import { UserModel } from './models/UserModel'; import PluginUserDataSyncServer from '@nocobase/plugin-user-data-sync'; @@ -155,19 +153,9 @@ export default class PluginUsersServer extends Plugin { } async load() { - await this.importCollections(resolve(__dirname, 'collections')); - this.db.addMigrations({ - namespace: 'users', - directory: resolve(__dirname, 'migrations'), - context: { - plugin: this, - }, - }); - - this.app.resourcer.use(async (ctx, next) => { + this.app.resourceManager.use(async (ctx, next) => { await next(); const { associatedName, resourceName, actionName, values } = ctx.action.params; - const cache = ctx.app.cache as Cache; if ( associatedName === 'roles' && resourceName === 'users' && @@ -175,9 +163,7 @@ export default class PluginUsersServer extends Plugin { values?.length ) { // Delete cache when the members of a role changed - for (const userId of values) { - await cache.del(`roles:${userId}`); - } + await Promise.all(values.map((userId: number) => this.app.emitAsync('cache:del:roles', { userId }))); } }); @@ -185,6 +171,15 @@ export default class PluginUsersServer extends Plugin { if (userDataSyncPlugin && userDataSyncPlugin.enabled) { userDataSyncPlugin.resourceManager.registerResource(new UserDataSyncResource(this.db, this.app.logger)); } + + this.app.db.on('users.beforeUpdate', async (model: Model) => { + if (!model._changed.has('password')) { + return; + } + model.set('passwordChangeTz', Date.now()); + await this.app.emitAsync('cache:del:roles', { userId: model.get('id') }); + await this.app.emitAsync('cache:del:auth', { userId: model.get('id') }); + }); } getInstallingData(options: any = {}) {