Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: favor permission name over id #5409

Merged
merged 6 commits into from
Nov 27, 2023
Merged

refactor: favor permission name over id #5409

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d002de5
refactor: favor permission name over id
nunogois Nov 23, 2023
99de5b2
refactor: adapt access store to favor name over id
nunogois Nov 23, 2023
553a586
test: add tests
nunogois Nov 23, 2023
f5fa390
test: adapt access-store tests to the new logic
nunogois Nov 23, 2023
c8993d5
fix: don't drop the permission_id column just yet
nunogois Nov 24, 2023
a56a1db
test: adapt test to column being there
nunogois Nov 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion frontend/src/component/common/RoleDescription/RoleDescription.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ export const RoleDescription = ({
</StyledDescriptionHeader>
<StyledPermissionsList>
{permissions.map((permission) => (
<li key={permission.id}>
<li key={permission.name}>
{permission.displayName}
</li>
))}
Expand Down
1 change: 0 additions & 1 deletion frontend/src/interfaces/permissions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ export type PermissionType =
| typeof ENVIRONMENT_PERMISSION_TYPE;

export interface IPermission {
id: number;
name: string;
displayName: string;
type: PermissionType;
Expand Down
4 changes: 2 additions & 2 deletions frontend/src/utils/permissions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@ import cloneDeep from 'lodash.clonedeep';

export const getRoleKey = (permission: IPermission): string => {
return permission.environment
? `${permission.id}-${permission.environment}`
: `${permission.id}`;
? `${permission.name}-${permission.environment}`
: `${permission.name}`;
};

export const permissionsToCheckedPermissions = (
Expand Down
42 changes: 19 additions & 23 deletions src/lib/db/access-store.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,15 +39,14 @@ test('resolvePermissions returns empty list if empty list', async () => {
});

test.each([
args([{ id: 1 }]),
args([{ id: 4, environment: 'development' }]),
args([{ id: 4, name: 'should keep the id' }]),
args([{ name: 'CREATE_CONTEXT_FIELD' }]),
args([{ name: 'CREATE_FEATURE', environment: 'development' }]),
args([
{ id: 1, environment: 'development' },
{ id: 2, name: 'ignore this name' },
{ name: 'CREATE_CONTEXT_FIELD' },
{ name: 'CREATE_FEATURE', environment: 'development' },
]),
])(
'resolvePermissions with permission ids (%o) returns the list unmodified',
'resolvePermissions with permission names (%o) will return the list unmodified',
async (permissions) => {
const access = db.stores.accessStore as AccessStore;
const result = await access.resolvePermissions(permissions);
Expand All @@ -56,26 +55,23 @@ test.each([
);

test.each([
args([{ id: 1 }], [{ id: 1, name: 'ADMIN' }]),
args(
[{ name: 'CREATE_CONTEXT_FIELD' }],
[{ id: 18, name: 'CREATE_CONTEXT_FIELD' }],
),
args(
[{ name: 'CREATE_FEATURE', environment: 'development' }],
[{ id: 2, name: 'CREATE_FEATURE', environment: 'development' }],
[{ id: 4, environment: 'development' }],
[{ id: 4, name: 'CREATE_ADDON', environment: 'development' }],
),
args(
[
{ name: 'CREATE_CONTEXT_FIELD' },
{ name: 'CREATE_FEATURE', environment: 'development' },
{ id: 1, environment: 'development' },
{ id: 2, name: 'ignore this name' },
],
[
{ id: 18, name: 'CREATE_CONTEXT_FIELD' },
{ id: 2, name: 'CREATE_FEATURE', environment: 'development' },
{ id: 1, name: 'ADMIN', environment: 'development' },
{ id: 2, name: 'ignore this name' },
],
),
])(
'resolvePermissions with permission names (%o) will inject the ids',
'resolvePermissions with only permission ids (%o) will resolve to named permissions without an id',
async (permissions, expected) => {
const access = db.stores.accessStore as AccessStore;
const result = await access.resolvePermissions(permissions);
Expand All @@ -93,15 +89,15 @@ test.each([
{ name: 'UPDATE_FEATURE', environment: 'development' },
],
[
{ id: 18, name: 'CREATE_CONTEXT_FIELD' },
{ id: 3 },
{ id: 2, name: 'CREATE_FEATURE', environment: 'development' },
{ id: 15, environment: 'development' },
{ id: 7, name: 'UPDATE_FEATURE', environment: 'development' },
{ name: 'CREATE_CONTEXT_FIELD' },
{ id: 3, name: 'CREATE_STRATEGY' },
{ name: 'CREATE_FEATURE', environment: 'development' },
{ id: 15, name: 'UPDATE_STRATEGY', environment: 'development' },
{ name: 'UPDATE_FEATURE', environment: 'development' },
],
),
])(
'resolvePermissions mixed ids and names (%o) will inject the ids where they are missing',
'resolvePermissions mixed ids and names (%o) will inject the names where they are missing',
async (permissions, expected) => {
const access = db.stores.accessStore as AccessStore;
const result = await access.resolvePermissions(permissions);
Expand Down
120 changes: 55 additions & 65 deletions src/lib/db/access-store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,11 +51,7 @@ interface IPermissionRow {
role_id: number;
}

type ResolvedPermission = {
id: number;
name?: string;
environment?: string;
};
type NameAndIdPermission = NamePermissionRef & IdPermissionRef;

export class AccessStore implements IAccessStore {
private logger: Logger;
Expand All @@ -74,70 +70,71 @@ export class AccessStore implements IAccessStore {
});
}

private permissionHasId = (permission: PermissionRef): boolean => {
return (permission as IdPermissionRef).id !== undefined;
private permissionHasName = (permission: PermissionRef): boolean => {
return (permission as NamePermissionRef).name !== undefined;
};

private permissionNamesToIds = async (
permissions: NamePermissionRef[],
): Promise<ResolvedPermission[]> => {
const permissionNames = (permissions ?? [])
.filter((p) => p.name !== undefined)
.map((p) => p.name);
private permissionIdsToNames = async (
permissions: IdPermissionRef[],
): Promise<NameAndIdPermission[]> => {
const permissionIds = (permissions ?? [])
.filter((p) => p.id !== undefined)
.map((p) => p.id);

if (permissionNames.length === 0) {
if (permissionIds.length === 0) {
return [];
}

const stopTimer = this.timer('permissionNamesToIds');
const stopTimer = this.timer('permissionIdsToNames');

const rows = await this.db
.select('id', 'permission')
.from(T.PERMISSIONS)
.whereIn('permission', permissionNames);
.whereIn('id', permissionIds);

const rowByPermissionName = rows.reduce((acc, row) => {
acc[row.permission] = row;
const rowByPermissionId = rows.reduce((acc, row) => {
acc[row.id] = row;
return acc;
}, {} as Map<string, IPermissionRow>);

const permissionsWithIds = permissions.map((permission) => ({
id: rowByPermissionName[permission.name].id,
const permissionsWithNames = permissions.map((permission) => ({
name: rowByPermissionId[permission.id].permission,
...permission,
}));

stopTimer();
return permissionsWithIds;
return permissionsWithNames;
};

resolvePermissions = async (
permissions: PermissionRef[],
): Promise<ResolvedPermission[]> => {
): Promise<NamePermissionRef[]> => {
if (permissions === undefined || permissions.length === 0) {
return [];
}
// permissions without ids (just names)
const permissionsWithoutIds = permissions.filter(
(p) => !this.permissionHasId(p),
) as NamePermissionRef[];
const idPermissionsFromNamed = await this.permissionNamesToIds(
permissionsWithoutIds,
);

if (permissionsWithoutIds.length === permissions.length) {
// all named permissions without ids
return idPermissionsFromNamed;
} else if (permissionsWithoutIds.length === 0) {
// all permissions have ids
return permissions as ResolvedPermission[];
// permissions without names (just ids)
const permissionsWithoutNames = permissions.filter(
(p) => !this.permissionHasName(p),
) as IdPermissionRef[];

if (permissionsWithoutNames.length === permissions.length) {
// all permissions without names
return await this.permissionIdsToNames(permissionsWithoutNames);
} else if (permissionsWithoutNames.length === 0) {
// all permissions have names
return permissions as NamePermissionRef[];
}
// some permissions have ids, some don't (should not happen!)

// some permissions have names, some don't (should not happen!)
const namedPermissionsFromIds = await this.permissionIdsToNames(
permissionsWithoutNames,
);
return permissions.map((permission) => {
if (this.permissionHasId(permission)) {
return permission as ResolvedPermission;
if (this.permissionHasName(permission)) {
return permission as NamePermissionRef;
} else {
return idPermissionsFromNamed.find(
(p) => p.name === (permission as NamePermissionRef).name,
return namedPermissionsFromIds.find(
(p) => p.id === (permission as IdPermissionRef).id,
)!;
}
});
Expand Down Expand Up @@ -204,20 +201,20 @@ export class AccessStore implements IAccessStore {
let userPermissionQuery = this.db
.select(
'project',
'permission',
'rp.permission',
'environment',
'type',
'ur.role_id',
)
.from<IPermissionRow>(`${T.ROLE_PERMISSION} AS rp`)
.join(`${T.ROLE_USER} AS ur`, 'ur.role_id', 'rp.role_id')
.join(`${T.PERMISSIONS} AS p`, 'p.id', 'rp.permission_id')
.join(`${T.PERMISSIONS} AS p`, 'p.permission', 'rp.permission')
.where('ur.user_id', '=', userId);

userPermissionQuery = userPermissionQuery.union((db) => {
db.select(
'project',
'permission',
'rp.permission',
'environment',
'p.type',
'gr.role_id',
Expand All @@ -226,14 +223,14 @@ export class AccessStore implements IAccessStore {
.join(`${T.GROUPS} AS g`, 'g.id', 'gu.group_id')
.join(`${T.GROUP_ROLE} AS gr`, 'gu.group_id', 'gr.group_id')
.join(`${T.ROLE_PERMISSION} AS rp`, 'rp.role_id', 'gr.role_id')
.join(`${T.PERMISSIONS} AS p`, 'p.id', 'rp.permission_id')
.join(`${T.PERMISSIONS} AS p`, 'p.permission', 'rp.permission')
.andWhere('gu.user_id', '=', userId);
});

userPermissionQuery = userPermissionQuery.union((db) => {
db.select(
this.db.raw("'default' as project"),
'permission',
'rp.permission',
'environment',
'p.type',
'g.root_role_id as role_id',
Expand All @@ -245,7 +242,7 @@ export class AccessStore implements IAccessStore {
'rp.role_id',
'g.root_role_id',
)
.join(`${T.PERMISSIONS} as p`, 'p.id', 'rp.permission_id')
.join(`${T.PERMISSIONS} as p`, 'p.permission', 'rp.permission')
.whereNotNull('g.root_role_id')
.andWhere('gu.user_id', '=', userId);
});
Expand Down Expand Up @@ -280,13 +277,13 @@ export class AccessStore implements IAccessStore {
const rows = await this.db
.select(
'p.id',
'p.permission',
'rp.permission',
'rp.environment',
'p.display_name',
'p.type',
)
.from<IPermission>(`${T.ROLE_PERMISSION} as rp`)
.join(`${T.PERMISSIONS} as p`, 'p.id', 'rp.permission_id')
.join(`${T.PERMISSIONS} as p`, 'p.permission', 'rp.permission')
.where('rp.role_id', '=', roleId);
stopTimer();
return rows.map((permission) => {
Expand All @@ -304,12 +301,12 @@ export class AccessStore implements IAccessStore {
role_id: number,
permissions: PermissionRef[],
): Promise<void> {
const resolvedPermission = await this.resolvePermissions(permissions);
const resolvedPermissions = await this.resolvePermissions(permissions);

const rows = resolvedPermission.map((permission) => {
const rows = resolvedPermissions.map((permission) => {
return {
role_id,
permission_id: permission.id,
permission: permission.name,
environment: permission.environment,
};
});
Expand Down Expand Up @@ -808,14 +805,14 @@ export class AccessStore implements IAccessStore {
}
});
// no need to pass down the environment in this particular case because it'll be overriden
const permissionsWithIds = await this.resolvePermissions(
const permissionsWithNames = await this.resolvePermissions(
permissionsAsRefs,
);

const newRoles = permissionsWithIds.map((p) => ({
const newRoles = permissionsWithNames.map((p) => ({
role_id,
environment,
permission_id: p.id,
permission: p.name,
}));

return this.db.batchInsert(T.ROLE_PERMISSION, newRoles);
Expand All @@ -826,17 +823,10 @@ export class AccessStore implements IAccessStore {
permission: string,
environment?: string,
): Promise<void> {
const rows = await this.db
.select('id as permissionId')
.from<number>(T.PERMISSIONS)
.where('permission', permission);

const permissionId = rows[0].permissionId;

return this.db(T.ROLE_PERMISSION)
.where({
role_id,
permission_id: permissionId,
permission,
environment,
})
.delete();
Expand All @@ -856,8 +846,8 @@ export class AccessStore implements IAccessStore {
): Promise<void> {
return this.db.raw(
`insert into role_permission
(role_id, permission_id, environment)
(select role_id, permission_id, ?
(role_id, permission, environment)
(select role_id, permission, ?
from ${T.ROLE_PERMISSION} where environment = ?)`,
[destinationEnvironment, sourceEnvironment],
);
Expand Down
Loading