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.
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
Introduce permissions class #2206
Conversation
…hat'll make it easier to do a find and replace down the road.
… just context.permissions, and updates throwPermissionError to not take an argument.
packages/front-end/components/Settings/EditDataSource/DataSourceMetrics.tsx
Outdated
Show resolved
Hide resolved
@jdorn I've addressed the feedback from last week and also added patterns for |
@@ -1807,3 +1811,636 @@ describe("hasReadAccess filter", () => { | |||
]); | |||
}); | |||
}); | |||
|
|||
describe("PermissionsUtilClass.canCreateMetric check", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that canCreateMetric
is a wrapper around checkProjectFilterPermission
, it feels like what we actually want to test is checkProjectFilterPermission
. But, given checkProjectFilterPermission
is a private method, I'm not aware of a pattern that would allow us to do so without having to mock checkProjectFilterPermission
.
Open to suggestions.
permission: Permission | ||
): boolean { | ||
const projects = obj.projects?.length ? obj.projects : [""]; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll need to add logic here for READ_ONLY_TYPE
permissions e.g. readData
, addComments
, etc, but will wait to do so until it's relevant.
Your preview environment pr-2206-bttf has been deployed. Preview environment endpoints are available at: |
packages/back-end/src/routers/demo-datasource-project/demo-datasource-project.controller.ts
Outdated
Show resolved
Hide resolved
packages/front-end/components/Settings/EditDataSource/DataSourceMetrics.tsx
Outdated
Show resolved
Hide resolved
@@ -244,27 +252,31 @@ export default function DataSourceMetrics({ | |||
</Tooltip> | |||
) : null} | |||
</div> | |||
{editMetricsPermissions[metric.id] ? ( | |||
{!metric.managedBy && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change doesn't seem right. Just because this metric is managed externally, doesn't mean you can't create a copy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah - I thought those were protected from duplicating.
Do you think we should prevent a user from archiving an externally managed metric?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'm pretty sure archiving an official metric will fail in the back-end already, so we should block it in the UI as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good - just updated this logic to:
{editMetricsPermissions[metric.id].canDuplicate ||
(!metric.managedBy &&
editMetricsPermissions[metric.id].canUpdate) ? (
<MoreMenu className="px-2">
{editMetricsPermissions[metric.id]
.canDuplicate ? (
<button
className="btn dropdown-item py-2"
onClick={(e) => {
e.stopPropagation();
e.preventDefault();
setModalData({
current: {
...metric,
managedBy: "",
name: metric.name + " (copy)",
},
edit: false,
duplicate: true,
});
}}
>
<FaRegCopy /> Duplicate
</button>
) : null}
{!metric.managedBy &&
editMetricsPermissions[metric.id].canUpdate ? (
<button
className="btn dropdown-item py-2"
color=""
onClick={async () => {
const newStatus =
metric.status === "archived"
? "active"
: "archived";
await apiCall(`/metric/${metric.id}`, {
method: "PUT",
body: JSON.stringify({
status: newStatus,
}),
});
mutateDefinitions({});
mutate();
}}
>
<FaArchive />{" "}
{metric.status === "archived"
? "Unarchive"
: "Archive"}
</button>
) : null}
</MoreMenu>
) : null}
Features and Changes
This PR introduces a new class with the goal of (eventually) simplifying how engineers can check a user's permission throughout the application.
To kick this off, I've taken on refactoring the
createMetrics
permission as a first stab.Currently, there are a lot of intracacies around how to check a user's permissions, specifically around
projects
.Some resources can only be a part of 1 project, whereas others, can be a part of multiple. Likewise, in the event a resource is in multiple projects, if the user's global role doesn't grant them access, they need to have a project-level role that grants them permission in EVERY project the resource is a part of. However, there are some permissions, that only require the user to have the permission in atleast 1 of the projects the resource is apart of.
Given this complexity, this class will eventually create helper methods that take care of this detail automatically, so the engineer doesn't have to worry about it.
During the transition period, this will live in the context object on the backend, and within the UserContext on the frontend. Likewise, I've created a new
usePermissionsUtil
hook for the frontend. Once we migrate all permissions over, we can remove the oldusePermissions
hook - and we can rename the new one, the standardusePermission
hook, if we'd like.