ref(dashboards): Modify how permissions are handled for editing/deleting dashboards #80684
Conversation
github-actions
bot
added
the
Scope: Backend
This comment was marked as outdated.
This comment was marked as outdated.
tests/sentry/api/endpoints/test_organization_dashboard_details.py
Outdated
Show resolved
Hide resolved
| # allow when Open Membership | ||
| if obj.organization.flags.allow_joinleave: | ||
| return True | ||
| if not features.has( |
There was a problem hiding this comment.
You should just do a if-else with the block on Line 71. It's a bit odd to split it into 2.
| # 2. Dashboard covers all projects or all my projects | ||
| # allow when Open Membership | ||
| if obj.organization.flags.allow_joinleave: | ||
| return True |
There was a problem hiding this comment.
I believe that this should always be applicable even without the flag. It doesn't need to be nested inside the flag.
There was a problem hiding this comment.
The reason we don't want to allow this case is because we want to move towards more explicit dashboard permissions via teams. Users should need to assign themselves to a team to get edit perms, regardless of open membership or project assignments. This gives us clearer control over permissions, as team membership is the real factor, not just project coverage. Plus, all data is already queryable, so project-based restrictions are just a proxy for what we really want—team-based permissions. Also, this permissions class is used to only restrict edit and delete access, so anyone can still view any dashboard.
| ): | ||
| # check if user is restricted from editing dashboard | ||
| if hasattr(obj, "permissions"): | ||
| return obj.permissions.has_edit_permissions(request.user.id) |
There was a problem hiding this comment.
Adding a once-off function like has_edit_permissions to a permission class seems like a code smell to me.
Have you considered adding dashboard scopes? i.e. dashboard:write, dashboard:read, dashboard:admin
There was a problem hiding this comment.
We did consider this, but decided that the scopes are being used for more generic purposes right now (like team and project based access). However, we’ll definitely look into this approach if it becomes more relevant down the line.
harshithadurai
deleted the
harshi/ref/modify-permission-restrictions-for-dashboards
branch
…edit dashboards (#81181) add frontend check to allow managers and owners to always be able to edit dashboards, and dashboard edit perms. this won't work until #80684 is merged --------- Co-authored-by: harshithadurai <[email protected]>
…ing dashboards (#80684) Modify how permissions are handled for editing/deleting dashboards: Previously, users without access to some projects were restricted from editing/deleting dashboards. (Change made here #78615). This PR removes that restriction, and enables restricting edit perms to specific teams. --------- Co-authored-by: harshithadurai <[email protected]>
…edit dashboards (#81181) add frontend check to allow managers and owners to always be able to edit dashboards, and dashboard edit perms. this won't work until #80684 is merged --------- Co-authored-by: harshithadurai <[email protected]>
Modify how permissions are handled for editing/deleting dashboards:
Previously, users without access to some projects were restricted from editing/deleting dashboards. (Change made here #78615). This PR removes that restriction, and enables restricting edit perms to specific teams.