-
Notifications
You must be signed in to change notification settings - Fork 45
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
editoast: add configurable authorization to TestAppBuilder
#10197
Conversation
fe89a25
to
1bf9525
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## dev #10197 +/- ##
=======================================
Coverage 81.46% 81.46%
=======================================
Files 1058 1058
Lines 104318 104320 +2
Branches 724 722 -2
=======================================
+ Hits 84981 84985 +4
+ Misses 19295 19294 -1
+ Partials 42 41 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
This PR revealed a bug. When no authentication header is sent to editoast, we respond with a 403 instead of a 401. Its fix belongs to another PR, but I don't believe we should be testing a faulty behavior. Here's my proposition:
- Add a function
TestApp::def_user(&self, UserInfo, impl IntoIterator<Item = BuiltinRole>)
that persists a user configuration for a test. - Add a convenience function
TestAppBuilder::user(self, UserInfo, impl IntoIterator<Item = BuiltinRole>)
- Add a function like
app.post("...").from_user(UserInfo { ... }).json(...
to add the headers to the request. - Change the test behavior to "make a request with a real authenticated user which doesn't have the required role", where responding 403 is a valid response.
Wdyt?
1bf9525
to
d708641
Compare
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.
Thanks for the modifications! It looks much better, a few comments about API readability and convenience.
1314060
to
67d9838
Compare
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.
LGTM, thanks!
Signed-off-by: hamz2a <[email protected]>
67d9838
to
8713f84
Compare
Update
TestAppBuilder
to support configurable authorization settingsTestAppBuilder
to allow enabling/disabling authorization via theenable_authorization
field.