api2go icon indicating copy to clipboard operation
api2go copied to clipboard

Middlewares should allow ending a request.

Open twisted1919 opened this issue 9 years ago • 9 comments

Right now if you write a auth middleware and you want to stop processing in it because the user credentials are not correct, you can't do that.

In gin i can do:

func GinAuth() gin.HandlerFunc {
    return func(c *gin.Context) {
        reqSignature := "..."
        user, err := loadUserFromToken(c.Request.Header.Get("Authorization"), reqSignature)

        if err != nil {
            c.Data(http.StatusForbidden, cType, []byte(err.Error()))
            c.Abort()
            return
        }

        // store user for the request
        c.Set("user", user)

        // continue with next middleware
        c.Next()
    }
}

Which allows allows me stop going to next middleware if i want to, but with api2go i don't see how to do this...

func API2GoAuth(c api2go.APIContexter, w http.ResponseWriter, r *http.Request) {

    reqSignature := "..."
    user, err := loadUserFromToken(r.Header.Get("Authorization"), reqSignature)

    if err != nil {
        w.Header().Set("Content-Type", cType)
        w.WriteHeader(http.StatusForbidden)
        w.Write([]byte(invalidAuthErrors))
        panic("this is the only way that stops things, return does not!")
    }

    // store user for the request
    c.Set("user", user)
}

Let me know if i am missing something here. Thanks.

twisted1919 avatar Dec 31 '15 14:12 twisted1919

Yes, you are right, the context is not entirely implemented. Do you want to work on improving that?

wwwdata avatar Jan 04 '16 10:01 wwwdata

@wwwdata - My GO skills are not there yet, i wouldn't want to do something stupid.

twisted1919 avatar Jan 04 '16 16:01 twisted1919

everybody starts at some point. Even if your go skills are not advanced, just go ahead and practice stuff. We will support and help you and it doesn't matter if you change some things a thousand times :) As long as you also write tests, the coverage is ok, and all our automated tests are passing, what could go wrong?

I guess this ticket also relates somehow to #230 Edit: In the CRUD Operations, the Context can be accessed via the Request.

I personally have not been using the new context feature at all. But I will also look into it and think about it.

@interlock what do you think? What is needed to finish the context feature and make it really useful?

wwwdata avatar Jan 04 '16 16:01 wwwdata

Indeed, we did the least amount to get it to work for our needs. Initially we tried to sneak the GIN style context in, but we decided it was coupling too tightly and pulled back.

An interesting conversation followed at Picatic at the time. net/context is actually more about the context of the handling the network request/connection. In this case @twisted1919 needs, we should really be passing along a cancellable https://godoc.org/golang.org/x/net/context#WithCancel context that is respected within our middleware handlers. That would allow a request to be cancelled on failed Auth for example.

That would not deviate too far from the standard request handler interface we have right now. The downside is that this extends the context (it is a singly linked list) which then means if you used a middleware to add a db session it would be lost on return... which leads me to...

The information we have been putting in context (at least at Picatic) was DB connections, request-ids' and other per request information we needed. That should more appropriately be in the Request as something like a Session Context. The meaning is more clear as to the purpose if we had done that... but it appears nearly everyone abuses net/context the same way we did :frowning:

The downside to splitting the net/context and session is that nearly any already existing middleware that uses net/context will likely break.

The proper solution probably looks something like this:

  • Create a Request.Session as map[string]{}interface
  • Require api2go middlwares/handlers to continue to take our wrapped Request
  • Update middleware and request handlers to handle a Context.WithCancel()

interlock avatar Jan 04 '16 18:01 interlock

Any update?

trungpham avatar Jun 16 '16 09:06 trungpham

maybe it's time to build this ourselves :-) unless you have a PR ready

sharpner avatar Jun 20 '16 07:06 sharpner

For anyone facing this issue, a very crude workaround is to write a HTTP Handler that proxies the api2go handler. This allows one to abort request.

Short example:

type Frontware struct {
    API *api2go.API
}

func (f *Frontware) ServeHTTP(w http.ResponseWriter, r *http.Request) {
    // Abort here if credentials invalid
    f.API.Handler().ServeHTTP(w, r)
}

func main() {
    api := api2go.NewAPI("v1")

    // -- snip api setup -- //

    http.ListenAndServe("[::1]:80", &Frontware{API: api})
}

tscs37 avatar Sep 14 '16 13:09 tscs37

Hi,

I started using this package for a critical service and am honestly a bit bummed out about the missing support for this. I had simply assumed that cancelling a request "early" would be a standard thing to do.

I am willing to submit a PR but am not sure on how to implement this, especially when looking at the details @interlock gave:

- Create a Request.Session as map[string]{}interface
- Require api2go middlwares/handlers to continue to take our wrapped Request
- Update middleware and request handlers to handle a Context.WithCancel()

These three changes could be added in on PR each I guess.

I don't really know what to think about adding a Request.Session property. Wouldn't that be confusing as it could be misinterpreted as something like a user session or similar?

fabiante avatar Jul 26 '21 13:07 fabiante

I think if your only concern is cancelling, it might just be enough to adapt the middleware calling loop to check the context if it has been cancelled, and check all callers of the middlewarechain.

sharpner avatar Jul 26 '21 14:07 sharpner