refactor: PATs (#6101)

https://linear.app/unleash/issue/SR-379/refactor-pats

This PR refactors PATs.

- Adds a new `createPatSchema`, which better aligns with
https://docs.getunleash.io/contributing/ADRs/overarching/separation-request-response-schemas
- Drops the model type and class in favor of using the schema types
directly, which is more consistent with the rest of the codebase and
easier to maintain
 - Misc scouting, improvement and fixes

This breaks Enterprise temporarily, but it's faster to move forward this
way.

src/lib/db/pat-store.ts

@@ -1,8 +1,8 @@
 import { Logger, LogProvider } from '../logger';
 import { IPatStore } from '../types/stores/pat-store';
-import Pat, { IPat } from '../types/models/pat';
 import NotFoundError from '../error/notfound-error';
 import { Db } from './db';
+import { CreatePatSchema, PatSchema } from '../openapi';
 
 const TABLE = 'personal_access_tokens';
 
@@ -15,26 +15,25 @@ const PAT_PUBLIC_COLUMNS = [
     'seen_at',
 ];
 
-const fromRow = (row) => {
-    if (!row) {
-        throw new NotFoundError('No PAT found');
-    }
-    return new Pat({
-        id: row.id,
-        secret: row.secret,
-        userId: row.user_id,
-        description: row.description,
-        createdAt: row.created_at,
-        seenAt: row.seen_at,
-        expiresAt: row.expires_at,
-    });
-};
+const rowToPat = ({
+    id,
+    description,
+    expires_at,
+    user_id,
+    created_at,
+    seen_at,
+}): PatSchema => ({
+    id,
+    description,
+    expiresAt: expires_at,
+    userId: user_id,
+    createdAt: created_at,
+    seenAt: seen_at,
+});
 
-const toRow = (pat: IPat) => ({
-    secret: pat.secret,
-    description: pat.description,
-    user_id: pat.userId,
-    expires_at: pat.expiresAt,
+const patToRow = ({ description, expiresAt }: CreatePatSchema) => ({
+    description,
+    expires_at: expiresAt,
 });
 
 export default class PatStore implements IPatStore {
@@ -47,9 +46,15 @@ export default class PatStore implements IPatStore {
         this.logger = getLogger('pat-store.ts');
     }
 
-    async create(token: IPat): Promise<IPat> {
-        const row = await this.db(TABLE).insert(toRow(token)).returning('*');
-        return fromRow(row[0]);
+    async create(
+        pat: CreatePatSchema,
+        secret: string,
+        userId: number,
+    ): Promise<PatSchema> {
+        const rows = await this.db(TABLE)
+            .insert({ ...patToRow(pat), secret, user_id: userId })
+            .returning('*');
+        return rowToPat(rows[0]);
     }
 
     async delete(id: number): Promise<void> {
@@ -96,21 +101,24 @@ export default class PatStore implements IPatStore {
         return count;
     }
 
-    async get(id: number): Promise<Pat> {
+    async get(id: number): Promise<PatSchema> {
         const row = await this.db(TABLE).where({ id }).first();
-        return fromRow(row);
+        if (!row) {
+            throw new NotFoundError('No PAT found.');
+        }
+        return rowToPat(row);
     }
 
-    async getAll(): Promise<Pat[]> {
-        const groups = await this.db.select(PAT_PUBLIC_COLUMNS).from(TABLE);
-        return groups.map(fromRow);
+    async getAll(): Promise<PatSchema[]> {
+        const pats = await this.db.select(PAT_PUBLIC_COLUMNS).from(TABLE);
+        return pats.map(rowToPat);
     }
 
-    async getAllByUser(userId: number): Promise<Pat[]> {
-        const groups = await this.db
+    async getAllByUser(userId: number): Promise<PatSchema[]> {
+        const pats = await this.db
             .select(PAT_PUBLIC_COLUMNS)
             .from(TABLE)
             .where('user_id', userId);
-        return groups.map(fromRow);
+        return pats.map(rowToPat);
     }
 }

src/lib/openapi/index.ts

@@ -79,6 +79,7 @@ import {
     passwordSchema,
     patchesSchema,
     patchSchema,
+    createPatSchema,
     patSchema,
     patsSchema,
     permissionSchema,
@@ -306,6 +307,7 @@ export const schemas: UnleashSchemas = {
     passwordSchema,
     patchesSchema,
     patchSchema,
+    createPatSchema,
     patSchema,
     patsSchema,
     permissionSchema,

src/lib/openapi/spec/create-pat-schema.ts

@@ -0,0 +1,25 @@
+import { FromSchema } from 'json-schema-to-ts';
+
+export const createPatSchema = {
+    $id: '#/components/schemas/createPatSchema',
+    description:
+        'Describes the properties required to create a [personal access token](https://docs.getunleash.io/how-to/how-to-create-personal-access-tokens), or PAT. PATs are automatically scoped to the authenticated user.',
+    type: 'object',
+    required: ['description', 'expiresAt'],
+    properties: {
+        description: {
+            type: 'string',
+            description: `The PAT's description.`,
+            example: 'user:xyzrandomstring',
+        },
+        expiresAt: {
+            type: 'string',
+            format: 'date-time',
+            description: `The PAT's expiration date.`,
+            example: '2023-04-19T08:15:14.000Z',
+        },
+    },
+    components: {},
+} as const;
+
+export type CreatePatSchema = FromSchema<typeof createPatSchema>;

src/lib/openapi/spec/index.ts

@@ -1,5 +1,6 @@
 export * from './id-schema';
 export * from './me-schema';
+export * from './create-pat-schema';
 export * from './pat-schema';
 export * from './tag-schema';
 export * from './date-schema';

src/lib/openapi/spec/pat-schema.ts

@@ -1,36 +1,30 @@
 import { FromSchema } from 'json-schema-to-ts';
+import { createPatSchema } from './create-pat-schema';
 
 export const patSchema = {
     $id: '#/components/schemas/patSchema',
     type: 'object',
     description:
-        'An overview of a [Personal Access Token](https://docs.getunleash.io/how-to/how-to-create-personal-access-tokens).',
+        'Describes a [personal access token](https://docs.getunleash.io/how-to/how-to-create-personal-access-tokens), or PAT. PATs are automatically scoped to the authenticated user.',
+    required: ['id', 'createdAt', ...createPatSchema.required],
     properties: {
         id: {
             type: 'integer',
-            description:
-                'The unique identification number for this Personal Access Token. (This property is set by Unleash when the token is created and cannot be set manually: if you provide a value when creating a PAT, Unleash will ignore it.)',
+            description: `The PAT's ID. PAT IDs are incrementing integers. In other words, a more recently created PAT will always have a higher ID than an older one.`,
             example: 1,
             minimum: 1,
         },
         secret: {
             type: 'string',
             description:
-                'The token used for authentication. (This property is set by Unleash when the token is created and cannot be set manually: if you provide a value when creating a PAT, Unleash will ignore it.)',
+                'The token used for authentication. It is automatically generated by Unleash when the PAT is created and that is the only time this property is returned.',
             example: 'user:xyzrandomstring',
         },
-        expiresAt: {
-            type: 'string',
-            format: 'date-time',
-            description: `The token's expiration date.`,
-            example: '2023-04-19T08:15:14.000Z',
-        },
         createdAt: {
             type: 'string',
             format: 'date-time',
             example: '2023-04-19T08:15:14.000Z',
-            description:
-                'When the token was created. (This property is set by Unleash when the token is created and cannot be set manually: if you provide a value when creating a PAT, Unleash will ignore it.)',
+            description: 'The date and time of when the PAT was created.',
         },
         seenAt: {
             type: 'string',
@@ -38,8 +32,14 @@ export const patSchema = {
             nullable: true,
             example: '2023-04-19T08:15:14.000Z',
             description:
-                'When the token was last seen/used to authenticate with. `null` if it has not been used yet. (This property is set by Unleash when the token is created and cannot be set manually: if you provide a value when creating a PAT, Unleash will ignore it.)',
+                'When the PAT was last seen/used to authenticate with. `null` if it has not been used yet.',
         },
+        userId: {
+            type: 'integer',
+            description: 'The ID of the user this PAT belongs to.',
+            example: 1337,
+        },
+        ...createPatSchema.properties,
     },
     components: {
         schemas: {},

src/lib/openapi/spec/pats-schema.ts

@@ -5,14 +5,14 @@ export const patsSchema = {
     $id: '#/components/schemas/patsSchema',
     type: 'object',
     description:
-        'Contains a collection of [Personal Access Tokens](https://docs.getunleash.io/how-to/how-to-create-personal-access-tokens).',
+        'Contains a collection of [personal access tokens](https://docs.getunleash.io/how-to/how-to-create-personal-access-tokens), or PATs. PATs are automatically scoped to the authenticated user.',
     properties: {
         pats: {
             type: 'array',
+            description: 'A collection of PATs.',
             items: {
-                $ref: '#/components/schemas/patSchema',
+                $ref: patSchema.$id,
             },
-            description: 'A collection of Personal Access Tokens',
         },
     },
     components: {

src/lib/routes/admin-api/user/pat.ts

@@ -19,8 +19,13 @@ import PatService from '../../../services/pat-service';
 import { NONE } from '../../../types/permissions';
 import { IAuthRequest } from '../../unleash-types';
 import { serializeDates } from '../../../types/serialize-dates';
-import { patSchema } from '../../../openapi/spec/pat-schema';
+import { PatSchema, patSchema } from '../../../openapi/spec/pat-schema';
 import { PatsSchema, patsSchema } from '../../../openapi/spec/pats-schema';
+import {
+    CreatePatSchema,
+    createPatSchema,
+} from '../../../openapi/spec/create-pat-schema';
+import { ForbiddenError, NotFoundError } from '../../../error';
 
 export default class PatController extends Controller {
     private patService: PatService;
@@ -53,11 +58,11 @@ export default class PatController extends Controller {
                     tags: ['Personal access tokens'],
                     operationId: 'getPats',
                     summary:
-                        'Get all Personal Access Tokens for the current user.',
+                        'Get all personal access tokens (PATs) for the current user.',
                     description:
-                        'Returns all of the [Personal Access Tokens](https://docs.getunleash.io/how-to/how-to-create-personal-access-tokens) belonging to the current user.',
+                        'Returns all of the [personal access tokens](https://docs.getunleash.io/how-to/how-to-create-personal-access-tokens) (PATs) belonging to the current user.',
                     responses: {
-                        200: createResponseSchema('patsSchema'),
+                        200: createResponseSchema(patsSchema.$id),
                         ...getStandardResponses(401, 403, 404),
                     },
                 }),
@@ -72,12 +77,13 @@ export default class PatController extends Controller {
                 openApiService.validPath({
                     tags: ['Personal access tokens'],
                     operationId: 'createPat',
-                    summary: 'Create a new Personal Access Token.',
+                    summary:
+                        'Create a new personal access token (PAT) for the current user.',
                     description:
-                        'Creates a new [Personal Access Token](https://docs.getunleash.io/how-to/how-to-create-personal-access-tokens) for the current user.',
-                    requestBody: createRequestSchema('patSchema'),
+                        'Creates a new [personal access token](https://docs.getunleash.io/how-to/how-to-create-personal-access-tokens) (PAT) belonging to the current user.',
+                    requestBody: createRequestSchema(createPatSchema.$id),
                     responses: {
-                        201: resourceCreatedResponseSchema('patSchema'),
+                        201: resourceCreatedResponseSchema(patSchema.$id),
                         ...getStandardResponses(401, 403, 404),
                     },
                 }),
@@ -94,9 +100,10 @@ export default class PatController extends Controller {
                 openApiService.validPath({
                     tags: ['Personal access tokens'],
                     operationId: 'deletePat',
-                    summary: 'Delete a Personal Access Token.',
+                    summary:
+                        'Delete a personal access token (PAT) for the current user.',
                     description:
-                        'This endpoint allows for deleting a [Personal Access Token](https://docs.getunleash.io/how-to/how-to-create-personal-access-tokens) belonging to the current user.',
+                        'Deletes a [personal access token](https://docs.getunleash.io/how-to/how-to-create-personal-access-tokens) (PAT) belonging to the current user.',
                     responses: {
                         200: emptyResponse,
                         ...getStandardResponses(401, 403, 404),
@@ -106,10 +113,16 @@ export default class PatController extends Controller {
         });
     }
 
-    async createPat(req: IAuthRequest, res: Response): Promise<void> {
+    async createPat(
+        req: IAuthRequest<unknown, unknown, CreatePatSchema>,
+        res: Response<PatSchema>,
+    ): Promise<void> {
         if (this.flagResolver.isEnabled('personalAccessTokensKillSwitch')) {
-            res.status(404).send({ message: 'PAT is disabled' });
-            return;
+            throw new NotFoundError('PATs are disabled.');
+        }
+
+        if (!req.user.id) {
+            throw new ForbiddenError('PATs require an authenticated user.');
         }
 
         const pat = req.body;
@@ -128,9 +141,13 @@ export default class PatController extends Controller {
 
     async getPats(req: IAuthRequest, res: Response<PatsSchema>): Promise<void> {
         if (this.flagResolver.isEnabled('personalAccessTokensKillSwitch')) {
-            res.status(404).send({ message: 'PAT is disabled' });
-            return;
+            throw new NotFoundError('PATs are disabled.');
         }
+
+        if (!req.user.id) {
+            throw new ForbiddenError('PATs require an authenticated user.');
+        }
+
         const pats = await this.patService.getAll(req.user.id);
         this.openApiService.respondWithValidation(200, res, patsSchema.$id, {
             pats: serializeDates(pats),

src/lib/services/pat-service.ts

@@ -2,7 +2,6 @@ import { IUnleashConfig, IUnleashStores } from '../types';
 import { Logger } from '../logger';
 import { IPatStore } from '../types/stores/pat-store';
 import { PAT_CREATED, PAT_DELETED } from '../types/events';
-import { IPat } from '../types/models/pat';
 import crypto from 'crypto';
 import { IUser } from '../types/user';
 import BadDataError from '../error/bad-data-error';
@@ -10,6 +9,7 @@ import NameExistsError from '../error/name-exists-error';
 import { OperationDeniedError } from '../error/operation-denied-error';
 import { PAT_LIMIT } from '../util/constants';
 import EventService from '../features/events/event-service';
+import { CreatePatSchema, PatSchema } from '../openapi';
 
 export default class PatService {
     private config: IUnleashConfig;
@@ -32,54 +32,50 @@ export default class PatService {
     }
 
     async createPat(
-        pat: IPat,
+        pat: CreatePatSchema,
         forUserId: number,
-        editor: IUser,
-    ): Promise<IPat> {
+        byUser: IUser,
+    ): Promise<PatSchema> {
         await this.validatePat(pat, forUserId);
-        pat.secret = this.generateSecretKey();
-        pat.userId = forUserId;
-        const newPat = await this.patStore.create(pat);
 
-        pat.secret = '***';
-        await this.eventService.storeEvent({
+        const secret = this.generateSecretKey();
+        const newPat = await this.patStore.create(pat, secret, forUserId);
+
+        await this.eventService.storeUserEvent({
             type: PAT_CREATED,
-            createdBy: editor.email || editor.username,
-            createdByUserId: editor.id,
-            data: pat,
+            byUser,
+            data: { ...pat, secret: '***' },
         });
 
-        return newPat;
+        return { ...newPat, secret };
     }
 
-    async getAll(userId: number): Promise<IPat[]> {
+    async getAll(userId: number): Promise<PatSchema[]> {
         return this.patStore.getAllByUser(userId);
     }
 
     async deletePat(
         id: number,
         forUserId: number,
-        editor: IUser,
+        byUser: IUser,
     ): Promise<void> {
         const pat = await this.patStore.get(id);
 
-        pat.secret = '***';
-        await this.eventService.storeEvent({
+        await this.eventService.storeUserEvent({
             type: PAT_DELETED,
-            createdBy: editor.email || editor.username,
-            createdByUserId: editor.id,
-            data: pat,
+            byUser,
+            data: { ...pat, secret: '***' },
         });
 
         return this.patStore.deleteForUser(id, forUserId);
     }
 
     async validatePat(
-        { description, expiresAt }: IPat,
+        { description, expiresAt }: CreatePatSchema,
         userId: number,
     ): Promise<void> {
         if (!description) {
-            throw new BadDataError('PAT description cannot be empty');
+            throw new BadDataError('PAT description cannot be empty.');
         }
 
         if (new Date(expiresAt) < new Date()) {
@@ -95,7 +91,7 @@ export default class PatService {
         if (
             await this.patStore.existsWithDescriptionByUser(description, userId)
         ) {
-            throw new NameExistsError('PAT description already exists');
+            throw new NameExistsError('PAT description already exists.');
         }
     }
 

src/lib/types/models/pat.ts

@@ -1,43 +0,0 @@
-export interface IPat {
-    id: number;
-    secret: string;
-    description: string;
-    userId: number;
-    expiresAt?: Date;
-    createdAt?: Date;
-    seenAt?: Date;
-}
-
-export default class Pat implements IPat {
-    id: number;
-
-    secret: string;
-
-    description: string;
-
-    userId: number;
-
-    expiresAt: Date;
-
-    seenAt: Date;
-
-    createdAt: Date;
-
-    constructor({
-        id,
-        userId,
-        expiresAt,
-        seenAt,
-        createdAt,
-        secret,
-        description,
-    }: IPat) {
-        this.id = id;
-        this.secret = secret;
-        this.userId = userId;
-        this.expiresAt = expiresAt;
-        this.seenAt = seenAt;
-        this.createdAt = createdAt;
-        this.description = description;
-    }
-}

src/lib/types/stores/pat-store.ts

@@ -1,9 +1,13 @@
 import { Store } from './store';
-import { IPat } from '../models/pat';
+import { CreatePatSchema, PatSchema } from '../../openapi';
 
-export interface IPatStore extends Store<IPat, number> {
-    create(group: IPat): Promise<IPat>;
-    getAllByUser(userId: number): Promise<IPat[]>;
+export interface IPatStore extends Store<PatSchema, number> {
+    create(
+        pat: CreatePatSchema,
+        secret: string,
+        userId: number,
+    ): Promise<PatSchema>;
+    getAllByUser(userId: number): Promise<PatSchema[]>;
     deleteForUser(id: number, userId: number): Promise<void>;
     existsWithDescriptionByUser(
         description: string,

src/test/e2e/api/admin/user/pat.e2e.test.ts

@@ -1,7 +1,6 @@
 import { IUnleashTest, setupAppWithAuth } from '../../../helpers/test-helper';
 import dbInit, { ITestDb } from '../../../helpers/database-init';
 import getLogger from '../../../../fixtures/no-logger';
-import { IPat } from '../../../../../lib/types/models/pat';
 import { IPatStore } from '../../../../../lib/types/stores/pat-store';
 import { PAT_LIMIT } from '../../../../../lib/util/constants';
 
@@ -43,7 +42,7 @@ test('should create a PAT', async () => {
         .send({
             expiresAt: tomorrow,
             description: description,
-        } as IPat)
+        })
         .set('Content-Type', 'application/json')
         .expect(201);
 
@@ -69,7 +68,7 @@ test('should delete the PAT', async () => {
         .send({
             description,
             expiresAt: tomorrow,
-        } as IPat)
+        })
         .set('Content-Type', 'application/json')
         .expect(201);
 
@@ -134,7 +133,7 @@ test('should get only current user PATs', async () => {
         .send({
             description: 'my pat',
             expiresAt: tomorrow,
-        } as IPat)
+        })
         .set('Content-Type', 'application/json')
         .expect(201);
 
@@ -156,7 +155,7 @@ test('should fail creation of PAT with passed expiry', async () => {
         .send({
             description: 'my expired pat',
             expiresAt: yesterday,
-        } as IPat)
+        })
         .set('Content-Type', 'application/json')
         .expect(400);
 });
@@ -166,7 +165,7 @@ test('should fail creation of PAT without a description', async () => {
         .post('/api/admin/user/tokens')
         .send({
             expiresAt: tomorrow,
-        } as IPat)
+        })
         .set('Content-Type', 'application/json')
         .expect(400);
 });
@@ -179,7 +178,7 @@ test('should fail creation of PAT with a description that already exists for the
         .send({
             description,
             expiresAt: tomorrow,
-        } as IPat)
+        })
         .set('Content-Type', 'application/json')
         .expect(201);
 
@@ -188,7 +187,7 @@ test('should fail creation of PAT with a description that already exists for the
         .send({
             description,
             expiresAt: tomorrow,
-        } as IPat)
+        })
         .set('Content-Type', 'application/json')
         .expect(409);
 });
@@ -201,7 +200,7 @@ test('should not fail creation of PAT when a description already exists for anot
         .send({
             description,
             expiresAt: tomorrow,
-        } as IPat)
+        })
         .set('Content-Type', 'application/json')
         .expect(201);
 
@@ -217,7 +216,7 @@ test('should not fail creation of PAT when a description already exists for anot
         .send({
             description,
             expiresAt: tomorrow,
-        } as IPat)
+        })
         .set('Content-Type', 'application/json')
         .expect(201);
 });
@@ -260,17 +259,21 @@ test('should not get user with invalid token', async () => {
 });
 
 test('should not get user with expired token', async () => {
-    const token = await patStore.create({
-        id: 1,
-        secret: 'user:expired-token',
-        description: 'expired-token',
-        userId: 1,
-        expiresAt: new Date('2020-01-01'),
-    });
+    const secret = 'user:expired-token';
+
+    await patStore.create(
+        {
+            id: 1,
+            description: 'expired-token',
+            expiresAt: '2020-01-01',
+        },
+        secret,
+        1,
+    );
 
     await app.request
         .get('/api/admin/user')
-        .set('Authorization', token.secret)
+        .set('Authorization', secret)
         .expect(401);
 });
 
@@ -290,7 +293,7 @@ test('should fail creation of PAT when PAT limit has been reached', async () =>
                 .send({
                     description: `my pat ${i}`,
                     expiresAt: tomorrow,
-                } as IPat)
+                })
                 .set('Content-Type', 'application/json')
                 .expect(201),
         );
@@ -302,7 +305,7 @@ test('should fail creation of PAT when PAT limit has been reached', async () =>
         .send({
             description: `my pat ${PAT_LIMIT}`,
             expiresAt: tomorrow,
-        } as IPat)
+        })
         .set('Content-Type', 'application/json')
         .expect(403);
 });

src/test/fixtures/fake-pat-store.ts

@@ -1,8 +1,12 @@
 import { IPatStore } from '../../lib/types/stores/pat-store';
-import { IPat } from '../../lib/types/models/pat';
+import { CreatePatSchema, PatSchema } from '../../lib/openapi';
 /* eslint-disable @typescript-eslint/no-unused-vars */
 export default class FakePatStore implements IPatStore {
-    create(group: IPat): Promise<IPat> {
+    create(
+        pat: CreatePatSchema,
+        secret: string,
+        userId: number,
+    ): Promise<PatSchema> {
         throw new Error('Method not implemented.');
     }
 
@@ -31,15 +35,15 @@ export default class FakePatStore implements IPatStore {
         throw new Error('Method not implemented.');
     }
 
-    get(key: number): Promise<IPat> {
+    get(key: number): Promise<PatSchema> {
         throw new Error('Method not implemented.');
     }
 
-    getAll(query?: Object): Promise<IPat[]> {
+    getAll(query?: Object): Promise<PatSchema[]> {
         throw new Error('Method not implemented.');
     }
 
-    getAllByUser(userId: number): Promise<IPat[]> {
+    getAllByUser(userId: number): Promise<PatSchema[]> {
         throw new Error('Method not implemented.');
     }