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

Jump to bottom

More fine-grained permissions for Fact Tables and SDK Connections #2503

Merged
merged 13 commits into from
May 13, 2024
Merged

More fine-grained permissions for Fact Tables and SDK Connections #2503

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
56bce54
Permission Roles Test Suite
jdorn May 9, 2024
42f5aa9
Merge branch 'main' into permission-roles-test
mknowlton89 May 9, 2024
9d33eff
Added recently added methods.
mknowlton89 May 9, 2024
6b5959a
Adds more granular permissions
mknowlton89 May 10, 2024
e0f1c03
Merges in main and resolves conflicts
mknowlton89 May 10, 2024
d6f6efa
makes manageSDKWebhooks an env scoped permission and updates all of t…
mknowlton89 May 10, 2024
b72d447
Fixes lint issue
mknowlton89 May 10, 2024
de0bb45
Merge remote-tracking branch 'origin/main' into mk/manage-fact-filter…
jdorn May 12, 2024
4c13d2e
Improve method and argument names
jdorn May 12, 2024
9ab33e0
Fix import
jdorn May 13, 2024
60a30e8
Merge remote-tracking branch 'origin/main' into mk/manage-fact-filter…
jdorn May 13, 2024
90e98dc
Clean up test suite
jdorn May 13, 2024
94e8976
Moves manageSDKWebhooks, to ENV_SCOPED_PERMISSIONS in permissions-con…
mknowlton89 May 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions packages/back-end/src/util/organization.util.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,6 +339,7 @@ export function getRoles(_organization: OrganizationInterface): Role[] {
"manageFeatureDrafts",
"manageTargetingAttributes",
"manageEnvironments",
"manageSDKConnections",
"manageNamespaces",
"manageSavedGroups",
"manageArchetype",
Expand All @@ -358,8 +359,10 @@ export function getRoles(_organization: OrganizationInterface): Role[] {
"createAnalyses",
"createDimensions",
"createMetrics",
"manageFactMetrics",
"createSegments",
"manageFactTables",
"manageFactFilters",
"manageTags",
"runQueries",
"editDatasourceSettings",
Expand All @@ -379,6 +382,7 @@ export function getRoles(_organization: OrganizationInterface): Role[] {
"manageFeatureDrafts",
"manageTargetingAttributes",
"manageEnvironments",
"manageSDKConnections",
"manageNamespaces",
"manageSavedGroups",
"manageArchetype",
Expand All @@ -388,7 +392,9 @@ export function getRoles(_organization: OrganizationInterface): Role[] {
"createDimensions",
"createSegments",
"createMetrics",
"manageFactMetrics",
"manageFactTables",
"manageFactFilters",
"runQueries",
"editDatasourceSettings",
"canReview",
Expand Down
6 changes: 5 additions & 1 deletion packages/back-end/src/util/permission-constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
export const ENV_SCOPED_PERMISSIONS = [
"publishFeatures",
"manageEnvironments",
"manageSDKConnections",
"runExperiments",
] as const;

Expand All @@ -14,7 +15,9 @@ export const PROJECT_SCOPED_PERMISSIONS = [
"createAnalyses",
"createIdeas",
"createMetrics",
"manageFactMetrics",
"manageFactTables",
"manageFactFilters",
"createDatasources",
"editDatasourceSettings",
"runQueries",
Expand All @@ -31,7 +34,8 @@ export const GLOBAL_PERMISSIONS = [
"manageTags",
"manageApiKeys",
"manageIntegrations",
"manageWebhooks",
"manageEventWebhooks",
"manageSDKWebhooks",
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be moved to ENV_SCOPED_PERMISSIONS?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file in back-end is not being used. I deleted it in the other PR.

"manageBilling",
"manageNorthStarMetric",
"manageTargetingAttributes",
Expand Down
3 changes: 2 additions & 1 deletion packages/front-end/services/UserContext.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,8 @@ export const DEFAULT_PERMISSIONS: Record<GlobalPermission, boolean> = {
manageArchetype: false,
manageTags: false,
manageTeam: false,
manageWebhooks: false,
manageEventWebhooks: false,
manageSDKWebhooks: false,
manageIntegrations: false,
organizationSettings: false,
superDeleteReport: false,
Expand Down
6 changes: 5 additions & 1 deletion packages/shared/src/permissions/permissions.utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
export const ENV_SCOPED_PERMISSIONS = [
"publishFeatures",
"manageEnvironments",
"manageSDKConnections",
"runExperiments",
] as const;

Expand All @@ -21,7 +22,9 @@ export const PROJECT_SCOPED_PERMISSIONS = [
"createAnalyses",
"createIdeas",
"createMetrics",
"manageFactMetrics",
"manageFactTables",
"manageFactFilters",
"createDatasources",
"editDatasourceSettings",
"runQueries",
Expand All @@ -40,7 +43,8 @@ export const GLOBAL_PERMISSIONS = [
"manageTags",
"manageApiKeys",
"manageIntegrations",
"manageWebhooks",
"manageEventWebhooks",
"manageSDKWebhooks",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about this more, I think we should make a breaking change and grant manageSDKWebhooks permission to engineers and experimenters. I think it was a bug to not do this originally and it will simplify the policies since we can just include this permission in the SDKConnectionsFullAccess policy instead of making a separate SDKWebhook one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good.

So, just to clarify, we'll keep the change that removes manageWebhooks in lieu of the two different manageEventWebhooks and manageSDKWebhooks.

And we'll update engineer and experimenter roles to include manageSDKWebhooks, but we'll keep manageEventWebhooks as an admin-only permission.

In these changes, I think it also makes sense to change the scoping of manageSDKWebhooks so it's env scoped like manageSDKConnections.

"manageBilling",
"manageNorthStarMetric",
"manageNamespaces",
Expand Down
30 changes: 15 additions & 15 deletions packages/shared/src/permissions/permissionsClass.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,31 +61,31 @@ export class Permissions {
};

public canViewEventWebhook = (): boolean => {
return this.checkGlobalPermission("manageWebhooks");
return this.checkGlobalPermission("manageEventWebhooks");
};

public canCreateEventWebhook = (): boolean => {
return this.checkGlobalPermission("manageWebhooks");
return this.checkGlobalPermission("manageEventWebhooks");
};

public canUpdateEventWebhook = (): boolean => {
return this.checkGlobalPermission("manageWebhooks");
return this.checkGlobalPermission("manageEventWebhooks");
};

public canDeleteEventWebhook = (): boolean => {
return this.checkGlobalPermission("manageWebhooks");
return this.checkGlobalPermission("manageEventWebhooks");
};

public canCreateSDKWebhook = (): boolean => {
return this.checkGlobalPermission("manageWebhooks");
return this.checkGlobalPermission("manageSDKWebhooks");
};

public canUpdateSDKWebhook = (): boolean => {
return this.checkGlobalPermission("manageWebhooks");
return this.checkGlobalPermission("manageSDKWebhooks");
};

public canDeleteSDKWebhook = (): boolean => {
return this.checkGlobalPermission("manageWebhooks");
return this.checkGlobalPermission("manageSDKWebhooks");
};

public canCreateAndUpdateTag = (): boolean => {
Expand Down Expand Up @@ -436,19 +436,19 @@ export class Permissions {
public canCreateAndUpdateFactFilter = (
factTable: Pick<FactTableInterface, "projects">
): boolean => {
return this.checkProjectFilterPermission(factTable, "manageFactTables");
return this.checkProjectFilterPermission(factTable, "manageFactFilters");
};

public canDeleteFactFilter = (
factTable: Pick<FactTableInterface, "projects">
): boolean => {
return this.checkProjectFilterPermission(factTable, "manageFactTables");
return this.checkProjectFilterPermission(factTable, "manageFactFilters");
};

public canCreateFactMetric = (
metric: Pick<FactMetricInterface, "projects">
): boolean => {
return this.checkProjectFilterPermission(metric, "createMetrics");
return this.checkProjectFilterPermission(metric, "manageFactMetrics");
};

public canUpdateFactMetric = (
Expand All @@ -458,14 +458,14 @@ export class Permissions {
return this.checkProjectFilterUpdatePermission(
existing,
updates,
"createMetrics"
"manageFactMetrics"
);
};

public canDeleteFactMetric = (
metric: Pick<FactMetricInterface, "projects">
): boolean => {
return this.checkProjectFilterPermission(metric, "createMetrics");
return this.checkProjectFilterPermission(metric, "manageFactMetrics");
};

public canCreateMetric = (
Expand Down Expand Up @@ -688,7 +688,7 @@ export class Permissions {
return this.checkEnvFilterPermission(
sdkConnection,
[sdkConnection.environment],
"manageEnvironments"
"manageSDKConnections"
);
};

Expand All @@ -699,7 +699,7 @@ export class Permissions {
return this.checkEnvFilterUpdatePermission(
existing,
updates,
"manageEnvironments"
"manageSDKConnections"
);
};

Expand All @@ -709,7 +709,7 @@ export class Permissions {
return this.checkEnvFilterPermission(
sdkConnection,
[sdkConnection.environment],
"manageEnvironments"
"manageSDKConnections"
);
};

Expand Down