Skip to content

feat(project-creation): Backend for disableMemberProjectCreation flag #62294

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

Merged
merged 10 commits into from
Jul 8, 2024

Conversation

leedongwei
Copy link
Member

@leedongwei leedongwei commented Dec 22, 2023

Depends on #62277

@github-actions github-actions bot added Scope: Frontend Automatically applied to PRs that change frontend components Scope: Backend Automatically applied to PRs that change backend components labels Dec 22, 2023
Copy link
Contributor

🚨 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.

@leedongwei leedongwei changed the title feat(project-creation): Disable project creation using flag feat(project-creation): Backend for disableMemberProjectCreation flag Jan 4, 2024
Copy link

codecov bot commented Jan 4, 2024

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (e1bfc17) 81.25% compared to head (918adf8) 81.26%.
Report is 1064 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #62294   +/-   ##
=======================================
  Coverage   81.25%   81.26%           
=======================================
  Files        5198     5197    -1     
  Lines      230206   230225   +19     
  Branches    39759    39774   +15     
=======================================
+ Hits       187059   187095   +36     
+ Misses      37405    37385   -20     
- Partials     5742     5745    +3     
Files Coverage Δ
.../api/endpoints/organization_projects_experiment.py 100.00% <100.00%> (ø)
src/sentry/api/serializers/models/organization.py 95.33% <ø> (-0.02%) ⬇️
src/sentry/api/endpoints/organization_details.py 86.04% <33.33%> (-0.42%) ⬇️
src/sentry/api/endpoints/team_projects.py 90.14% <0.00%> (-2.62%) ⬇️

... and 72 files with indirect coverage changes

@leedongwei leedongwei marked this pull request as ready for review January 5, 2024 00:19
@leedongwei leedongwei requested a review from a team as a code owner January 5, 2024 00:19
and not request.access.has_scope("org:write")
):
return Response(
{"detail": "Your organization has disabled this feature for members."}, status=403
Copy link
Member

Choose a reason for hiding this comment

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

nit: add the const? DISABLED_FEATURE_ERROR_STRING

Comment on lines 171 to 173
return Response(
{"detail": "Your organization has disabled this feature for members."}, status=403
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return Response(
{"detail": "Your organization has disabled this feature for members."}, status=403
)
raise PermissionDenied(detail=DISABLED_FEATURE_ERROR_STRING)

@@ -164,6 +164,13 @@ def post(self, request: Request, team) -> Response:

if not serializer.is_valid():
return Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST)
if (
team.organization.flags.disable_member_project_creation
and not request.access.has_scope("org:write")
Copy link
Contributor

Choose a reason for hiding this comment

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

This would disable creating new projects for even team admins that are normal org members, is this intended?

Comment on lines +93 to +95
if organization.flags.disable_member_project_creation and not request.access.has_scope(
"org:write"
):
Copy link
Contributor

Choose a reason for hiding this comment

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

Still learning the models, so sorry for the basic questions:

  • Will admins always have the org:write scope attached to their access?
  • Is the permission/scope limited to the project, or is it a global user permission?
  • What about if this is invoked through an API token?
  • Is seems like based on OrgProjectPermission, the org:write is already required, and the endpoint will fail if that scope is not part of the request, so is this line still needed?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is seems like based on OrgProjectPermission, the org:write is already required, and the endpoint will fail if that scope is not part of the request, so is this line still needed?

Nevermind, we overwrite the POST permissions, and the source map is not additive, so this line is needed

Copy link
Contributor

@schew2381 schew2381 Jan 5, 2024

Choose a reason for hiding this comment

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

org admins have org:write, but regular org members can become team admins, in which case they don't have org:write. Org admins are automatically made into team admins when they join teams.

Copy link
Contributor

Choose a reason for hiding this comment

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

Scopes are attached to each user depending on their role in the organization. For example, org owners are the only ones with org:admin.

Api tokens have scopes attached to them, which you can select when making a user token or integration token

Copy link
Contributor

Choose a reason for hiding this comment

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

Would a regular member/user have access to tokens that have this elevated permission (scope)? Trying to understand if a user can side step this permission check even if it's not coming directly from them. So even if I'm blocked on the UI, I could get around this if I go through automation

Copy link
Contributor

@schew2381 schew2381 Jan 5, 2024

Choose a reason for hiding this comment

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

User tokens can have any scopes b/c it can be applied to multiple orgs the user is part of. We take the intersection between the user token's scopes and the user's role in the org.

Integration tokens are tied to orgs, but we don't allow regular members to create them

@getsantry
Copy link
Contributor

getsantry bot commented Jan 27, 2024

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you remove the label Waiting for: Community, I will leave it alone ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

@getsantry getsantry bot added the Stale label Jan 27, 2024
@getsantry getsantry bot closed this Feb 4, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Feb 19, 2024
@leedongwei leedongwei added WIP and removed Stale labels Feb 28, 2024
@leedongwei leedongwei reopened this Feb 28, 2024
RyanSkonnord and others added 2 commits June 18, 2024 15:17
@RyanSkonnord RyanSkonnord merged commit 5992460 into master Jul 8, 2024
47 of 48 checks passed
@RyanSkonnord RyanSkonnord deleted the dlee/pc-changes branch July 8, 2024 18:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Backend Automatically applied to PRs that change backend components Scope: Frontend Automatically applied to PRs that change frontend components WIP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants