Skip to content
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

Add authentication support to the downloader rewriter #13822

Closed
wants to merge 1 commit into from

Conversation

ma-oli
Copy link
Contributor

@ma-oli ma-oli commented Aug 9, 2021

The external repository downloader rewriter currently allows configuring
bazel to pull external dependencies from alternative locations. However,
it doesn't take credentials in the url into account.

This change fixes that by reading the user information from the URL
object and updating the authHeaders accordingly.

The external repository downloader rewriter currently allows configuring
bazel to pull external dependencies from alternative locations. However,
it doesn't take credentials in the url into account.

This change fixes that by reading the user information from the URL
object and updating the authHeaders accordingly.
@google-cla google-cla bot added the cla: yes label Aug 9, 2021
@ma-oli
Copy link
Contributor Author

ma-oli commented Aug 19, 2021

Just a friendly reminder :)

@@ -101,8 +102,11 @@ public Path download(
}

List<URL> rewrittenUrls = originalUrls;
Map<URI, Map<String, String>> rewrittenAuthHeaders = ImmutableMap.copyOf(authHeaders);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: rewrittenAuthHeaders = authHeaders to save the copy?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please address this comment or I can do it during import if you don't mind.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ma-oli I submitted ma-oli#1 to do this in case that's helpful!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will import the PR and address this during that. Thanks for the PR!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey; sorry about that. I'm not super familiar with Github and I didn't receive these on the right email. Thanks a lot @keith for following up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants