From 414dff1d22dac07e5df542d79afbdd90bb49adaf Mon Sep 17 00:00:00 2001 From: Samir Patel <48686912+54mir@users.noreply.github.com> Date: Mon, 3 Jan 2022 22:56:21 -0600 Subject: [PATCH] revisions --- authn/authenticate.go | 41 ++++++++++++++--------------- authn/authenticate_internal_test.go | 14 +++++----- http/handler.go | 37 +++++++------------------- http/handler_internal_test.go | 26 +++--------------- 4 files changed, 40 insertions(+), 78 deletions(-) diff --git a/authn/authenticate.go b/authn/authenticate.go index 123446893..76c9f8d5d 100644 --- a/authn/authenticate.go +++ b/authn/authenticate.go @@ -6,7 +6,7 @@ import ( "encoding/hex" "encoding/json" "fmt" - "io/ioutil" + "io" "net/http" "time" @@ -114,23 +114,24 @@ func (a *Auth) Login(w http.ResponseWriter, r *http.Request) { } func (a *Auth) Logout(w http.ResponseWriter, r *http.Request) { - newCookie := a.getEmptyCookie() - http.SetCookie(w, newCookie) + http.SetCookie(w, a.getEmptyCookie()) redirect := fmt.Sprintf("%s?post_logout_redirect_uri=%s/", a.logoutEndpoint, a.fbURL) http.Redirect(w, r, redirect, http.StatusTemporaryRedirect) } -// Gets user information from dP and sets a secure cookie +// Gets user information from IdP and sets a secure cookie func (a *Auth) Redirect(w http.ResponseWriter, r *http.Request) { code := r.FormValue("code") - token, err := a.getToken(code) + token, err := a.getToken(r, code) if err != nil { + a.logger.Warnf("getting token from IdP: %+v", err) http.Error(w, "Bad Request: 400", http.StatusBadRequest) return } cv, err := a.newCookieValue(token) if err != nil || cv == nil { + a.logger.Warnf("creating cookie: %+v", err) http.Error(w, "Bad Request: 400", http.StatusBadRequest) return } @@ -143,17 +144,18 @@ func (a *Auth) GetUserInfo(w http.ResponseWriter, r *http.Request) *UserInfo { var resp UserInfo cookie, err := a.readCookie(w, r) if err != nil { - //add logging + a.logger.Warnf("was not able to read cookie for req: %+v", r) return &resp } - resp.UserID = cookie.UserID - resp.UserName = cookie.UserName - return &resp + return &UserInfo{ + UserID: cookie.UserID, + UserName: cookie.UserName, + } } -func (a *Auth) getToken(code string) (*oauth2.Token, error) { - token, err := a.oAuthConfig.Exchange(context.Background(), code) +func (a *Auth) getToken(r *http.Request, code string) (*oauth2.Token, error) { + token, err := a.oAuthConfig.Exchange(r.Context(), code) if err != nil { return nil, errors.Wrap(err, "exchanging auth code for token") } @@ -189,21 +191,20 @@ func (a *Auth) newCookieValue(token *oauth2.Token) (*CookieValue, error) { func (a *Auth) getGroupMembership(token *oauth2.Token) (Groups, error) { var groups Groups - var bearer = fmt.Sprintf("Bearer %s", token.AccessToken) req, err := http.NewRequest("GET", a.groupEndpoint, nil) if err != nil { return groups, errors.Wrap(err, "creating new request to group endpoint") } - req.Header.Add("Authorization", bearer) - client := &http.Client{} + req.Header.Add("Authorization", fmt.Sprintf("Bearer %s", token.AccessToken)) + client := http.DefaultClient response, err := client.Do(req) if err != nil { return groups, errors.Wrap(err, "getting group membership info") } defer response.Body.Close() - rawGroups, err := ioutil.ReadAll(response.Body) + rawGroups, err := io.ReadAll(response.Body) if err != nil { return groups, errors.Wrap(err, "failed reading group membership response") } @@ -224,8 +225,7 @@ func (a *Auth) readCookie(w http.ResponseWriter, r *http.Request) (*CookieValue, var value CookieValue err = a.secure.Decode(a.cookieName, cookie.Value, &value) if err != nil { - newCookie := a.getEmptyCookie() - http.SetCookie(w, newCookie) + http.SetCookie(w, a.getEmptyCookie()) return nil, errors.Wrap(err, "decoding cookie") } @@ -238,7 +238,7 @@ func (a *Auth) setCookie(w http.ResponseWriter, cookie *CookieValue) error { return errors.Wrap(err, "encoding CookieValue") } - newCookie := &http.Cookie{ + http.SetCookie(w, &http.Cookie{ Name: a.cookieName, Value: encoded, Path: "/", @@ -246,8 +246,7 @@ func (a *Auth) setCookie(w http.ResponseWriter, cookie *CookieValue) error { HttpOnly: true, SameSite: http.SameSiteStrictMode, Expires: cookie.Token.Expiry, - } - http.SetCookie(w, newCookie) + }) return nil } @@ -264,7 +263,7 @@ func (a *Auth) refreshToken(w http.ResponseWriter, cookie *CookieValue) error { if newToken.Expiry != cookie.Token.Expiry { cv, err := a.newCookieValue(newToken) if err != nil { - errors.Wrap(err, "setting cookie") + return errors.Wrap(err, "setting cookie") } a.setCookie(w, cv) diff --git a/authn/authenticate_internal_test.go b/authn/authenticate_internal_test.go index 8df8c3b03..07cce1396 100644 --- a/authn/authenticate_internal_test.go +++ b/authn/authenticate_internal_test.go @@ -67,20 +67,20 @@ func TestAuth(t *testing.T) { w := httptest.NewRecorder() err := a.setCookie(w, &validCV) if err != nil { - t.Errorf("expected no errors, got: %v", err) + t.Fatalf("expected no errors, got: %v", err) } if w.Result().Cookies()[0].Value == "" { - t.Errorf("expected some value, got: %+v", w.Result().Cookies()[0].Value) + t.Fatalf("expected some value, got: %+v", w.Result().Cookies()[0].Value) } if w.Result().Cookies()[0].Path != "/" { - t.Errorf("expected path to be /, got: %+v", w.Result().Cookies()[0].Path) + t.Fatalf("expected path to be /, got: %+v", w.Result().Cookies()[0].Path) } }) t.Run("GetEmptyCookie", func(t *testing.T) { c := a.getEmptyCookie() if c.Value != "" { - t.Errorf("expected empty cookie, got: %+v", c.Value) + t.Fatalf("expected empty cookie, got: %+v", c.Value) } }) t.Run("KeyLength", func(t *testing.T) { @@ -98,20 +98,20 @@ func TestAuth(t *testing.T) { ShortKey, ) if err == nil || !strings.Contains(err.Error(), "decoding block key") { - t.Errorf("expected error decoding block key got: %v", err) + t.Fatalf("expected error decoding block key got: %v", err) } }) t.Run("NewCookieValue-BadAccessToken", func(t *testing.T) { _, err := a.newCookieValue(&tokenAT) if err == nil || !strings.Contains(err.Error(), "jwt claims") { - t.Errorf("expected failure regarding jwt claims, got: %v", err) + t.Fatalf("expected failure regarding jwt claims, got: %v", err) } }) t.Run("CookieValue-NoAccessToken", func(t *testing.T) { _, err := a.newCookieValue(&tokenNoAT) if err == nil || !strings.Contains(err.Error(), "access token") { - t.Errorf("expected failure regarding access token, got: %v", err) + t.Fatalf("expected failure regarding access token, got: %v", err) } }) diff --git a/http/handler.go b/http/handler.go index 3eb824cfb..7f9aaf014 100644 --- a/http/handler.go +++ b/http/handler.go @@ -38,7 +38,6 @@ import ( "github.com/molecula/featurebase/v2/rbf" "github.com/molecula/featurebase/v2/topology" "github.com/molecula/featurebase/v2/tracing" - "github.com/molecula/featurebase/v2/vprint" "github.com/pkg/errors" "github.com/prometheus/client_golang/prometheus/promhttp" dto "github.com/prometheus/client_model/go" @@ -541,15 +540,12 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { func (h *Handler) chkAuthN(handler http.HandlerFunc) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { if h.auth != nil { - _, err := h.auth.Authenticate(w, r) - if err != nil { + if _, err := h.auth.Authenticate(w, r); err != nil { http.Error(w, errors.Wrap(err, "authenticating").Error(), http.StatusBadRequest) return } - } else { - handler.ServeHTTP(w, r) } - + handler.ServeHTTP(w, r) } } @@ -564,13 +560,12 @@ func (h *Handler) chkAuthZ(handler http.HandlerFunc, perm authz.Permission) http } if h.permissions == nil { - panic("authentication is turned on without authorization permissions set") + h.logger.Errorf("authentication is turned on without authorization permissions set") + http.Error(w, errors.New("authorizing").Error(), http.StatusInternalServerError) } uinfo := h.auth.GetUserInfo(w, r) - //get query string if applicable - var queryString string queryRequest := r.Context().Value(contextKeyQueryRequest) if req, ok := queryRequest.(*pilosa.QueryRequest); ok { @@ -596,7 +591,6 @@ func (h *Handler) chkAuthZ(handler http.HandlerFunc, perm authz.Permission) http indexName, ok := mux.Vars(r)["index"] if ok { p, err := h.permissions.GetPermissions(groups, indexName) - vprint.VV("p: %+v,perm: %+v,indexName: %+v", p, lperm, indexName) ctx = context.WithValue(r.Context(), contextKeyPermission, p) if err != nil || !p.Satisfies(lperm) { w.Header().Add("Content-Type", "text/plain") @@ -785,8 +779,7 @@ func (h *Handler) filterResponse(w http.ResponseWriter, r *http.Request, schema if h.auth != nil { g := r.Context().Value(contextKeyGroupMembership) if g == nil { - w.Header().Add("Content-Type", "text/plain") - w.WriteHeader(http.StatusForbidden) + http.Error(w, "not authorized", http.StatusForbidden) return nil } indexes := h.permissions.GetAuthorizedIndexList(g.([]authn.Group), authz.Read) @@ -971,10 +964,6 @@ func (h *Handler) handlePostQuery(w http.ResponseWriter, r *http.Request) { qerr := r.Context().Value(contextKeyQueryError) req, ok := qreq.(*pilosa.QueryRequest) - // if !h.isAuthorized(w, r, req, req.Index, authz.Admin.String(), r.URL.Path) { - // return - // } - if DoPerQueryProfiling { backend := pilosa.CurrentBackend() reqHash := hash(req.Query) @@ -3514,9 +3503,7 @@ func (h *Handler) handlePostRestore(w http.ResponseWriter, r *http.Request) { func (h *Handler) handleLogin(w http.ResponseWriter, r *http.Request) { if h.auth == nil { - w.Header().Add("Content-Type", "text/plain") - w.WriteHeader(http.StatusNoContent) - w.Write([]byte("Auth Off")) //nolint:errcheck + http.Error(w, "Auth Off", http.StatusNoContent) return } @@ -3539,9 +3526,7 @@ func (h *Handler) handleCheckAuthentication(w http.ResponseWriter, r *http.Reque return } if h.auth == nil { - w.Header().Add("Content-Type", "text/plain") - w.WriteHeader(http.StatusNoContent) - w.Write([]byte("Auth Off")) //nolint:errcheck + http.Error(w, "Auth Off", http.StatusNoContent) return } groups, err := h.auth.Authenticate(w, r) @@ -3562,9 +3547,7 @@ func (h *Handler) handleUserInfo(w http.ResponseWriter, r *http.Request) { return } if h.auth == nil { - w.Header().Add("Content-Type", "text/plain") - w.WriteHeader(http.StatusNoContent) - w.Write([]byte("Auth Off")) //nolint:errcheck + http.Error(w, "Auth Off", http.StatusNoContent) return } if err := json.NewEncoder(w).Encode(h.auth.GetUserInfo(w, r)); err != nil { @@ -3574,9 +3557,7 @@ func (h *Handler) handleUserInfo(w http.ResponseWriter, r *http.Request) { func (h *Handler) handleLogout(w http.ResponseWriter, r *http.Request) { if h.auth == nil { - w.Header().Add("Content-Type", "text/plain") - w.WriteHeader(http.StatusNoContent) - w.Write([]byte("Auth Off")) //nolint:errcheck + http.Error(w, "Auth Off", http.StatusNoContent) return } h.auth.Logout(w, r) diff --git a/http/handler_internal_test.go b/http/handler_internal_test.go index 6240f813e..f4856e235 100644 --- a/http/handler_internal_test.go +++ b/http/handler_internal_test.go @@ -312,29 +312,11 @@ func TestAuthentication(t *testing.T) { Expires: token.Expiry, } - // permissions1 := `"user-groups": - // "dca35310-ecda-4f23-86cd-876aee55906b": - // "test": "read" - // admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe"` - - permissions2 := `"user-groups": + permissions1 := `"user-groups": "dca35310-ecda-4f23-86cd-876aee559900": "test": "write" admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe"` - // permissions3 := `"user-groups": - // "dca35310-ecda-4f23-86cd-876aee55906b": - // "test": "write" - // "test2": "read" - // "dca35310-ecda-4f23-86cd-876aee559900": - // "test": "read" - // admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe"` - - // permissions4 := `"user-groups": - // "dca35310-ecda-4f23-86cd-876aee559900": - // "test": "" - // admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe"` - tests := []struct { name string path string @@ -631,7 +613,7 @@ admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe"` cookie: validCookie, handler: func(w gohttp.ResponseWriter, r *gohttp.Request) { h := h - permFile := strings.NewReader(permissions2) + permFile := strings.NewReader(permissions1) var p authz.GroupPermissions if err := p.ReadPermissionsFile(permFile); err != nil { t.Errorf("Error: %s", err) @@ -641,8 +623,8 @@ admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe"` f(w, r) }, fn: func(w *httptest.ResponseRecorder, data []byte) { - if w.Result().StatusCode != 403 { - t.Errorf("expected http code 403, got: %+v", w.Result().StatusCode) + if w.Result().StatusCode != 400 { + t.Errorf("expected http code 400, got: %+v", w.Result().StatusCode) } },