chore(sentry_apps): Move bases file for sentryapps to sentry_apps #78096
Conversation
github-actions
bot
added
the
Scope: Backend
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## master #78096 +/- ##
==========================================
+ Coverage 78.08% 78.10% +0.01%
==========================================
Files 7067 7074 +7
Lines 311797 312014 +217
Branches 50958 50985 +27
==========================================
+ Hits 243464 243685 +221
+ Misses 61988 61971 -17
- Partials 6345 6358 +13 |
| assert isinstance( | ||
| request.user, (RpcUser, User) | ||
| ), "user must be authenticated to check if they're a sentry app" |
There was a problem hiding this comment.
We should be able to start using user.is_authenticated to discriminate the anonymous user part of the union. See #78057 (comment)
Our main utils func for making requets builds HttpRequests but our inner endpoint functions should be dealing with Request objects (most of the time). Unforutnately, converting with Request() leads to various authentication issues. This change creates a helper method that uses the _force_auth_user/token in the rest_framework Request constructor to add the ForcedAuthenticator() which tells any authenticate() calls in Request to just trust the user/token we are sending and skip other authentication steps. Unforunately this flag is not present on the HttpRequest object but used in rest_framework so it'll fail typing :(
| request._force_auth_user = user | ||
|
|
||
| if token: | ||
| request._force_auth_token = token |
There was a problem hiding this comment.
The 'flag' in rest_framework
Kinda jank.99 because this method will fail typing as the attributes (_force_auth_X) don't exist on the base HttpRequest object, so RIP. This option is 'cleaner', since we're skipping all the auth stuff and only testing the classes/functions related to the module. But it fails typing sooooooo
There are a few other options we could take too...
-
Create a dummy
Endpointobject add the tested permission to itspermission_classesproperty and then pass the HttpRequest to the endpointsinitialize_requestfunction which is more similar to how it works in reality but we also introduce a 'new variable' in the endpoint so not sure if that's like straying away/not really a unit test. -
Pass in some fun authenticators to the
Requestobject, most of the test cases are provided a user that would work withSessionAuthentication. Only exception is the test which takes in no user and uses ApiToken to authenticate, we could use a different authentication and set the header. Kinda similar concern to the previous point where we have to work around another system/tie in another 'variable' (not sure if that's just the norm tho ?) -
Make another
make_requestutils func that returns aRequestobj ??? ( no clue what this would look like) -
something else ?????????? 👀
There was a problem hiding this comment.
Make another make_request utils func that returns a Request obj ??? ( no clue what this would look like)
This approach could work well and be done in way that satisfies mypy. We could construct an instance of a relevant endpoint and use that endpoint to wrap HttpRequest with Request.
Something like
drf_request: Request = DiscordInteractionsEndpoint().initialize_request(self.request)is how the discord integration is handling this scenario.
Co-authored-by: Mark Story <[email protected]>
Move the bases file for sentry apps to sentry_apps!
[X] base file
[X] tests
[x] typing - yes mostly asserts on the sentryapps.py
[X] getsentry shim
issue ref(#73857)