Change PAT primary key from string to number (#2163)

* Update primary key

* Fix tests

src/lib/db/pat-store.ts

@@ -6,8 +6,8 @@ import NotFoundError from '../error/notfound-error';
 
 const TABLE = 'personal_access_tokens';
 
-const PAT_COLUMNS = [
-    'secret',
+const PAT_PUBLIC_COLUMNS = [
+    'id',
     'description',
     'user_id',
     'expires_at',
@@ -20,6 +20,7 @@ const fromRow = (row) => {
         throw new NotFoundError('No PAT found');
     }
     return new Pat({
+        id: row.id,
         secret: row.secret,
         userId: row.user_id,
         description: row.description,
@@ -51,8 +52,12 @@ export default class PatStore implements IPatStore {
         return fromRow(row[0]);
     }
 
-    async delete(secret: string): Promise<void> {
-        return this.db(TABLE).where({ secret: secret }).del();
+    async delete(id: number): Promise<void> {
+        return this.db(TABLE).where({ id: id }).del();
+    }
+
+    async deleteForUser(id: number, userId: number): Promise<void> {
+        return this.db(TABLE).where({ id: id, user_id: userId }).del();
     }
 
     async deleteAll(): Promise<void> {
@@ -61,28 +66,28 @@ export default class PatStore implements IPatStore {
 
     destroy(): void {}
 
-    async exists(secret: string): Promise<boolean> {
+    async exists(id: number): Promise<boolean> {
         const result = await this.db.raw(
-            `SELECT EXISTS(SELECT 1 FROM ${TABLE} WHERE secret = ?) AS present`,
-            [secret],
+            `SELECT EXISTS(SELECT 1 FROM ${TABLE} WHERE id = ?) AS present`,
+            [id],
         );
         const { present } = result.rows[0];
         return present;
     }
 
-    async get(secret: string): Promise<Pat> {
-        const row = await this.db(TABLE).where({ secret }).first();
+    async get(id: number): Promise<Pat> {
+        const row = await this.db(TABLE).where({ id }).first();
         return fromRow(row);
     }
 
     async getAll(): Promise<Pat[]> {
-        const groups = await this.db.select(PAT_COLUMNS).from(TABLE);
+        const groups = await this.db.select(PAT_PUBLIC_COLUMNS).from(TABLE);
         return groups.map(fromRow);
     }
 
     async getAllByUser(userId: number): Promise<Pat[]> {
         const groups = await this.db
-            .select(PAT_COLUMNS)
+            .select(PAT_PUBLIC_COLUMNS)
             .from(TABLE)
             .where('user_id', userId);
         return groups.map(fromRow);

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

@@ -4,6 +4,9 @@ export const patSchema = {
     $id: '#/components/schemas/patSchema',
     type: 'object',
     properties: {
+        id: {
+            type: 'number',
+        },
         secret: {
             type: 'string',
         },

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

@@ -62,7 +62,7 @@ export default class PatController extends Controller {
 
         this.route({
             method: 'delete',
-            path: '/:secret',
+            path: '/:id',
             acceptAnyContentType: true,
             handler: this.deletePat,
             permission: NONE,
@@ -95,11 +95,11 @@ export default class PatController extends Controller {
     }
 
     async deletePat(
-        req: IAuthRequest<{ secret: string }>,
+        req: IAuthRequest<{ id: number }>,
         res: Response,
     ): Promise<void> {
-        const { secret } = req.params;
-        await this.patService.deletePat(secret);
+        const { id } = req.params;
+        await this.patService.deletePat(id, req.user.id);
         res.status(200).end();
     }
 }

src/lib/services/pat-service.ts

@@ -37,6 +37,7 @@ export default class PatService {
         pat.userId = user.id;
         const newPat = await this.patStore.create(pat);
 
+        pat.secret = '***';
         await this.eventStore.store({
             type: PAT_CREATED,
             createdBy: user.email || user.username,
@@ -50,8 +51,8 @@ export default class PatService {
         return this.patStore.getAllByUser(user.id);
     }
 
-    async deletePat(secret: string): Promise<void> {
-        return this.patStore.delete(secret);
+    async deletePat(id: number, userId: number): Promise<void> {
+        return this.patStore.deleteForUser(id, userId);
     }
 
     private generateSecretKey() {

src/lib/types/models/pat.ts

@@ -1,4 +1,5 @@
 export interface IPat {
+    id: number;
     secret: string;
     description: string;
     userId: number;
@@ -8,6 +9,8 @@ export interface IPat {
 }
 
 export default class Pat implements IPat {
+    id: number;
+
     secret: string;
 
     description: string;
@@ -21,13 +24,15 @@ export default class Pat implements IPat {
     createdAt: Date;
 
     constructor({
-        secret,
+        id,
         userId,
         expiresAt,
         seenAt,
         createdAt,
+        secret,
         description,
     }: IPat) {
+        this.id = id;
         this.secret = secret;
         this.userId = userId;
         this.expiresAt = expiresAt;

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

@@ -1,7 +1,8 @@
 import { Store } from './store';
 import { IPat } from '../models/pat';
 
-export interface IPatStore extends Store<IPat, string> {
+export interface IPatStore extends Store<IPat, number> {
     create(group: IPat): Promise<IPat>;
     getAllByUser(userId: number): Promise<IPat[]>;
+    deleteForUser(id: number, userId: number): Promise<void>;
 }

src/migrations/20221010114644-pat-auto-increment.js

@@ -0,0 +1,28 @@
+'use strict';
+
+exports.up = function (db, cb) {
+    db.runSql(
+        `
+            alter table personal_access_tokens
+                drop constraint personal_access_tokens_pkey;
+            ALTER TABLE personal_access_tokens
+                add column id serial primary key;
+        `,
+        cb,
+    );
+};
+
+exports.down = function (db, cb) {
+    db.runSql(
+        `
+            alter table personal_access_tokens
+                drop constraint personal_access_tokens_pkey;
+            alter table personal_access_tokens
+                drop column id;
+            alter table personal_access_tokens
+                add primary key (secret);
+
+        `,
+        cb,
+    );
+};

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

@@ -8,6 +8,7 @@ let db: ITestDb;
 
 let tomorrow = new Date();
 let firstSecret;
+let firstId;
 tomorrow.setDate(tomorrow.getDate() + 1);
 
 beforeAll(async () => {
@@ -44,6 +45,7 @@ test('should create a PAT', async () => {
     expect(new Date(body.expiresAt)).toEqual(tomorrow);
     expect(body.description).toEqual(description);
     firstSecret = body.secret;
+    firstId = body.id;
 
     const response = await request
         .get('/api/admin/user/tokens')
@@ -64,9 +66,9 @@ test('should delete the PAT', async () => {
         .set('Content-Type', 'application/json')
         .expect(201);
 
-    const createdSecret = body.secret;
+    const createdId = body.id;
 
-    await request.delete(`/api/admin/user/tokens/${createdSecret}`).expect(200);
+    await request.delete(`/api/admin/user/tokens/${createdId}`).expect(200);
 });
 
 test('should get all PATs', async () => {
@@ -78,6 +80,36 @@ test('should get all PATs', async () => {
         .expect(200);
 
     expect(body.pats).toHaveLength(1);
+    expect(body.pats[0].secret).toBeUndefined();
+    expect(body.pats[0].id).toBeDefined();
+});
+
+test('should not allow deletion of other users PAT', async () => {
+    const { request } = app;
+
+    await app.request
+        .post(`/auth/demo/login`)
+        .send({
+            email: 'user-second@getunleash.io',
+        })
+        .expect(200);
+
+    await request.delete(`/api/admin/user/tokens/${firstId}`).expect(200);
+
+    await app.request
+        .post(`/auth/demo/login`)
+        .send({
+            email: 'user@getunleash.io',
+        })
+        .expect(200);
+
+    const { body } = await request
+        .get('/api/admin/user/tokens')
+        .expect('Content-Type', /json/)
+        .expect(200);
+
+    expect(body.pats).toHaveLength(1);
+    expect(body.pats[0].secret).toBeUndefined();
 });
 
 test('should get only current user PATs', async () => {

src/test/e2e/api/openapi/__snapshots__/openapi.e2e.test.ts.snap

@@ -1688,6 +1688,9 @@ exports[`should serve the OpenAPI spec 1`] = `
             "nullable": true,
             "type": "string",
           },
+          "id": {
+            "type": "number",
+          },
           "secret": {
             "type": "string",
           },
@@ -7051,13 +7054,13 @@ If the provided project does not exist, the list of events will be empty.",
         ],
       },
     },
-    "/api/admin/user/tokens/{secret}": {
+    "/api/admin/user/tokens/{id}": {
       "delete": {
         "operationId": "deletePat",
         "parameters": [
           {
             "in": "path",
-            "name": "secret",
+            "name": "id",
             "required": true,
             "schema": {
               "type": "string",

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

@@ -6,7 +6,7 @@ export default class FakePatStore implements IPatStore {
         throw new Error('Method not implemented.');
     }
 
-    delete(key: string): Promise<void> {
+    delete(key: number): Promise<void> {
         throw new Error('Method not implemented.');
     }
 
@@ -16,11 +16,11 @@ export default class FakePatStore implements IPatStore {
 
     destroy(): void {}
 
-    exists(key: string): Promise<boolean> {
+    exists(key: number): Promise<boolean> {
         throw new Error('Method not implemented.');
     }
 
-    get(key: string): Promise<IPat> {
+    get(key: number): Promise<IPat> {
         throw new Error('Method not implemented.');
     }
 
@@ -31,4 +31,8 @@ export default class FakePatStore implements IPatStore {
     getAllByUser(userId: number): Promise<IPat[]> {
         throw new Error('Method not implemented.');
     }
+
+    deleteForUser(id: number, userId: number): Promise<void> {
+        throw new Error('Method not implemented.');
+    }
 }