Feature/permissions #1176
Feature/permissions #1176
Conversation
…ction (#1137) * fix: default privileges definition clashes with the interpolation function * moved interpolation function inside getPrivilegeRulesForUser * Use FormRecord instead of FormConfiguration in permissions * change names to plurial in migration Co-authored-by: Bryan Robitaille <bryan.robitaille.work@gmail.com>
There was a problem hiding this comment.
First off I want to start off by saying super super fantastic work. The amount of changes made my head spin honestly and I can't imagine it was easy to have to jump between contexts in the code base. I tried my best to be thorough in my review. I still probably missed some things. There is a lot of comments and obviously we have a limited amount of time to deliver this feature. Comments you feel can be addressed at a later date we can create issues for and resolve them that way. I'm happy to pair program on things or go over things in a meeting if there is any confusion. Fantastic work once again !
lib/policyBuilder.ts
Outdated
|
|
||
| /* | ||
| This file is referenced by the useAccessControl hook so no server-side | ||
| only dependencies can be referenced in this file. |
There was a problem hiding this comment.
Good comment. Will allow us to avoid some headaches :)
lib/policyBuilder.ts
Outdated
| export type Action = "create" | "view" | "update" | "delete"; | ||
|
|
||
| export type Subject = InferSubjects<CASL_FormRecord | CASL_User | CASL_Privilege | CASL_Flag>; | ||
|
|
There was a problem hiding this comment.
Nit: Spacing here is a little inconsistent between lines. Furthermore there is a jumble of types, variables, classes and interfaces being defined. Better to split them for readability and have consistent spacing between logical groupings of lines of code
lib/policyBuilder.ts
Outdated
|
|
||
| export type Abilities = [Action, Subject]; | ||
| export type AppAbility = MongoAbility<Abilities>; | ||
| export const createAbility = (rules: RawRuleOf<AppAbility>[]) => |
There was a problem hiding this comment.
Nit: Is there any particular reason why we have a function who's sole purpose is to call another and return its value ?
lib/policyBuilder.ts
Outdated
| export const createAbility = (rules: RawRuleOf<AppAbility>[]) => | ||
| createMongoAbility<AppAbility>(rules); | ||
| export class AccessControlError extends Error {} | ||
| export type Ability = MongoAbility; |
There was a problem hiding this comment.
Nit: Is this for convenience sake ? If so I would argue that it causes more of an inconvenience IMO. If I want to introspect the type I will be bought here and then I would need to introspect the MongoAbility type.
lib/policyBuilder.ts
Outdated
| createMongoAbility<AppAbility>(rules); | ||
| export class AccessControlError extends Error {} | ||
| export type Ability = MongoAbility; | ||
| export type Permission = { |
There was a problem hiding this comment.
Nit:This seems like an interface to me
| <div className="mb-8">{`${t("managePermissionsFor")} ${user.name}`}</div> | ||
| <ul className="flex flex-row flex-wrap gap-8 pb-8 pl-0 list-none"> | ||
| {privileges?.map((privilege) => { | ||
| const active = userPrivileges.includes(privilege.id); |
There was a problem hiding this comment.
Every time one privilege is updated the whole list re-renders. It's not so much of a problem I guess considering it's a small list. Ideally the active state would be managed individually by each of the cards
There was a problem hiding this comment.
Future Refactor. No designs exist for this screen, it's just a maintenance screen for now.
pages/admin/vault.tsx
Outdated
| @@ -1,14 +1,13 @@ | |||
| import React, { useState, useEffect } from "react"; | |||
| import { useTranslation } from "next-i18next"; | |||
| import axios from "axios"; | |||
| import { serverSideTranslations } from "next-i18next/serverSideTranslations"; | |||
| // import { serverSideTranslations } from "next-i18next/serverSideTranslations"; | |||
There was a problem hiding this comment.
Why this change ? Doesn't really matter since this page is not really used. Honestly if you want to make changes here you might as well just remove the page
There was a problem hiding this comment.
Will delete the file. The original commenting out was to ignore esLint's no-unused-vars.
Summary | Résumé
This PR modifies the application's access control from a Role Based architecture to an Asset Based architecture.
Implements a new User Management Interface:
Pull Request Checklist
Please complete the following items in the checklist before you request a review: