From 076a007b428ecb9948f28e379bc9fc6e0ffb2fb7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nuno=20G=C3=B3is?= Date: Fri, 14 Oct 2022 13:28:29 +0100 Subject: [PATCH] fix: PATs should have an unique description (per user) (#2191) * fix: PATs should have an unique description * add pat validation on the back-end service * Update src/lib/services/pat-service.ts Co-authored-by: Simon Hornby * fix: only consider current user's PATs * fix tests * cleanup * Update frontend/src/component/user/Profile/PersonalAPITokensTab/CreatePersonalAPIToken/CreatePersonalAPIToken.tsx Co-authored-by: Thomas Heartman * Update src/test/e2e/api/admin/user/pat.e2e.test.ts Co-authored-by: Thomas Heartman Co-authored-by: Simon Hornby Co-authored-by: Thomas Heartman --- .../CreatePersonalAPIToken.tsx | 29 +++++++- src/lib/db/pat-store.ts | 12 ++++ src/lib/services/pat-service.ts | 25 ++++++- src/lib/types/stores/pat-store.ts | 4 ++ src/test/e2e/api/admin/user/pat.e2e.test.ts | 67 ++++++++++++++++++- src/test/fixtures/fake-pat-store.ts | 7 ++ 6 files changed, 138 insertions(+), 6 deletions(-) diff --git a/frontend/src/component/user/Profile/PersonalAPITokensTab/CreatePersonalAPIToken/CreatePersonalAPIToken.tsx b/frontend/src/component/user/Profile/PersonalAPITokensTab/CreatePersonalAPIToken/CreatePersonalAPIToken.tsx index ff570cad1b..64b6cd1b93 100644 --- a/frontend/src/component/user/Profile/PersonalAPITokensTab/CreatePersonalAPIToken/CreatePersonalAPIToken.tsx +++ b/frontend/src/component/user/Profile/PersonalAPITokensTab/CreatePersonalAPIToken/CreatePersonalAPIToken.tsx @@ -93,7 +93,7 @@ export const CreatePersonalAPIToken: FC = ({ setOpen, newToken, }) => { - const { refetchTokens } = usePersonalAPITokens(); + const { tokens, refetchTokens } = usePersonalAPITokens(); const { createPersonalAPIToken, loading } = usePersonalAPITokensApi(); const { setToastApiError } = useToast(); const { uiConfig } = useUiConfig(); @@ -103,6 +103,11 @@ export const CreatePersonalAPIToken: FC = ({ const [expiration, setExpiration] = useState( ExpirationOption['30DAYS'] ); + const [errors, setErrors] = useState<{ [key: string]: string }>({}); + + const clearErrors = () => { + setErrors({}); + }; const calculateDate = () => { const expiresAt = new Date(); @@ -157,6 +162,23 @@ export const CreatePersonalAPIToken: FC = ({ --data-raw '${JSON.stringify(getPersonalAPITokenPayload(), undefined, 2)}'`; }; + const isDescriptionEmpty = (description: string) => description.length; + const isDescriptionUnique = (description: string) => + !tokens?.some(token => token.description === description); + const isValid = + isDescriptionEmpty(description) && isDescriptionUnique(description); + + const onSetDescription = (description: string) => { + clearErrors(); + if (!isDescriptionUnique(description)) { + setErrors({ + description: + 'A personal API token with that description already exists.', + }); + } + setDescription(description); + }; + return ( = ({ setDescription(e.target.value)} + onChange={e => onSetDescription(e.target.value)} required /> @@ -226,6 +250,7 @@ export const CreatePersonalAPIToken: FC = ({ type="submit" variant="contained" color="primary" + disabled={!isValid} > Create token diff --git a/src/lib/db/pat-store.ts b/src/lib/db/pat-store.ts index b7b0db88dd..29a1ed11c3 100644 --- a/src/lib/db/pat-store.ts +++ b/src/lib/db/pat-store.ts @@ -75,6 +75,18 @@ export default class PatStore implements IPatStore { return present; } + async existsWithDescriptionByUser( + description: string, + userId: number, + ): Promise { + const result = await this.db.raw( + `SELECT EXISTS(SELECT 1 FROM ${TABLE} WHERE description = ? AND user_id = ?) AS present`, + [description, userId], + ); + const { present } = result.rows[0]; + return present; + } + async get(id: number): Promise { const row = await this.db(TABLE).where({ id }).first(); return fromRow(row); diff --git a/src/lib/services/pat-service.ts b/src/lib/services/pat-service.ts index c27f67089b..9eb94add10 100644 --- a/src/lib/services/pat-service.ts +++ b/src/lib/services/pat-service.ts @@ -6,6 +6,8 @@ import { PAT_CREATED } from '../types/events'; import { IPat } from '../types/models/pat'; import crypto from 'crypto'; import User from '../types/user'; +import BadDataError from '../error/bad-data-error'; +import NameExistsError from '../error/name-exists-error'; export default class PatService { private config: IUnleashConfig; @@ -30,9 +32,7 @@ export default class PatService { } async createPat(pat: IPat, user: User): Promise { - if (new Date(pat.expiresAt) < new Date()) { - throw new Error('The expiry date should be in future.'); - } + await this.validatePat(pat, user.id); pat.secret = this.generateSecretKey(); pat.userId = user.id; const newPat = await this.patStore.create(pat); @@ -55,6 +55,25 @@ export default class PatService { return this.patStore.deleteForUser(id, userId); } + async validatePat( + { description, expiresAt }: IPat, + userId: number, + ): Promise { + if (!description) { + throw new BadDataError('PAT description cannot be empty'); + } + + if (new Date(expiresAt) < new Date()) { + throw new BadDataError('The expiry date should be in future.'); + } + + if ( + await this.patStore.existsWithDescriptionByUser(description, userId) + ) { + throw new NameExistsError('PAT description already exists'); + } + } + private generateSecretKey() { const randomStr = crypto.randomBytes(28).toString('hex'); return `user:${randomStr}`; diff --git a/src/lib/types/stores/pat-store.ts b/src/lib/types/stores/pat-store.ts index 66c72d3237..2f4a44afca 100644 --- a/src/lib/types/stores/pat-store.ts +++ b/src/lib/types/stores/pat-store.ts @@ -5,4 +5,8 @@ export interface IPatStore extends Store { create(group: IPat): Promise; getAllByUser(userId: number): Promise; deleteForUser(id: number, userId: number): Promise; + existsWithDescriptionByUser( + description: string, + userId: number, + ): Promise; } 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 364c96b104..5c217593f6 100644 --- a/src/test/e2e/api/admin/user/pat.e2e.test.ts +++ b/src/test/e2e/api/admin/user/pat.e2e.test.ts @@ -59,11 +59,13 @@ test('should create a PAT', async () => { }); test('should delete the PAT', async () => { + const description = 'pat to be deleted'; const { request } = app; const { body } = await request .post('/api/admin/user/tokens') .send({ + description, expiresAt: tomorrow, } as IPat) .set('Content-Type', 'application/json') @@ -128,6 +130,7 @@ test('should get only current user PATs', async () => { await request .post('/api/admin/user/tokens') .send({ + description: 'my pat', expiresAt: tomorrow, } as IPat) .set('Content-Type', 'application/json') @@ -149,10 +152,72 @@ test('should fail creation of PAT with passed expiry', async () => { await request .post('/api/admin/user/tokens') .send({ + description: 'my expired pat', expiresAt: yesterday, } as IPat) .set('Content-Type', 'application/json') - .expect(500); + .expect(400); +}); + +test('should fail creation of PAT without a description', async () => { + await app.request + .post('/api/admin/user/tokens') + .send({ + expiresAt: tomorrow, + } as IPat) + .set('Content-Type', 'application/json') + .expect(400); +}); + +test('should fail creation of PAT with a description that already exists for the current user', async () => { + const description = 'duplicate description'; + + await app.request + .post('/api/admin/user/tokens') + .send({ + description, + expiresAt: tomorrow, + } as IPat) + .set('Content-Type', 'application/json') + .expect(201); + + await app.request + .post('/api/admin/user/tokens') + .send({ + description, + expiresAt: tomorrow, + } as IPat) + .set('Content-Type', 'application/json') + .expect(409); +}); + +test('should not fail creation of PAT when a description already exists for another user PAT', async () => { + const description = 'another duplicate description'; + + await app.request + .post('/api/admin/user/tokens') + .send({ + description, + expiresAt: tomorrow, + } as IPat) + .set('Content-Type', 'application/json') + .expect(201); + + await app.request + .post(`/auth/demo/login`) + .send({ + email: 'user-other@getunleash.io', + }) + .expect(200); + + await app.request + .post('/api/admin/user/tokens') + .send({ + description, + expiresAt: tomorrow, + } as IPat) + .set('Content-Type', 'application/json') + .expect(201); }); test('should get user id 1', async () => { diff --git a/src/test/fixtures/fake-pat-store.ts b/src/test/fixtures/fake-pat-store.ts index 8b7cf1e6d3..951b6745b8 100644 --- a/src/test/fixtures/fake-pat-store.ts +++ b/src/test/fixtures/fake-pat-store.ts @@ -20,6 +20,13 @@ export default class FakePatStore implements IPatStore { throw new Error('Method not implemented.'); } + existsWithDescriptionByUser( + description: string, + userId: number, + ): Promise { + throw new Error('Method not implemented.'); + } + get(key: number): Promise { throw new Error('Method not implemented.'); }