ref(dashboards): Add new attribute to Dashboards Perms Model #79909 #80136

harshithadurai · 2024-11-01T17:42:33Z

This PR modifies the existing DashbaordsPermissions model. Previously, edit access for a dashboard could be restricted only to the dashboard's creator. Now, selective teams can be granted edit access.

Summary of changes:

Changes to DashboardPermissions model:

Add new ManytoMany field, teams_with_edit_access
Add new field, is_editable_by_everyone which will eventually serve as a replacement for is_creator_only_editable

Serializer changes:

Add logic to handle teams_with_edit_access as a list of team IDs in the API requests
Add validation to ensure the teams exist
Add validation to ensure that only the creator of a Dashboard can modify the perms

Tests:

Update old tests that update perms by ensuring that the user making the request is logged in as the creator (because of above validation)
Add new tests for teams functionality

…ms-backend

wedamija

I'd recommend splitting out the migrations here from the logic, just since the migration is a little complicated

src/sentry/models/dashboard_permissions.py

github-actions · 2024-11-01T18:56:51Z

🚨 Warning: This pull request contains Frontend and Backend changes!

It's discouraged to make changes to Sentry's Frontend and Backend in a single pull request. The Frontend and Backend are not atomically deployed. If the changes are interdependent of each other, they must be separated into two pull requests and be made forward or backwards compatible, such that the Backend or Frontend can be safely deployed independently.

Have questions? Please ask in the #discuss-dev-infra channel.

markstory

Schema changes look good to me.

markstory · 2024-11-05T14:44:46Z

src/sentry/api/serializers/rest_framework/dashboard.py

+                if teams_data == [] or permissions_data["is_editable_by_everyone"] is True:
+                    permissions.teams_with_edit_access.clear()
+                else:
+                    permissions.teams_with_edit_access.set(Team.objects.filter(id__in=teams_data))


At this point has teams_data been run through validation to ensure that only teams within the current organization are referenced?

Yes, but added an org filter here as well

markstory · 2024-11-05T14:48:42Z

src/sentry/models/dashboard_permissions.py

+        for team in self.teams_with_edit_access.all():
+            if user_id in team.get_member_user_ids():


You have N queries here. If team assignments to dashboards get long this could get slow.

Changed to this:

return self.teams_with_edit_access.filter( organizationmemberteam__organizationmember__user_id=user_id ).exists()

🎉 Very nice, much better.

markstory · 2024-11-05T14:51:30Z

tests/sentry/api/endpoints/test_organization_dashboard_details.py

+        updated_perms = DashboardPermissions.objects.get(dashboard=self.dashboard)
+        assert set(updated_perms.teams_with_edit_access.all()) == {team1, new_team1, new_team2}
+
+    def test_update_dashboard_permissions_with_invalid_teams(self):


It would be good to have a test for teams from other organizations as well.

narsaynorath

One last comment! Everything else looks good, I just realized we should feature flag the validation check for now. Let me know what you want to do with the permission check. I think the easiest option to avoid bugs in the future is to handle it in the backend or else someone in the frontend may forget and send permissions, and we'd get validation issues.

narsaynorath · 2024-11-06T14:11:54Z

src/sentry/api/serializers/rest_framework/dashboard.py

+        permissions = data.get("permissions")
+        if permissions and self.instance:
+            currentUser = self.context["request"].user
+            if self.instance.created_by_id != currentUser.id:
+                raise serializers.ValidationError(
+                    "Only the Dashboard Creator may modify Dashboard Edit Access"
+                )


I just realized, this will trigger if a user updates the dashboard (e.g. they change the title) and the dashboard that we're sending from the frontend has "permissions" in its state, right?

We should feature flag this so it doesn't affect customers inadvertently

Either we should check that the request is actually changing the permissions, or we should ensure that the frontend only sends this when they change the permissions selector

In the case that the user is just updating the dashboard's title, permissions will be None, so it wouldn't go through the if condition. Do you mean that there needs to be a check before the data.get("permissions") statement?

…ms-backend

github-actions · 2024-11-06T17:20:38Z

This PR has a migration; here is the generated SQL for src/sentry/migrations/0785_add_new_field_to_dashboard_permissions.py ()

--
-- Custom state/database change combination
--

                    ALTER TABLE "sentry_dashboardpermissions" ADD COLUMN "is_editable_by_everyone" boolean NOT NULL DEFAULT true;
                    
--
-- Create model DashboardPermissionsTeam
--
CREATE TABLE "sentry_dashboardpermissionsteam" ("id" bigint NOT NULL PRIMARY KEY GENERATED BY DEFAULT AS IDENTITY, "permissions_id" bigint NOT NULL, "team_id" bigint NOT NULL);
CREATE UNIQUE INDEX CONCURRENTLY "sentry_dashboardpermissi_team_id_permissions_id_87e7a578_uniq" ON "sentry_dashboardpermissionsteam" ("team_id", "permissions_id");
ALTER TABLE "sentry_dashboardpermissionsteam" ADD CONSTRAINT "sentry_dashboardpermissi_team_id_permissions_id_87e7a578_uniq" UNIQUE USING INDEX "sentry_dashboardpermissi_team_id_permissions_id_87e7a578_uniq";
ALTER TABLE "sentry_dashboardpermissionsteam" ADD CONSTRAINT "sentry_dashboardperm_permissions_id_bf945b43_fk_sentry_da" FOREIGN KEY ("permissions_id") REFERENCES "sentry_dashboardpermissions" ("id") DEFERRABLE INITIALLY DEFERRED NOT VALID;
ALTER TABLE "sentry_dashboardpermissionsteam" VALIDATE CONSTRAINT "sentry_dashboardperm_permissions_id_bf945b43_fk_sentry_da";
ALTER TABLE "sentry_dashboardpermissionsteam" ADD CONSTRAINT "sentry_dashboardperm_team_id_3de35a2f_fk_sentry_te" FOREIGN KEY ("team_id") REFERENCES "sentry_team" ("id") DEFERRABLE INITIALLY DEFERRED NOT VALID;
ALTER TABLE "sentry_dashboardpermissionsteam" VALIDATE CONSTRAINT "sentry_dashboardperm_team_id_3de35a2f_fk_sentry_te";
CREATE INDEX CONCURRENTLY "sentry_dashboardpermissionsteam_permissions_id_bf945b43" ON "sentry_dashboardpermissionsteam" ("permissions_id");
CREATE INDEX CONCURRENTLY "sentry_dashboardpermissionsteam_team_id_3de35a2f" ON "sentry_dashboardpermissionsteam" ("team_id");
--
-- Add field teams_with_edit_access to dashboardpermissions
--
-- (no-op)

narsaynorath

Great! Changes are looking good, excited to see teams support for permissions 🙏

…#80344) Addresses: #79828 and #79833 Corresponding backend change: #80136 - Add teams as options to the `EditAccessSelector` component - Refactor use of `isCreatorOnlyEditable` to use `isEditableByEveryone` - Add `updateDashbaordPermissions()` function to make a separate `PUT` request to dashboard details endpoint that sends only the perms object - Rename 'Everyone' option to 'All users', and modify tests as well Before: <img width="200" alt="Screenshot 2024-11-07 at 10 26 47 AM" src="https://github.com/user-attachments/assets/9aacc1d6-3002-439b-9a03-c39ec952b955"> After: <img width="200" alt="Screenshot 2024-11-07 at 10 25 38 AM" src="https://github.com/user-attachments/assets/1358752c-30e7-48d0-ba64-f36f15b183b3"> --------- Co-authored-by: harshithadurai <harshi.durai@esentry.io>

harshithadurai added 6 commits November 1, 2024 13:30

edit access for teams

c1b4146

fix serializers, add tests

6dc66bc

fix serializer, add tests

ff832a0

tests + serializer validation

442507d

add through table

641757b

fix typing error

c201cd0

harshithadurai requested a review from a team as a code owner November 1, 2024 17:42

harshithadurai requested review from a team November 1, 2024 17:42

github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Nov 1, 2024

harshithadurai and others added 2 commits November 1, 2024 13:42

Merge branch 'master' into harshi/ref/update-dashboard-perms-with-tea…

b06c0b6

…ms-backend

make migration

da362f0

harshithadurai requested a review from a team as a code owner November 1, 2024 17:43

vercel bot deployed to Preview November 1, 2024 17:48 View deployment

harshithadurai requested a review from narsaynorath November 1, 2024 17:48

wedamija reviewed Nov 1, 2024

View reviewed changes

src/sentry/models/dashboard_permissions.py Show resolved Hide resolved

This comment was marked as outdated.

Sign in to view

harshithadurai added 2 commits November 1, 2024 13:56

remake migration

205cc7b

update apidocs

74f7ab5

vercel bot deployed to Preview November 1, 2024 17:59 View deployment

fix migration

3556b26

vercel bot deployed to Preview November 1, 2024 18:06 View deployment

update dependency

9eb243e

github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Nov 1, 2024

vercel bot deployed to Preview November 1, 2024 18:59 View deployment

fix migration w default field

0aedc16

This comment was marked as outdated.

Sign in to view

vercel bot deployed to Preview November 1, 2024 19:06 View deployment

fix exhaustive or creation for tests

548062f

vercel bot deployed to Preview November 5, 2024 14:42 View deployment

markstory approved these changes Nov 5, 2024

View reviewed changes

improve perms query + add validation to teams in org + tests

6ea70e5

vercel bot deployed to Preview November 5, 2024 20:11 View deployment

update snapshot

574a308

vercel bot deployed to Preview November 5, 2024 21:42 View deployment

narsaynorath reviewed Nov 6, 2024

View reviewed changes

delete to resolve merge conflict

8a6963b

vercel bot deployed to Preview November 6, 2024 14:43 View deployment

harshithadurai and others added 2 commits November 6, 2024 09:50

migration+snapshots

b41624c

Merge branch 'master' into harshi/ref/update-dashboard-perms-with-tea…

e63a1d6

…ms-backend

vercel bot deployed to Preview November 6, 2024 14:59 View deployment

add ff for validation check

bab7340

vercel bot deployed to Preview November 6, 2024 15:08 View deployment

Merge branch 'master' into harshi/ref/update-dashboard-perms-with-tea…

cc85d96

…ms-backend

vercel bot deployed to Preview November 6, 2024 16:36 View deployment

Merge branch 'master' into harshi/ref/update-dashboard-perms-with-tea…

061d48f

…ms-backend

vercel bot deployed to Preview November 6, 2024 17:11 View deployment

add migration + snapshots

1587d21

vercel bot deployed to Preview November 6, 2024 17:21 View deployment

update snapshopts

ec26e9b

harshithadurai requested a review from narsaynorath November 6, 2024 18:17

vercel bot deployed to Preview November 6, 2024 18:20 View deployment

narsaynorath approved these changes Nov 6, 2024

View reviewed changes

harshithadurai merged commit 0ca1a46 into master Nov 6, 2024
50 of 51 checks passed

harshithadurai deleted the harshi/ref/update-dashboard-perms-with-teams-backend branch November 6, 2024 19:00

harshithadurai mentioned this pull request Nov 7, 2024

feat(dashboards): frontend to restrict dashboard edit access to teams #80344

Merged

github-actions bot locked and limited conversation to collaborators Nov 22, 2024

		for team in self.teams_with_edit_access.all():
		if user_id in team.get_member_user_ids():

Uh oh!

ref(dashboards): Add new attribute to Dashboards Perms Model #79909 #80136

ref(dashboards): Add new attribute to Dashboards Perms Model #79909 #80136

Uh oh!

Conversation

harshithadurai commented Nov 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wedamija left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment was marked as outdated.

github-actions bot commented Nov 1, 2024

Uh oh!

This comment was marked as outdated.

markstory left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

narsaynorath left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Nov 6, 2024

Uh oh!

narsaynorath left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

harshithadurai commented Nov 1, 2024 •

edited

Loading