feat: permissions guards actions#1355
Conversation
fdf414b to
0045a4e
Compare
|
| throw new Error('No user'); | ||
| } | ||
| userService.user = data.user; | ||
| navigationService.refreshSidebar(); |
There was a problem hiding this comment.
I don't think you have to do this, since it already initialize the sidebar on its construction (when the service is provided for the first time)
| export class ApplicationsAccessGuard implements CanActivate { | ||
| private readonly userService = inject(UserService); | ||
| private readonly router = inject(Router); | ||
|
|
||
| canActivate(): boolean { | ||
| const permissions = this.userService.user?.permissions ?? []; | ||
| const hasPermission = permissions.includes('Applications:ListApplications'); | ||
|
|
||
| if (!hasPermission) { | ||
| this.router.navigate(['/dashboard']); | ||
| return false; | ||
| } | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
Could you create an abstract class with just an abstract member permission, which would represent in this case "Applications:ListApplications".
We would have in the end:
export class ApplicationsAccessGuard extends AbstractAcessGuard {
permission = 'Applications:ListApplications';
}| { provide: UserService, useValue: mockUserService }, | ||
| { provide: UserService, useValue: mockUserService } | ||
| ] | ||
| }).inject(CountTasksByStatusComponent); |
There was a problem hiding this comment.
Could you provide a test for:
- hasCountPermission
- The changes you've made in the ngOnInit ?
- The changes you've made in the initRefresh ?
| get hasDownloadPermission(): boolean { | ||
| const permissions = this.userService.user?.permissions ?? []; | ||
| return permissions.includes('Results:DownloadResultData'); | ||
| } |
There was a problem hiding this comment.
This is not generic enough. if we have another download action, we will have to add it there, and it will not be easy to do it in every component.
If you look inside the inspection-object component, you will see that we use an object called field. I think you should update the type Field by adding a permission (which could be a string or an array of string containing "result:downloadResultdata").
Then you could add the input permissions to the component, pass it via inspection-object, and replace the string with this input.
Finally, you will just have to add the permission inside the various ...-inspection.service.ts.
| { provide: NotificationService, useValue: mockNotificationService }, | ||
| { provide: UserService, useValue: mockUserService }, | ||
| ] | ||
| }).inject(ByteArrayComponent); |
There was a problem hiding this comment.
Please provide test for hasDownloadPermission
| <button mat-flat-button | ||
| [color]="hasCancelTaskPermission ? 'accent' : ''" | ||
| (click)="onCancelTasksSelection()" | ||
| [disabled]="!selection.length || !hasCancelTaskPermission" | ||
| [class.disabled-button]="!hasCancelTaskPermission"> |
There was a problem hiding this comment.
No need to change color or add disabled-button class.
| get hasListTasksPermission(): boolean { | ||
| const permissions = this.userService.user?.permissions ?? []; | ||
| return permissions.includes('Tasks:ListTasks') || permissions.includes('Tasks:ListTasksDetailed') || permissions.includes('Tasks:GetResultId'); | ||
| } | ||
|
|
||
| get hasCancelTaskPermission(): boolean { | ||
| const permissions = this.userService.user?.permissions ?? []; | ||
| return permissions.includes('Tasks:CancelTask'); | ||
| } |
| private readonly detailedColumns: string[] = [ | ||
| 'acquiredAt', 'endedAt', 'initialTaskId', 'ownerPodId', 'podHostname', | ||
| 'podTtl', 'receivedAt', 'startedAt', 'statusMessage', 'submittedAt', | ||
| 'creationToEndDuration', 'processingToEndDuration', 'receivedToEndDuration', | ||
| 'countDataDependencies', 'countExpectedOutputIds', 'countParentTaskIds', | ||
| 'countRetryOfIds', 'error', 'fetchedAt', 'processedAt', 'payloadId' | ||
| ]; | ||
|
|
||
| get availableTableColumns(): TableColumn<TaskSummary, TaskOptions>[] { | ||
| const permissions = this.userService.user?.permissions ?? []; | ||
| const hasDetailedPermission = permissions.includes('Tasks:ListTasksDetailed'); | ||
|
|
||
| if (hasDetailedPermission) { | ||
| return this._allTableColumns; | ||
| } | ||
|
|
||
| return this._allTableColumns.filter(column => !this.detailedColumns.includes(column.key as string)); | ||
| } |
There was a problem hiding this comment.
TaskDetailed is a weirdly specific thing for armonik, but it is overall just a task. No need to make such a specific implementation to check it. ListTask is the same.
| <button mat-flat-button | ||
| [color]="canCancel ? 'accent' : ''" | ||
| (click)="cancel()" | ||
| [disabled]="!canCancel" | ||
| [class.disabled-button]="!canCancel"> |
There was a problem hiding this comment.
No need to change code or add disabled-button
| get hasCancelTaskPermission(): boolean { | ||
| const permissions = this.userService.user?.permissions ?? []; | ||
| return permissions.includes('Tasks:CancelTask'); | ||
| } | ||
|
|



No description provided.