From fb86b73a546af488828acb77c96f3305b6168de8 Mon Sep 17 00:00:00 2001 From: Roman Kalyakin Date: Thu, 11 Jul 2024 14:49:33 +0200 Subject: [PATCH] [chore] JWT Authentication review (#396) * fixes for the auth flow * add isStaff * added configuration flag controlling JWT strategy --- README.md | 16 +++ src/app.ts | 10 +- src/authentication.js | 60 --------- src/authentication.ts | 116 ++++++++++++++++++ src/configuration.ts | 31 ++++- src/hooks/authenticate.ts | 2 + src/middleware/openApiValidator.ts | 22 +++- .../authentication/authentication.schema.ts | 1 + 8 files changed, 189 insertions(+), 69 deletions(-) delete mode 100644 src/authentication.js create mode 100644 src/authentication.ts diff --git a/README.md b/README.md index 55ac333d..e9ecd7ee 100644 --- a/README.md +++ b/README.md @@ -198,6 +198,22 @@ When a schema is updated, the typescript types should be regenerated. This can b npm run generate-types ``` +## Configuration + +### Public API + +There are several configuration options that should be set differently in Public API: + + * `isPublicApi` - set to `true` to enable the public API. This configures openapi schema, validation, REST transport. + * `rateLimiter` - `enabled` must be set to `true` to enable rate limiting. + capacity and refill rate should be adjusted too. + * `authentication.jwtOptions`: + * `audience` - should be set to the public API URL. This must be different + from the internal API URL to make sure tokens from one could not be used + in another. + * `expiresIn` - should be set to a reasonable value for the public API (e.g. `8h` for 8 hours) + * `authentication.cookie.enabled` set to `false` - cookies are not used in the public API + ## Help For more information on all the things you can do with Feathers visit [docs.feathersjs.com](http://docs.feathersjs.com). diff --git a/src/app.ts b/src/app.ts index 6615a9d2..2e04b1d1 100644 --- a/src/app.ts +++ b/src/app.ts @@ -17,6 +17,8 @@ import { ensureServiceIsFeathersCompatible } from './util/feathers' import channels from './channels' import { ImpressoApplication } from './types' import { Application } from '@feathersjs/express' +import bodyParser from 'body-parser' +import authentication from './authentication' const path = require('path') const compress = require('compression') @@ -29,8 +31,6 @@ const middleware = require('./middleware') // const services = require('./services'); const appHooks = require('./app.hooks') -const authentication = require('./authentication') - const multer = require('./multer') const cache = require('./cache') const cachedSolr = require('./cachedSolr') @@ -62,6 +62,8 @@ app.use('cachedSolr', ensureServiceIsFeathersCompatible(cachedSolr(app)), { app.use(helmet()) app.use(compress()) app.use(cookieParser()) +// needed to access body in non-feathers middlewares, like openapi validator +app.use(bodyParser.json({ limit: '50mb' })) // configure local multer service. app.configure(multer) @@ -80,8 +82,6 @@ app.configure(media) app.configure(proxy) app.configure(schemas) -app.configure(errorHandling) - // Enable Swagger and API validator if needed app.configure(swagger) app.configure(openApiValidator) @@ -105,4 +105,6 @@ app.configure(appHooks) // because one of the services is used in the channels. app.configure(channels) +app.configure(errorHandling) + module.exports = app diff --git a/src/authentication.js b/src/authentication.js deleted file mode 100644 index e0689289..00000000 --- a/src/authentication.js +++ /dev/null @@ -1,60 +0,0 @@ -import { createSwaggerServiceOptions } from 'feathers-swagger' -import { docs } from './services/authentication/authentication.schema' - -const debug = require('debug')('impresso/authentication') - -const { AuthenticationService, JWTStrategy } = require('@feathersjs/authentication') -const { LocalStrategy } = require('@feathersjs/authentication-local') -const { NotAuthenticated } = require('@feathersjs/errors') -const User = require('./models/users.model') - -class CustomisedAuthenticationService extends AuthenticationService { - async getPayload(authResult, params) { - const payload = await super.getPayload(authResult, params) - const { user } = authResult - if (user) { - payload.userId = user.uid - if (user.groups.length) { - payload.userGroups = user.groups.map(d => d.name) - } - } - return payload - } -} - -class HashedPasswordVerifier extends LocalStrategy { - comparePassword(user, password) { - return new Promise((resolve, reject) => { - if (!(user instanceof User)) { - debug('_comparePassword: user is not valid', user) - return reject(new NotAuthenticated('Login incorrect')) - } - - const isValid = User.comparePassword({ - encrypted: user.password, - password, - }) - - if (!isValid) { - return reject(new NotAuthenticated('Login incorrect')) - } - return resolve({ - ...user, - }) - }) - } -} - -module.exports = app => { - const isPublicApi = app.get('isPublicApi') - const authentication = new CustomisedAuthenticationService(app) - - authentication.register('jwt', new JWTStrategy()) - authentication.register('local', new HashedPasswordVerifier()) - - app.use('/authentication', authentication, { - methods: isPublicApi ? ['create'] : undefined, - events: [], - docs: createSwaggerServiceOptions({ schemas: {}, docs }), - }) -} diff --git a/src/authentication.ts b/src/authentication.ts new file mode 100644 index 00000000..075afef9 --- /dev/null +++ b/src/authentication.ts @@ -0,0 +1,116 @@ +import { + AuthenticationParams, + AuthenticationRequest, + AuthenticationResult, + AuthenticationService, + JWTStrategy, +} from '@feathersjs/authentication' +import { LocalStrategy } from '@feathersjs/authentication-local' +import { NotAuthenticated } from '@feathersjs/errors' +import initDebug from 'debug' +import { createSwaggerServiceOptions } from 'feathers-swagger' +import User from './models/users.model' +import { docs } from './services/authentication/authentication.schema' +import { ImpressoApplication } from './types' +import { ServiceOptions } from '@feathersjs/feathers' + +const debug = initDebug('impresso/authentication') + +class CustomisedAuthenticationService extends AuthenticationService { + async getPayload(authResult: AuthenticationResult, params: AuthenticationParams) { + const payload = await super.getPayload(authResult, params) + const { user } = authResult as { user: User } + if (user) { + payload.userId = user.uid + if (user.groups.length) { + payload.userGroups = user.groups.map(d => d.name) + } + payload.isStaff = user.isStaff + } + return payload + } +} + +class HashedPasswordVerifier extends LocalStrategy { + comparePassword(user: User, password: string) { + return new Promise((resolve, reject) => { + if (!(user instanceof User)) { + debug('_comparePassword: user is not valid', user) + return reject(new NotAuthenticated('Login incorrect')) + } + + const isValid = User.comparePassword({ + encrypted: user.password, + password, + }) + + if (!isValid) { + return reject(new NotAuthenticated('Login incorrect')) + } + return resolve({ + ...user, + }) + }) + } +} + +export interface SlimUser { + uid: string + id: number + isStaff: boolean +} + +/** + * A custom JWT strategy that does not load the user from the database. + * Instead, it uses the payload from the JWT token to create a slim user object + * which is enough for most of the use cases across the codebase. + * Where a full user object is required, it is requested explicitly. + */ +class NoDBJWTStrategy extends JWTStrategy { + async authenticate(authentication: AuthenticationRequest, params: AuthenticationParams) { + const { accessToken } = authentication + const { entity } = this.configuration + if (!accessToken) { + throw new NotAuthenticated('No access token') + } + const payload = await this.authentication?.verifyAccessToken(accessToken, params.jwt) + const result = { + accessToken, + authentication: { + strategy: 'jwt', + accessToken, + payload, + }, + } + if (entity === null) { + return result + } + + const slimUser: SlimUser = { + uid: payload.userId, + id: parseInt(payload.sub), + isStaff: payload.isStaff ?? false, + } + return { + ...result, + [entity]: slimUser, + } + } +} + +export default (app: ImpressoApplication) => { + const isPublicApi = app.get('isPublicApi') + const useDbUserInRequestContext = app.get('useDbUserInRequestContext') + const authentication = new CustomisedAuthenticationService(app) + + const jwtStrategy = useDbUserInRequestContext ? new JWTStrategy() : new NoDBJWTStrategy() + + authentication.register('jwt', jwtStrategy) + authentication.register('local', new HashedPasswordVerifier()) + + app.use('/authentication', authentication, { + methods: isPublicApi ? ['create'] : undefined, + events: [], + docs: createSwaggerServiceOptions({ schemas: {}, docs }), + } as ServiceOptions) +} diff --git a/src/configuration.ts b/src/configuration.ts index 0b13d009..aaaa4d92 100644 --- a/src/configuration.ts +++ b/src/configuration.ts @@ -27,6 +27,7 @@ export interface Configuration { redis?: RedisConfiguration rateLimiter?: RateLimiterConfiguration & { enabled?: boolean } publicApiPrefix?: string + useDbUserInRequestContext?: boolean // TODO: move to services: sequelizeClient?: Sequelize @@ -39,10 +40,7 @@ const configurationSchema: JSONSchemaDefinition = { $id: 'configuration', type: 'object', properties: { - isPublicApi: { - type: 'boolean', - description: 'If `true`, the app serves a public API', - }, + isPublicApi: { type: 'boolean', description: 'If `true`, the app serves a public API' }, allowedCorsOrigins: { type: 'array', items: { @@ -50,6 +48,31 @@ const configurationSchema: JSONSchemaDefinition = { }, description: 'List of allowed origins for CORS', }, + redis: { + type: 'object', + properties: { + enable: { type: 'boolean', description: 'Enable Redis' }, + brokerUrl: { type: 'string', description: 'URL of the Redis broker' }, + }, + description: 'Redis configuration', + }, + rateLimiter: { + type: 'object', + properties: { + enabled: { type: 'boolean', description: 'Enable rate limiter' }, + capacity: { type: 'number', description: 'Capacity of the rate limiter' }, + refillRate: { type: 'number', description: 'Refill rate of the rate limiter' }, + }, + description: 'Rate limiter configuration', + required: ['capacity', 'refillRate'], + }, + publicApiPrefix: { type: 'string', description: 'Prefix for the public API' }, + useDbUserInRequestContext: { + type: 'boolean', + description: + 'If `true`, the user object is loaded from the db on every request. ' + + 'If `false` (default), the user object is created from the JWT token', + }, }, } as const diff --git a/src/hooks/authenticate.ts b/src/hooks/authenticate.ts index 4b150f61..7e3067bd 100644 --- a/src/hooks/authenticate.ts +++ b/src/hooks/authenticate.ts @@ -43,6 +43,8 @@ interface AuthenticateAroundOptions { export const authenticateAround = ({ strategy = 'jwt', allowUnauthenticated = false }: AuthenticateAroundOptions = {}) => async (context: HookContext, next: NextFunction) => { + if (context.type !== 'around') throw new Error('authenticateAround must be used in "around" hooks') + const isPublicApi = context.app.get('isPublicApi') // only allow unauthenticated in non-public API const doAllowUnauthenticated = isPublicApi ? false : allowUnauthenticated diff --git a/src/middleware/openApiValidator.ts b/src/middleware/openApiValidator.ts index fd81d0a9..edeb4dd8 100644 --- a/src/middleware/openApiValidator.ts +++ b/src/middleware/openApiValidator.ts @@ -3,11 +3,18 @@ import type { Application } from '@feathersjs/express' import { HookContext, NextFunction } from '@feathersjs/hooks' import convertSchema from '@openapi-contrib/json-schema-to-openapi-schema' import * as OpenApiValidator from 'express-openapi-validator' +import { HttpError as OpenApiHttpError } from 'express-openapi-validator/dist/framework/types' import type { OpenAPIV3, OpenApiValidatorOpts, ValidationError } from 'express-openapi-validator/dist/framework/types' import fs from 'fs' import { logger } from '../logger' import type { ImpressoApplication } from '../types' import { parseFilters } from '../util/queryParameters' +import type { + NextFunction as ExpressNextFunction, + Request as ExpressRequest, + Response as ExpressResponse, +} from 'express' +import { FeathersError } from '@feathersjs/errors' export default (app: ImpressoApplication & Application) => { installMiddleware(app) @@ -66,7 +73,6 @@ const installMiddleware = (app: ImpressoApplication & Application) => { next() }) - // app.use(middlewares as any) // app.use(middlewares as any) middlewares.forEach((middleware, index) => { logger.debug('Install', middleware) @@ -75,6 +81,20 @@ const installMiddleware = (app: ImpressoApplication & Application) => { handler(req, res, next) }) }) + + app.use((error: any, req: ExpressRequest, res: ExpressResponse, next: ExpressNextFunction) => { + if (error instanceof OpenApiHttpError) { + next(convertOpenApiError(error)) + } else { + next(error) + } + }) +} + +const convertOpenApiError = (error: OpenApiHttpError): FeathersError => { + const newError = new FeathersError(error, 'OpenApiError', error.status, error.constructor.name, {}) + newError.stack = error.stack + return newError } /** diff --git a/src/services/authentication/authentication.schema.ts b/src/services/authentication/authentication.schema.ts index a45a6218..8a6b7786 100644 --- a/src/services/authentication/authentication.schema.ts +++ b/src/services/authentication/authentication.schema.ts @@ -17,6 +17,7 @@ export const docs: ServiceSwaggerOptions = { requestBody: { content: getRequestBodyContent('AuthenticationCreateRequest'), }, + security: [], responses: { 201: { description: 'Authentication successful',