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 test for DefaultHTTPInterceptor #4555

Closed
jpkrohling opened this issue Dec 15, 2021 · 9 comments
Closed

Add test for DefaultHTTPInterceptor #4555

jpkrohling opened this issue Dec 15, 2021 · 9 comments
Assignees
Labels
auth Authentication related ci-cd CI, CD, testing, build issues good first issue Good for newcomers

Comments

@jpkrohling
Copy link
Member

For some reason, a test for configauth.DefaultHTTPInterceptor is currently missing. There should definitely be a test for it.

@jpkrohling jpkrohling added good first issue Good for newcomers auth Authentication related ci-cd CI, CD, testing, build issues labels Dec 15, 2021
@jpkrohling jpkrohling self-assigned this Dec 15, 2021
@jpkrohling
Copy link
Member Author

This code has been moved to this place:

func authInterceptor(next http.Handler, authenticate configauth.AuthenticateFunc) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
ctx, err := authenticate(r.Context(), r.Header)
if err != nil {
http.Error(w, http.StatusText(http.StatusUnauthorized), http.StatusUnauthorized)
return
}
next.ServeHTTP(w, r.WithContext(ctx))
})
}

@josephwoodward
Copy link
Contributor

I'm happy to pick this one up?

@jpkrohling
Copy link
Member Author

It is yours!

@jpkrohling
Copy link
Member Author

@josephwoodward, do you need help moving forward with this?

@josephwoodward
Copy link
Contributor

josephwoodward commented Jan 4, 2022

@jpkrohling I had a quick look, but didn't get as much time over Christmas as I was hoping to. Now everything is settling down again I can get it sorted.

Whilst having a quick look at it I noticed the TestServerAuth test covers the happy path of the default handler, so I my understanding is this issue is to cover if authenticate returns an error, is that correct?

@jpkrohling
Copy link
Member Author

so I my understanding is this issue is to cover if authenticate returns an error, is that correct?

Yes, that seems right. Looks like the happy path is covered.

@josephwoodward
Copy link
Contributor

@jpkrohling Great, I've raised a PR here

@josephwoodward
Copy link
Contributor

This issue can now be closed 👍

@bogdandrutu
Copy link
Member

Thanks @josephwoodward

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auth Authentication related ci-cd CI, CD, testing, build issues good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants