diff --git a/src/lib/db/pat-store.ts b/src/lib/db/pat-store.ts index c5a0909897..b7b0db88dd 100644 --- a/src/lib/db/pat-store.ts +++ b/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 { - return this.db(TABLE).where({ secret: secret }).del(); + async delete(id: number): Promise { + return this.db(TABLE).where({ id: id }).del(); + } + + async deleteForUser(id: number, userId: number): Promise { + return this.db(TABLE).where({ id: id, user_id: userId }).del(); } async deleteAll(): Promise { @@ -61,28 +66,28 @@ export default class PatStore implements IPatStore { destroy(): void {} - async exists(secret: string): Promise { + async exists(id: number): Promise { 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 { - const row = await this.db(TABLE).where({ secret }).first(); + async get(id: number): Promise { + const row = await this.db(TABLE).where({ id }).first(); return fromRow(row); } async getAll(): Promise { - 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 { const groups = await this.db - .select(PAT_COLUMNS) + .select(PAT_PUBLIC_COLUMNS) .from(TABLE) .where('user_id', userId); return groups.map(fromRow); diff --git a/src/lib/openapi/spec/pat-schema.ts b/src/lib/openapi/spec/pat-schema.ts index bcd8a2b141..9e7401ead6 100644 --- a/src/lib/openapi/spec/pat-schema.ts +++ b/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', }, diff --git a/src/lib/routes/admin-api/user/pat.ts b/src/lib/routes/admin-api/user/pat.ts index c255fb63c8..aebd1d18b2 100644 --- a/src/lib/routes/admin-api/user/pat.ts +++ b/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 { - 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(); } } diff --git a/src/lib/services/pat-service.ts b/src/lib/services/pat-service.ts index 6f8a316cae..c27f67089b 100644 --- a/src/lib/services/pat-service.ts +++ b/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 { - return this.patStore.delete(secret); + async deletePat(id: number, userId: number): Promise { + return this.patStore.deleteForUser(id, userId); } private generateSecretKey() { diff --git a/src/lib/types/models/pat.ts b/src/lib/types/models/pat.ts index cd6b93b390..0cba11bf76 100644 --- a/src/lib/types/models/pat.ts +++ b/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; diff --git a/src/lib/types/stores/pat-store.ts b/src/lib/types/stores/pat-store.ts index a4cf48b7b0..66c72d3237 100644 --- a/src/lib/types/stores/pat-store.ts +++ b/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 { +export interface IPatStore extends Store { create(group: IPat): Promise; getAllByUser(userId: number): Promise; + deleteForUser(id: number, userId: number): Promise; } diff --git a/src/migrations/20221010114644-pat-auto-increment.js b/src/migrations/20221010114644-pat-auto-increment.js new file mode 100644 index 0000000000..217350b7b4 --- /dev/null +++ b/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, + ); +}; diff --git a/src/test/e2e/api/admin/user/pat.e2e.test.ts b/src/test/e2e/api/admin/user/pat.e2e.test.ts index 9527501fcf..d6a4feb7b8 100644 --- a/src/test/e2e/api/admin/user/pat.e2e.test.ts +++ b/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 () => { diff --git a/src/test/e2e/api/openapi/__snapshots__/openapi.e2e.test.ts.snap b/src/test/e2e/api/openapi/__snapshots__/openapi.e2e.test.ts.snap index d47e435f29..cc22c16530 100644 --- a/src/test/e2e/api/openapi/__snapshots__/openapi.e2e.test.ts.snap +++ b/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", diff --git a/src/test/fixtures/fake-pat-store.ts b/src/test/fixtures/fake-pat-store.ts index 73280995e0..8b7cf1e6d3 100644 --- a/src/test/fixtures/fake-pat-store.ts +++ b/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 { + delete(key: number): Promise { throw new Error('Method not implemented.'); } @@ -16,11 +16,11 @@ export default class FakePatStore implements IPatStore { destroy(): void {} - exists(key: string): Promise { + exists(key: number): Promise { throw new Error('Method not implemented.'); } - get(key: string): Promise { + get(key: number): Promise { throw new Error('Method not implemented.'); } @@ -31,4 +31,8 @@ export default class FakePatStore implements IPatStore { getAllByUser(userId: number): Promise { throw new Error('Method not implemented.'); } + + deleteForUser(id: number, userId: number): Promise { + throw new Error('Method not implemented.'); + } }