feat: tokenless auth (part II) #308

giovanni-guidini · 2023-12-14T12:37:48Z

Implement TokenlessAuth details, so it actually
does things.

Hook it tokenless auth to the upload endpoints that are supposed to support it.

codecov-qa · 2023-12-14T12:46:43Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c6f790d) 95.97% compared to head (332318c) 96.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #308      +/-   ##
==========================================
+ Coverage   95.97%   96.00%   +0.02%     
==========================================
  Files         613      613              
  Lines       15861    15877      +16     
==========================================
+ Hits        15222    15242      +20     
+ Misses        639      635       -4

Flag	Coverage Δ
unit	`96.00% <100.00%> (+0.02%)`	⬆️
unit-latest-uploader	`96.00% <100.00%> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

codecov-public-qa · 2023-12-14T12:46:49Z

Codecov Report

Merging #308 (332318c) into main (c6f790d) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #308      +/-   ##
==========================================
+ Coverage   95.97%   96.00%   +0.02%     
==========================================
  Files         613      613              
  Lines       15861    15877      +16     
==========================================
+ Hits        15222    15242      +20     
+ Misses        639      635       -4

Flag	Coverage Δ
unit	`96.00% <100.00%> (+0.02%)`	⬆️
unit-latest-uploader	`96.00% <100.00%> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
codecov_auth/authentication/repo_auth.py	`97.57% <100.00%> (+2.36%)`	⬆️
upload/views/commits.py	`100.00% <100.00%> (ø)`
upload/views/reports.py	`100.00% <ø> (ø)`
upload/views/uploads.py	`98.95% <100.00%> (+0.02%)`	⬆️

codecov · 2023-12-14T12:49:47Z

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (c6f790d) 95.59% compared to head (5e4f182) 95.57%.

❗ Current head 5e4f182 differs from pull request most recent head 332318c. Consider uploading reports for the commit 332318c to get more accurate results

Files	Patch %	Lines
codecov_auth/authentication/repo_auth.py	98.27%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##            main    #308     +/-   ##
=======================================
- Coverage   95.59   95.57   -0.02     
=======================================
  Files        728     727      -1     
  Lines      16370   16258    -112     
=======================================
- Hits       15648   15538    -110     
+ Misses       722     720      -2

Flag	Coverage Δ
unit	`95.66% <98.76%> (-0.31%)`	⬇️
unit-latest-uploader	`95.66% <98.76%> (-0.31%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

adrian-codecov · 2023-12-19T18:46:15Z

upload/views/commits.py

+            raise ValidationError("missing branch")
+        # The CLI might have pre-prended the branch with something already
+        if ":" in branch_info:
+            _, branch_info = branch_info.split(":")


Would branch_info always have 2 items if it had a ":"?

I think so. we do fork_slug:branch. And as mentioned by Dana here branches can't have : in them. I don't think repos can either.

adrian-codecov · 2023-12-19T18:48:33Z

upload/tests/views/test_commits.py

+    fake_provider_service.get_pull_request.assert_called_with("4")
+
+
+def test_commit_tokenless_missing_branch(db, client, mocker):


Is tokenless reliant on there being a branch?

only in this implementation, but it would be weird to not have a branch

yaaa makes sense, more so me learning how this works heh

adrian-codecov · 2023-12-19T18:51:26Z

codecov_auth/tests/unit/test_repo_authentication.py

+            authentication.authenticate(request)
+        assert str(exp.value) == "Not valid tokenless upload"
+
+    def test_tokenless_not_supported_services(self):


Does tokenless support gh and github and all there uppercase/lowercase variants?

depends if the upload URL supports that or not. I don't think it does.
If you look in the GetterMixin (here) you'll see that the Service has a limited number of options.

Good to know in case we ever want to support/change for multiple services

adrian-codecov

Generally looks okay, left some questions here and there but looks gtm

context: codecov/engineering-team#736 We want to accept tokenless uploads for forks of public repos. This is a feature that is currently missing from the CLI and upload. These changes are part I of that. What it adds: * `TokenlessAuthentication` - a new auth class that controls when a tokenless upload is accepted * scaffolding `TokenlessAuth` * `TOKENLESS_AUTH_BY_OWNER_SLUG` feature for internal testing * The logic that will let us accept a tokenless upload What is missing: * Implementation of the methods in `TokenlessAuth`, so that it can actually be used * Hooking up the auth to existing endpoints. Conditions to accept tokenless upload: 1. Request has `X-Tokenless` header, value is the fork repo slug AND `X-Tokenless-PR` header, value is the PR number 2. Request is to one of the new upload endpoints 3. The encoded repo (in the request path) exists AND is public repo 4. (temp) the repo is from an owner that can accept tokenless 5. The git service provider reports that the PR in `X-Tokenless-PR` indeed exists and is from `X-Tokenless` repo to the encoded repo.

Implement `TokenlessAuth` details, so it actually does things. Hook it tokenless auth to the upload endpoints that are supposed to support it.

Or "don't trust the CLI to do it". Introduces a check that changes request data prior to serializing commits to make sure we don't overwrite coverage for existing branch in the upstream repo if the upload comes from a fork (tokenless)

codecov-staging · 2023-12-20T19:16:11Z

Codecov Report

All modified and coverable lines are covered by tests ✅

📢 Thoughts on this report? Let us know!

* main: feat: tokenless auth (part II) (#308) feat: tokenless auth (part I) (#304) fix: stop returning 400 code when user installs github app (#314) Always send to mkto (#313) feat: Bundle analysis upload endpoint (#299) Reduce db queries coming from health endpoint (#311)

giovanni-guidini requested review from adrian-codecov, scott-codecov, matt-codecov and JerrySentry December 14, 2023 12:38

giovanni-guidini force-pushed the gio/tokenless/complete-auth branch from c7c8773 to 5e4f182 Compare December 15, 2023 13:38

giovanni-guidini force-pushed the gio/tokenless/token-auth-conditions branch from 57e3903 to 69829a2 Compare December 18, 2023 14:16

adrian-codecov reviewed Dec 19, 2023

View reviewed changes

adrian-codecov approved these changes Dec 19, 2023

View reviewed changes

Base automatically changed from gio/tokenless/token-auth-conditions to main December 20, 2023 18:26

giovanni-guidini added 3 commits December 20, 2023 15:37

feat: tokenless auth (part II)

859f247

Implement `TokenlessAuth` details, so it actually does things. Hook it tokenless auth to the upload endpoints that are supposed to support it.

Address branch conflation bug

332318c

Or "don't trust the CLI to do it". Introduces a check that changes request data prior to serializing commits to make sure we don't overwrite coverage for existing branch in the upstream repo if the upload comes from a fork (tokenless)

giovanni-guidini force-pushed the gio/tokenless/complete-auth branch from 5e4f182 to 332318c Compare December 20, 2023 19:03

giovanni-guidini merged commit 90bec21 into main Dec 20, 2023
18 of 19 checks passed

giovanni-guidini deleted the gio/tokenless/complete-auth branch December 20, 2023 19:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: tokenless auth (part II) #308

feat: tokenless auth (part II) #308

giovanni-guidini commented Dec 14, 2023

codecov-qa bot commented Dec 14, 2023 •

edited

codecov-public-qa bot commented Dec 14, 2023 •

edited

codecov bot commented Dec 14, 2023 •

edited

adrian-codecov Dec 19, 2023

giovanni-guidini Dec 19, 2023

adrian-codecov Dec 19, 2023

giovanni-guidini Dec 19, 2023

adrian-codecov Dec 19, 2023

adrian-codecov Dec 19, 2023

giovanni-guidini Dec 19, 2023

adrian-codecov Dec 19, 2023

adrian-codecov left a comment

codecov-staging bot commented Dec 20, 2023

		fake_provider_service.get_pull_request.assert_called_with("4")


		def test_commit_tokenless_missing_branch(db, client, mocker):

feat: tokenless auth (part II) #308

feat: tokenless auth (part II) #308

Conversation

giovanni-guidini commented Dec 14, 2023

codecov-qa bot commented Dec 14, 2023 • edited

Codecov Report

codecov-public-qa bot commented Dec 14, 2023 • edited

Codecov Report

codecov bot commented Dec 14, 2023 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrian-codecov left a comment

Choose a reason for hiding this comment

codecov-staging bot commented Dec 20, 2023

Codecov Report

codecov-qa bot commented Dec 14, 2023 •

edited

codecov-public-qa bot commented Dec 14, 2023 •

edited

codecov bot commented Dec 14, 2023 •

edited