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 support for AbortHandler #251

Closed
wants to merge 1 commit into from
Closed

Add support for AbortHandler #251

wants to merge 1 commit into from

Conversation

landerss1
Copy link

With this PR, we will have support for AbortHandler, which will be responsible for sending a result and aborting the middleware processing. This is to address the current issue where the Content-Type of the error response is always text/plan.

Copy link
Member

@simongottschlag simongottschlag left a comment

Choose a reason for hiding this comment

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

I like it overall. Would prefer to find a way that doesn't require us to use interface {} for the context.

Comment on lines +30 to +32
} else {
return c.SendStatus(statusCode)
}
Copy link
Member

Choose a reason for hiding this comment

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

I usually never use else and do early returns instead.

@@ -21,7 +21,10 @@ const DefaultClaimsContextKeyName ClaimsContextKeyName = "claims"
// ErrorHandler is called by the middleware if not nil
type ErrorHandler func(description ErrorDescription, err error)

// ErrorDescription is used to pass the description of the error to ErrorHandler
// AbortHandler is called by the middleware if not nil
type AbortHandler func(c interface{}, statusCode int, description ErrorDescription, err error)
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to not use interface {} here?

Copy link
Contributor

@bittrance bittrance left a comment

Choose a reason for hiding this comment

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

I note that ErrorHandler and AbortHandler are actually used only in the various web framework packages and I wonder if they shouldn't be removed and replaced by a mechanism that allows for framework-specific options? I'll try to PR a proposal in the coming days.

@bittrance
Copy link
Contributor

After some offline discussion, me and @landerss1 came to the conclusion that the current error handler is not very useful and that it would better to be able to provide a "failure handler" which gets a structured representation of the auth failure and would be expected to provide a status code, relevant headers and a []byte body. The framework adaptors would be expected to call this handle and convert the result to an actual HTTP response. Possibly, it should also be able to decide whether middleware evaluation should be aborted or not. Something like this:

type Response struct {
    StatusCode uint,
    Headers map[string]string
    Body []byte
}

func MyFailureHandler(failure Failure) *Response { ... }

We think a "HTTP abstraction" interface is reasonable, since go-oidc-middleware seems to have no ambition to support anything non-HTTP and the basic HTTP structure seems unlikely to change anytime soon. @simongottschlag ?

@simongottschlag
Copy link
Member

After some offline discussion, me and @landerss1 came to the conclusion that the current error handler is not very useful and that it would better to be able to provide a "failure handler" which gets a structured representation of the auth failure and would be expected to provide a status code, relevant headers and a []byte body. The framework adaptors would be expected to call this handle and convert the result to an actual HTTP response. Possibly, it should also be able to decide whether middleware evaluation should be aborted or not. Something like this:

type Response struct {

    StatusCode uint,

    Headers map[string]string

    Body []byte

}



func MyFailureHandler(failure Failure) *Response { ... }

We think a "HTTP abstraction" interface is reasonable, since go-oidc-middleware seems to have no ambition to support anything non-HTTP and the basic HTTP structure seems unlikely to change anytime soon. @simongottschlag ?

I'm quite fine with that, or at least I think so. If both of you think it's the way forward I listen to you! 😊

We do have this that isn't http, but maybe isn't affected?

https://github.com/XenitAB/go-oidc-middleware/blob/main/oidctoken/token.go

@bittrance
Copy link
Contributor

@simongottschlag Re oidctoken #253

@bittrance
Copy link
Contributor

Closing because alternative implementation in #252 merged.

@bittrance bittrance closed this Jun 29, 2023
@landerss1 landerss1 deleted the abort-handler branch March 29, 2024 08:25
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.

None yet

3 participants