ref(flags): limit scopes for secret updates #82897
Conversation
github-actions
bot
added
the
Scope: Backend
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #82897 +/- ##
===========================================
+ Coverage 56.21% 87.61% +31.40%
===========================================
Files 9416 9416
Lines 536122 535938 -184
Branches 21120 20959 -161
===========================================
+ Hits 301375 469570 +168195
+ Misses 234387 66003 -168384
- Partials 360 365 +5 |
| # these scopes can always update or post secrets | ||
| has_update_or_post_access = request.access.has_scope( | ||
| "org:write" | ||
| ) or request.access.has_scope("org:admin") | ||
|
|
||
| try: | ||
| secret = FlagWebHookSigningSecretModel.objects.filter( | ||
| organization_id=organization.id | ||
| ).get(provider=validator.validated_data["provider"]) | ||
| # allow update access if the user created the secret | ||
| if request.user.id == secret.created_by: | ||
| has_update_or_post_access = True | ||
| except FlagWebHookSigningSecretModel.DoesNotExist: | ||
| # anyone can post a new secret | ||
| has_update_or_post_access = True |
There was a problem hiding this comment.
This section confused me a little and is probably brittle. I think we can simplify and make it more understandable.
I would make two variables: has_permission and is_creator. And then implement as:
has_permission = request.access.has_scope(...)
try:
...
is_creator = request.user.id == secret.created_by
except:
is_creator = TrueFinally in the call-site:
if is_creator or has_permission:
...| return Response( | ||
| "You must be an organization owner or manager, or the creator of this secret in order to perform this action.", | ||
| status=403, | ||
| ) | ||
|
|
||
| return Response(status=201) |
There was a problem hiding this comment.
Nit: if we can only return this response if the boolean condition evaluates to true then we should move the response into the boolean itself. It's less confusing IMO if we know the function terminates at this conditional.
Edit: move into the if branch. Moving in to the boolean doesn't make any sense lol.
There was a problem hiding this comment.
updated this as well
michellewzhang
force-pushed
the
mz/fix-secret-auth
branch
from
0137d6e to
40255ef
Compare
closes https://github.com/getsentry/team-replay/issues/522
changes the secret endpoint permissions so that only managers & owners can update a secret (anyone can post) -- only exception is the original creator of the secret can always update their secret, regardless of their scope.