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

Fix registration of dev provider in Service.authMiddleware.Providers #201

Merged
merged 3 commits into from
Jun 18, 2024

Conversation

cyb3r4nt
Copy link
Contributor

@cyb3r4nt cyb3r4nt commented Jun 3, 2024

There was one minor problem related to the dev and Apple providers registration.

Now it is possible to have a configuration,
where only one single dev provider is enabled.

Providers were not registered into Service.authMiddleware.Providers slice in the Service.AddDevProvider() and Service.AddAppleProvider() methods before.

This worked before, because other providers were registered at the same time, and authMiddleware.Providers slice was reassigned by other methods.

Now it is possible to have a configuration,
where only one single dev provider is enabled.

Providers were not registered into Service.authMiddleware.Providers slice
in the Service.AddDevProvider() and Service.AddAppleProvider() methods before.
@cyb3r4nt cyb3r4nt requested a review from umputun as a code owner June 3, 2024 12:18
@paskal
Copy link
Collaborator

paskal commented Jun 5, 2024

I reviewed the changes and I think they are not breaking anything.

The only thing I still do not understand is the original issue, could you please describe it in more detail, and maybe provide a code snippet which reproduces it? Ideally a test which reproduces the problem in master.

@coveralls
Copy link

coveralls commented Jun 5, 2024

Pull Request Test Coverage Report for Build 9349982315

Details

  • 22 of 24 (91.67%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.02%) to 82.73%

Changes Missing Coverage Covered Lines Changed/Added Lines %
v2/auth.go 22 24 91.67%
Totals Coverage Status
Change from base Build 8627959567: -0.02%
Covered Lines: 2558
Relevant Lines: 3092

💛 - Coveralls

@cyb3r4nt
Copy link
Contributor Author

cyb3r4nt commented Jun 5, 2024

The only thing I still do not understand is the original issue, could you please describe it in more detail, and maybe provide a code snippet which reproduces it? Ideally a test which reproduces the problem in master.

I was trying to use this auth lib in my dev environment, and only a single dev provider was configured there.
It is possible to reproduce it with TestIntegrationProtected and commenting out all other providers in prepService() preparation method
This check was failing for me during the request to some protected web endpoint.
Same thing may be reproduced if remark42 is started only with option --auth.dev and without specifying any other providers.

The new and modified version of TestIntegrationProtected from this PR should cover this issue.

@cyb3r4nt cyb3r4nt requested a review from paskal June 5, 2024 16:43
@coveralls
Copy link

coveralls commented Jun 5, 2024

Pull Request Test Coverage Report for Build 9387863023

Details

  • 24 of 24 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.6%) to 83.344%

Totals Coverage Status
Change from base Build 8627959567: 0.6%
Covered Lines: 2577
Relevant Lines: 3092

💛 - Coveralls

@coveralls
Copy link

coveralls commented Jun 5, 2024

Pull Request Test Coverage Report for Build 9387863020

Details

  • 24 of 24 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.6%) to 83.344%

Totals Coverage Status
Change from base Build 8627959567: 0.6%
Covered Lines: 2577
Relevant Lines: 3092

💛 - Coveralls

@paskal
Copy link
Collaborator

paskal commented Jun 18, 2024

@umputun, this one is good to merge.

@cyb3r4nt, please prepare an MR to v2, which would prohibit underscore in the provider name; that would be good if we know it breaks something.

Copy link
Member

@umputun umputun left a comment

Choose a reason for hiding this comment

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

LGTM, thx

@umputun umputun merged commit 350854c into go-pkgz:master Jun 18, 2024
6 checks passed
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.

4 participants