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 custom HTTP handler for authentication errors #211

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

cyb3r4nt
Copy link
Contributor

@cyb3r4nt cyb3r4nt commented Aug 2, 2024

This patch gives a possibility to configure custom HTTP handlers, which can change the default authentication error responses in the middlewares.

The default implementation uses http.Error() method, which produces content type text/plain
and simple textual messages rendered directly into the response body.
In some applications this behavior may not be enough,
and there may be a need to produce different responses like JSON or HTML.
The AuthErrorHTTPHandler interface allows to intercept authentication errors and write HTTP responses according to specific application requirements.

I also added some additional middleware methods, which allow to configure different AuthErrorHTTPHandler implementations for different HTTP routes. Some routes in the applications may require different response bodies or attributes.

This patch gives ability to configure the custom HTTP handlers,
which render authentication and authorization errors.
In some use-cases it is necessary to produce different
Content-Type, body, HTTP headers and status codes
than ones provided by default.
It is possible to configure one global handler,
which is same for all web routes in the Authenticator,
and multiple on-demand handlers
for different middleware instances and web routes.
@cyb3r4nt cyb3r4nt requested a review from umputun as a code owner August 2, 2024 17:45
@coveralls
Copy link

coveralls commented Aug 2, 2024

Pull Request Test Coverage Report for Build 10662414915

Details

  • 43 of 45 (95.56%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.08%) to 83.519%

Changes Missing Coverage Covered Lines Changed/Added Lines %
v2/middleware/auth.go 38 40 95.0%
Totals Coverage Status
Change from base Build 10642899118: 0.08%
Covered Lines: 2630
Relevant Lines: 3149

💛 - Coveralls

Copy link
Collaborator

@paskal paskal left a comment

Choose a reason for hiding this comment

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

Overall looks good, just two minor nitpicks.

README.md Outdated Show resolved Hide resolved
auth_test.go Show resolved Hide resolved
@cyb3r4nt cyb3r4nt requested a review from paskal August 5, 2024 22:13
@umputun
Copy link
Member

umputun commented Oct 13, 2024

"In some applications, this behavior may not be enough, and there may be a need to produce different responses like JSON or HTML."

I don't really get it. What are those cases that need a custom body in response, in addition to the status code? So far, I have seen one semi-convincing case where UI code expected some JSON body in all responses, including 401/403, because it had some trouble handling content-type. To me, it is not a good reason for the change, but in case you have some valid use cases in mind, please share.

@cyb3r4nt
Copy link
Contributor Author

I am trying to create an application where server side page rendering is used (go html/template, templ or any other template engine).
If some protected page is opened in the browser without required authentication or authorization,
then i want to display a proper error page to the user,
and pick some template based on the auth outcome.
It is a simple exchange of the HTML documents without any AJAX support in the client.

This scenario is possible because there is an HttpOnly JWT cookie,
which is sent by browser in HTTP requests.

Right now it is not possible to output a custom response to user
because auth middleware works before any application logic,
and auth middleware writes its own HTTP response to the http.ResponseWriter.

It may be possible to wrap auth middleware and pass a fake http.ResponseWriter to the underlying logic,
including auth middleware itself,
and then decide what to do after wrapped middleware chain completes,
but such use case will require unnecessary copying for valid and authenticated requests,
which count is usually greater than count of auth failures.

In addition, i want to replace the logger.L with my own logger with structured logging and tracing support.

Such proposed AuthErrorHTTPHandler allows to control what is written to HTTP response,
and execute some application specific logic when authentication/authorization has failed.

This is what i want to achieve by introducing this change.

@umputun
Copy link
Member

umputun commented Oct 16, 2024

Right now it is not possible to output a custom response to user because auth middleware works before any application logic, and auth middleware writes its own HTTP response to the http.ResponseWriter.

I don't get this part. There is nothing in the auth middleware forcing you to put it first in the chain of middlewares. You can have others, like logging, panic/recovery, and the custom 4xx status code handling before the auth middleware in your chain.

Unless I'm missing something, I think you can just add your own middleware prior to the auth middleware in your chain and implement your own handling of status codes with a custom implementation of the ResponseWriter. I don't see why it would need to care about the underlying auth middleware, as all you do here is inspect the status code set by auth (or any other middleware in the chain it is wrapping).

Generally speaking, to me, the auth library seems to be the wrong place to provide rendering-related customizations.

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