require that the user and github token owner match#19
require that the user and github token owner match#19mgedmin merged 2 commits intomgedmin:masterfrom
Conversation
dhellmann
force-pushed
the
validate-user-and-token
branch
from
5abea51 to
807cd27
Compare
|
Oof, the cog failure on git master not being recognized as failure of the overall build means that I was right to suspect Should be fixed on git master. |
| else: | ||
| self.responses[self.user_endpoint] = MockResponse( | ||
| json={'login': user}, | ||
| ) |
There was a problem hiding this comment.
I'd like to make the mock a bit smarter and return a response only when the request contains an Authorization header, failing with a 401 otherwise.
I think that would have caught the use case where you're now calling _verify_user_token() even if the user hasn't provided a token.
There was a problem hiding this comment.
So if the user is None, the response should be 401 instead of failing with an error because the mock wasn't completely configured?
There was a problem hiding this comment.
I've just checked: get_json_and_links() converts all 4xx errors to an exception, so I don't think the distinction between 401 and 404 really matters for these tests.
mgedmin
left a comment
mgedmin
left a comment
There was a problem hiding this comment.
Overall response: I think this breaks unauthenticated access again by doing the token check too soon, which will return a 401 from GitHub, which IIRC will raise an exception from the get_json_and_links() helper, aborting the process.
Details in single comments.
dhellmann
force-pushed
the
validate-user-and-token
branch
from
807cd27 to
312ff14
Compare
|
I've updated this PR to fix the location of the validation and remove the extra changes in the tests. |
2 participants
No description provided.