add UploadTokenRequiredAuthenticationCheck #858
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅ 📢 Thoughts on this report? Let us know! |
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #858 +/- ##
=======================================
Coverage 96.31% 96.31%
=======================================
Files 818 818
Lines 18985 19007 +22
=======================================
+ Hits 18285 18307 +22
Misses 700 700
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| # There's a non-zero chance that the full test name would be mis-interpreted by pytest | ||
| # And these tests would never work with ATS. | ||
| # Sadly there's no way to escape the '::' sequence | ||
| request_uri = request_uri.replace("SEPARATOR", "::::") |
There was a problem hiding this comment.
[nit] Ah this is no longer necessary.
Well 1 because we're not using ATS anymore, so who cares.
But 2 because @michelletran-codecov showed me that parametrized tests can have an id.
So we can just add custom ids to the tests and we don't have to leave that SEPARATOR there.
I'm personally favor pytest.param with id= keyword argument
upload/views/helpers.py
Outdated
| service: Service, repo_identifier: str | ||
| ) -> typing.Optional[Repository]: | ||
| service: Service, repo_identifier: str, include_owner: bool = False | ||
| ) -> typing.Optional[Repository] | tuple[Repository | None, Owner | None]: |
There was a problem hiding this comment.
[nit]
this include_owner is unnecessarily complicating this function.
And it looks like it was added for the benefit of get_repository_and_owner_from_string. We have a _get_owner_from_string already.
I'd personally think the extra logic should be moved to get_repository_and_owner_from_string. Have that make use of the 2 other funcitons.
There was a problem hiding this comment.
agree! updating...
upload/views/helpers.py
Outdated
| def get_repository_and_owner_from_string( | ||
| service: Service, repo_identifier: str | ||
| ) -> tuple[Repository | None, Owner | None]: | ||
| return get_repository_from_string( | ||
| service=service, repo_identifier=repo_identifier, include_owner=True | ||
| ) |
There was a problem hiding this comment.
I would actually turn this around:
Move all the logic that does the resolution into get_repository_and_owner_from_string, and remove that include_owner parameter.
Then call this from get_repository_from_string, returning just result[0], which should make things simpler.
The type system can’t reason about that a True parameter means you get a tuple, and otherwise a Repository | None.
So I’m pretty sure the code as is will cause a type error.
There was a problem hiding this comment.
great suggestion!! updating...
Purpose/Motivation
NEW auth class to check for TrueTokenless
Links to relevant tickets
codecov/engineering-team#2298
What does this PR do?
This will be placed at the top of the list for the auth methods on upload endpoints (so it is checked first). In a separate pr.