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 option to authenticate to an upstream mirror #114

Merged
merged 7 commits into from
Aug 23, 2024

Conversation

Splamy
Copy link

@Splamy Splamy commented Mar 11, 2024

  • Allows users to authenticate to an upstream mirror with Basic, Bearer or Custom Header Auth

Addresses https://github.com/orgs/bagetter/discussions/88

Open questions:

  • Do you prefer logically tied classes in on file or in separate (MirrorOptions, MirrorAuthenticationOptions, MirrorAuthenticationType) ?
  • Since this feature is more or less just piping configuration through to the httpclient I had a hard time creating good unit tests and ended up making integration-ish tests.
    A lot of the composition is done via DI and private methods so there are also little points to mock nicely.
    If you have any suggestions or requests to improve this, I'm open.
    I've also considered adding a dedicated mocking library for http requests to reduce boilerplate, but would like your feedback first before adding new depencies to a project.

@FroggieFrog
Copy link

First thank you for the PR! It will be a valuable addition to the project.
I haven't run or checked the PR yet, but what caught my eye is the documentation.
First I have to say good job. Good use of the Tabs feature 😀
I think this addition justifies a separate page under the Configuration category to prevent cluttering the page.
So my suggestion is the following structure:

  • Configuration
    • Mirror (read-through caching)

The new page should contain the existing documentation for this feature and the new additional configuration.

To answer one of your questions: I personally prefer one class/enum/struct per file.

@loic-sharma
Copy link

loic-sharma commented Mar 12, 2024

This is awesome, excellent work!

I would consider allowing Windows user to opt-in to encrypting their secret using DPAPI, but definitely not a blocker to landing this.

Copy link

@Regenhardt Regenhardt left a comment

Choose a reason for hiding this comment

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

Thanks for the great addition!

docs/docs/configuration.md Outdated Show resolved Hide resolved
docs/docs/configuration.md Outdated Show resolved Hide resolved
@Splamy
Copy link
Author

Splamy commented Mar 12, 2024

Thanks for the feedback, will try to address as soon as I can.

This is awesome, excellent work!

I would consider allowing Windows user to opt-in to encrypting their secret using DPAPI, but definitely not a blocker to landing this.

Interesting idea. Adding this will also need to have a proper ux for that. Either a cli command creating the hashed pw, or creating and updating the field in the appsettings (though appsettings aren't supposed to be automatically written since you will lose formatting and comments).
Maybe ./BaGetter.exe encrypt-password pass123 to stdout like caddy does

@Splamy Splamy force-pushed the feature/mirror_auth branch from 582a10d to 4d47994 Compare March 13, 2024 19:29
@Splamy Splamy requested a review from Regenhardt March 15, 2024 14:28
@Splamy
Copy link
Author

Splamy commented Mar 15, 2024

I think I've adressed everything including the feedback from @FroggieFrog.

@FroggieFrog
Copy link

Your original issue was the authentication against a GitHub feed if I remember correctly.
So we could test it by releasing BaGetter packages on a GitHub feed 😬

@Splamy
Copy link
Author

Splamy commented Mar 15, 2024

For using as a test feed yeah, for actual distribution more only as a gimmick :P

@seriouz
Copy link

seriouz commented Mar 18, 2024

I think there is nothing wrong when we publish BaGetter on NuGet and here on GitHub. At least from my point of view, there is nothing to be said against it.

@Splamy Splamy force-pushed the feature/mirror_auth branch 3 times, most recently from 753703f to 9850ed5 Compare March 18, 2024 17:58
@Splamy Splamy force-pushed the feature/mirror_auth branch from 9850ed5 to 96c0dd1 Compare March 30, 2024 13:24
@Splamy
Copy link
Author

Splamy commented Mar 30, 2024

Sorry for the late update. Somehow oversaw the review response.
Updated the check and rebased to latest main.

Copy link

@Regenhardt Regenhardt left a comment

Choose a reason for hiding this comment

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

Back from vacation! Looks great, just a minor change. Has anyone tested this yet? Otherwise I'll gladly checkout and test it.

@vlada-dudr
Copy link

Can I help with something to get this merged?

@Regenhardt
Copy link

Can I help with something to get this merged?

I will merge it like this, would appreciate it if you made another PR to improve the exception message there.

Copy link

@Regenhardt Regenhardt left a comment

Choose a reason for hiding this comment

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

The last remaining comment from before can be worked on after it is merged too.

@Regenhardt Regenhardt merged commit 0fe5bac into bagetter:main Aug 23, 2024
2 checks passed
@Splamy Splamy deleted the feature/mirror_auth branch August 25, 2024 02:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants