-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
feat(demo-mode): read-only user #79665
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## master #79665 +/- ##
==========================================
+ Coverage 84.95% 87.76% +2.81%
==========================================
Files 9487 9341 -146
Lines 539524 532799 -6725
Branches 21177 20267 -910
==========================================
+ Hits 458344 467636 +9292
+ Misses 80739 64781 -15958
+ Partials 441 382 -59 |
This pull request 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 add the label "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're confident that we don't have a GET/HEAD method that modifies data, then the approach seems reasonable to me.
Are we planning to clean up all the old sandbox code in sentry? |
|
||
email = getattr(user, "email", None) | ||
|
||
return email in options.get("demo-mode.users") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this just a fixed set of demo users that can log in? Can new users sign up for accounts?
@@ -33,7 +33,7 @@ class AcceptProjectTransferEndpoint(Endpoint): | |||
"POST": ApiPublishStatus.PRIVATE, | |||
} | |||
authentication_classes = (SessionAuthentication,) | |||
permission_classes = (IsAuthenticated,) | |||
permission_classes = (SentryPermission,) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't usually use SentryPermission
directly, it's usually a base class we inherit from and apply scopes to.
It seems like if you use this directly, then no users will have permissions on this api:
sentry/src/sentry/api/permissions.py
Lines 167 to 169 in 219f2e6
allowed_scopes: set[str] = set(self.scope_map.get(request.method, [])) | |
current_scopes = request.auth.get_scopes() | |
return any(s in allowed_scopes for s in current_scopes) |
Since this just defined empty scopes, this any
can't return True. Not sure if there's some other piece I'm missing that circumvents this.
SentryPermission
class on User API endpointsContext
sandbox
org https://sandbox.sentry.io/sandbox
org will be synced withdemo
org through sentry-mirrorsandbox
org and Demo User are behinddemo-mode.orgs
anddemo-mode.users
options respectively. Those options are added in PR