-
Notifications
You must be signed in to change notification settings - Fork 17
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/arborist2 #298
Feat/arborist2 #298
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
ffbdc21
to
882defc
Compare
This comment has been minimized.
This comment has been minimized.
882defc
to
bf58edd
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
9d171c3
to
f8842fb
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Note: sheepdog used to check if the user had either "admin" or "delete" to allow deletion. With this update, users who have "admin" but not "delete" can't delete |
tests/conftest.py
Outdated
return response | ||
|
||
mocked_get = MagicMock(side_effect=make_mock_response()) | ||
patch_get = patch("gen3authz.client.arborist.client.ArboristClient.auth_request", mocked_get) |
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 code is not actually mocking GET in general, but rather the auth_request
method in specific, so the names mocked_get
and patch_get
seem a little misleading. Since this fixture is explicitly intended for mocking auth_request
only (per the docstring), and since the HTTP method being used doesn't actually affect the mocked response coming out of make_mock_response
, these could be renamed to be auth_request specific?
tests/conftest.py
Outdated
raise AuthZError('Mocked Arborist says no') | ||
mocked_response = MagicMock(requests.Response) | ||
mocked_response.status_code = 200 | ||
return mocked_response |
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.
I think this can be unrolled? (response
and make_mock_response
)
Can just use response
instead of make_mock_response()
below since the mock only depends on authorized
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.
🎉
@@ -119,6 +118,6 @@ def generate_signed_access_token( | |||
|
|||
# Browser may clip cookies larger than 4096 bytes | |||
if len(token) > 4096: | |||
raise JWTSizeError("JWT exceeded 4096 bytes") | |||
raise Exception("JWT exceeded 4096 bytes") |
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.
Just curious but why? 😅
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.
"JWTSizeError" is only defined in fence, i think that's a leftover from when you removed the fence dependency from the tests
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.
oooo, derp thanks!
@@ -62,7 +62,7 @@ def test_to_delete( | |||
submitter, | |||
require_index_exists_off, |
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.
Wait, how come this doesn't need to mock_arborist_requests()
?
Also: I think, if you want, you can avoid this code duplication by keeping the parametrization but adding another parametrized argument, bool mock_arborist or something, and then mocking arborist in the test based on that value. But probably I'm just missing something here...
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.
the arborist_authorized
fixture is in autouse so by default, mocked arborist requests return True
to False, it raises a 401 error. | ||
""" | ||
|
||
def do_patch(authorized=True): |
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.
Yay thanks!! 😍
project_access
from the test user tokensmember
fixture (only keepsubmitter
andadmin
) since authorization doesn't depend on project access in the token anymoreTODO:
TODO later:
Closes #297
New Features
Breaking Changes
Dependency updates
Deployment changes
ARBORIST
(arborist base URL)