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

merged 13 commits into from May 13, 2024

Conversation

mknowlton89
Copy link
Collaborator

@mknowlton89 mknowlton89 commented May 10, 2024 •
edited by jdorn

Features and Changes

This PR, currently based on 2499 introduces some finer grain permissions that will eventually be used to define the policies in 2495.

This includes net-new permissions manageFactMetrics, manageFactFilters, manageSDKConnections, and also replaces manageWebhooks with manageEventWebhooks and manageSDKWebhooks.

This was done in a way where it shouldn't impact existing organizations at all.

Dependencies

  • This is dependent on 2499

Testing

  • Ensure test suite passes
  • Ensure an admin (and only admins) can still create event webhooks and SDK webhooks
  • Ensure only engineers and experimenters can create SDK Connections
  • Ensure only analysts and experimenters can create Fact Metrics and Fact Filters
  • Ensure engineers and experimenters can create SDK Webhooks (new only) as long as their project/env permissions grant them the ability

@mknowlton89 mknowlton89 changed the base branch from main to permission-roles-test May 10, 2024 10:51
@mknowlton89 mknowlton89 self-assigned this May 10, 2024
@mknowlton89 mknowlton89 changed the title Mk/manage fact filter permissions Introduce more granular permissions to map to new permission class helper methods May 10, 2024
@mknowlton89 mknowlton89 mentioned this pull request May 10, 2024
Merged
3 tasks
jdorn
jdorn reviewed May 10, 2024
View reviewed changes
packages/shared/src/permissions/permissions.utils.ts Outdated
@@ -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.

Base automatically changed from permission-roles-test to main May 10, 2024 13:55
@jdorn jdorn changed the title Introduce more granular permissions to map to new permission class helper methods Add new permissions for managing fact metrics, fact filters, and SDK connections, and SDK webhooks May 13, 2024
@jdorn jdorn changed the title Add new permissions for managing fact metrics, fact filters, and SDK connections, and SDK webhooks More fine-grained permissions for Fact Tables and SDK Connections May 13, 2024
mknowlton89
mknowlton89 commented May 13, 2024
View reviewed changes
packages/back-end/src/util/permission-constants.ts Outdated
@@ -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.

@mknowlton89 mknowlton89 marked this pull request as ready for review May 13, 2024 11:24
@github-actions GitHub Actions
Copy link

Your preview environment pr-2503-bttf has been deployed.

Preview environment endpoints are available at:

@mknowlton89 mknowlton89 merged commit 1da984c into main May 13, 2024
4 checks passed
@mknowlton89 mknowlton89 deleted the mk/manage-fact-filter-permissions branch May 13, 2024 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdorn jdorn jdorn approved these changes

Assignees

@mknowlton89 mknowlton89

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@mknowlton89 @jdorn