influxdata / influxdb

Scalable datastore for metrics, events, and real-time analytics
https://influxdata.com
Apache License 2.0
28.98k stars 3.56k forks source link

Simplify access to Authorization in HTTP handlers #10702

Closed mark-rushakoff closed 5 years ago

mark-rushakoff commented 6 years ago

The current pattern looks roughly like this:

func NewBarHandler(a AuthorizationService) *BarHandler {
    h := &BarHandler{
        AuthorizationService: a,
        Router: httprouter.New(),
    }

    h.HandlerFunc("GET", "/api/v2/bars", h.handleGetBars)

    return h
}

func (h *BarHandler) handleGetBars(w http.ResponseWriter, r *http.Request) {
    tok, err := phttp.GetToken(r)
    if err != nil {
        EncodeError(ctx, err, w)
        return
    }

    ctx := r.Context()

    auth, err := h.AuthorizationService.FindAuthorizationByToken(ctx, tok)
    if err != nil {
        EncodeError(ctx, errors.Wrap(err, "invalid token", errors.InvalidData), w)
        return
    }

    // Do stuff with auth...
}

It's repetitive to duplicate that call across many handlers. It would be more convenient to have a middleware, perhaps on AuthorizationService, so that I could instead write:

func NewBarHandler(a AuthorizationService) *BarHandler {
    h := &BarHandler{
        AuthorizationService: a,
        Router: httprouter.New(),
    }

    h.HandlerFunc("GET", "/api/v2/bars", a.RequireAuthorizationOnContext(h.handleGetBars))

    return h
}

func (h *BarHandler) handleGetBars(w http.ResponseWriter, r *http.Request) {
    auth := pcontext.MustGetAuthorization(r.Context()) // Guaranteed to be set due to middleware

    // Do stuff with auth...
}

Maybe in addition to RequireAuthorizationOnContext there would be a way to indicate the token being optional too, but I think requiring it is the more common pattern.

/cc @goller @jademcgough @desa

(edit: code more reflective of reality)

desa commented 6 years ago

I really like the pattern.

stale[bot] commented 5 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] commented 5 years ago

This issue has been automatically closed because it has not had recent activity. Please reopen if this issue is still important to you. Thank you for your contributions.