From eea6a40fe08bd96d595ea67799380a85090ee535 Mon Sep 17 00:00:00 2001 From: Samir Patel <48686912+54mir@users.noreply.github.com> Date: Thu, 7 Apr 2022 14:05:55 -0500 Subject: [PATCH 1/4] Iterate through group membership http response Follows the nextLink in http response to iterate through paginated group membership response in order to obtain all groups that the user is a member of. Also, checks cache to make sure we don't add empty groups to the cache. --- authn/authenticate.go | 47 +++++++++++++++++++---------- authn/authenticate_internal_test.go | 39 ++++++++++++++++++------ 2 files changed, 61 insertions(+), 25 deletions(-) diff --git a/authn/authenticate.go b/authn/authenticate.go index 50373199c..a83d552c4 100644 --- a/authn/authenticate.go +++ b/authn/authenticate.go @@ -52,7 +52,8 @@ type Group struct { // Groups holds a slice of Group for marshalling from JSON type Groups struct { - Groups []Group `json:"value"` + NextLink string `json:"@odata.nextLink"` + Groups []Group `json:"value"` } // Auth holds state, configuration, and utilities needed for authentication. @@ -240,25 +241,39 @@ func (a *Auth) Redirect(w http.ResponseWriter, r *http.Request) { func (a *Auth) getGroups(token string) ([]Group, error) { var groups Groups - g, ok := a.groupsCache[token] - if ok && (time.Now().Sub(g.cacheTime) < a.cacheTTL) { - return g.groups, nil + gc, ok := a.groupsCache[token] + if ok && (time.Now().Sub(gc.cacheTime) < a.cacheTTL) && len(gc.groups) > 0 { + return gc.groups, nil } - req, err := http.NewRequest("GET", a.groupEndpoint, nil) - if err != nil { - return groups.Groups, errors.Wrap(err, "creating new request to group endpoint") + nextLink := a.groupEndpoint + for nextLink != "" { + req, err := http.NewRequest("GET", nextLink, nil) + if err != nil { + return nil, errors.Wrap(err, "creating new request to group endpoint") + } + + req.Header.Add("Authorization", fmt.Sprintf("Bearer %s", token)) + response, err := http.DefaultClient.Do(req) + if err != nil { + return nil, errors.Wrap(err, "getting group membership info") + } + if response.StatusCode != http.StatusOK { + return nil, fmt.Errorf("getting group membership info: %s", response.Status) + } + + var g Groups + if err = json.NewDecoder(response.Body).Decode(&g); err != nil { + return groups.Groups, errors.Wrap(err, "failed unmarshalling group membership response") + } + + response.Body.Close() + groups.Groups = append(groups.Groups, g.Groups...) + nextLink = g.NextLink } - req.Header.Add("Authorization", fmt.Sprintf("Bearer %s", token)) - response, err := http.DefaultClient.Do(req) - if err != nil { - return groups.Groups, errors.Wrap(err, "getting group membership info") - } - - defer response.Body.Close() - if err = json.NewDecoder(response.Body).Decode(&groups); err != nil { - return groups.Groups, errors.Wrap(err, "failed unmarshalling group membership response") + if len(groups.Groups) == 0 { + return nil, fmt.Errorf("no groups found") } a.groupsCache[token] = cachedGroups{ diff --git a/authn/authenticate_internal_test.go b/authn/authenticate_internal_test.go index 44c192d28..ca60b4c72 100644 --- a/authn/authenticate_internal_test.go +++ b/authn/authenticate_internal_test.go @@ -352,19 +352,36 @@ func TestGetGroups(t *testing.T) { cacheTime: time.Now(), groups: []Group{ { - GroupID: "i feel it in the water", - GroupName: "i feel it in the earth", + GroupID: "a han noston ned wilith", + GroupName: "I smell it in the air", }, }, }, } - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + srvNext := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { body, err := json.Marshal( Groups{ Groups: []Group{ { - GroupID: "much that once was is lost", - GroupName: "for none now live who remember it", + GroupID: "han mathon ne chae", + GroupName: "I feel it in the earth", + }, + }, + }, + ) + if err != nil { + t.Fatalf("unexpected error marshalling groups response: %v", err) + } + fmt.Fprintf(w, "%s", body) + })) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + body, err := json.Marshal( + Groups{ + NextLink: srvNext.URL, + Groups: []Group{ + { + GroupID: "han mathon ne nen", + GroupName: "i feel it in the water", }, }, }, @@ -384,8 +401,8 @@ func TestGetGroups(t *testing.T) { token: "the world is changed", groups: []Group{ { - GroupID: "i feel it in the water", - GroupName: "i feel it in the earth", + GroupID: "a han noston ned wilith", + GroupName: "I smell it in the air", }, }, }, @@ -393,8 +410,12 @@ func TestGetGroups(t *testing.T) { token: "i smell it in the air", groups: []Group{ { - GroupID: "much that once was is lost", - GroupName: "for none now live who remember it", + GroupID: "han mathon ne nen", + GroupName: "i feel it in the water", + }, + { + GroupID: "han mathon ne chae", + GroupName: "I feel it in the earth", }, }, }, From bddccf6ead84dd3b6c91fed1c2e1b89e77ea8cce Mon Sep 17 00:00:00 2001 From: Samir Patel <48686912+54mir@users.noreply.github.com> Date: Thu, 7 Apr 2022 16:03:27 -0500 Subject: [PATCH 2/4] add http status check to authenticate --- authn/authenticate.go | 3 +++ authn/authenticate_internal_test.go | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/authn/authenticate.go b/authn/authenticate.go index a83d552c4..5cc1d64aa 100644 --- a/authn/authenticate.go +++ b/authn/authenticate.go @@ -129,6 +129,9 @@ func (a *Auth) Authenticate(ctx context.Context, bearer string) (*UserInfo, erro if err != nil { return nil, errors.Wrap(err, "refreshing token") } + if resp.StatusCode != http.StatusOK { + return nil, fmt.Errorf("refreshing token: %s", resp.Status) + } defer resp.Body.Close() var t oauth2.Token if err := json.NewDecoder(resp.Body).Decode(&t); err != nil { diff --git a/authn/authenticate_internal_test.go b/authn/authenticate_internal_test.go index ca60b4c72..305c0af58 100644 --- a/authn/authenticate_internal_test.go +++ b/authn/authenticate_internal_test.go @@ -198,7 +198,7 @@ func TestAuthenticate(t *testing.T) { refresh: true, errOnRefresh: true, exp: -17764800, - err: fmt.Errorf("decoding refreshed token: invalid character 'b' looking for beginning of value"), + err: fmt.Errorf("refreshing token: 500 Internal Server Error"), }, } for _, test := range cases { From 3cf34d750329c36ab07b459fdd153c413c5c5f88 Mon Sep 17 00:00:00 2001 From: Samir Patel <48686912+54mir@users.noreply.github.com> Date: Thu, 7 Apr 2022 17:38:21 -0500 Subject: [PATCH 3/4] update TestChkAuthN --- http_handler_internal_test.go | 44 ++++++++++++++++++++--------------- 1 file changed, 25 insertions(+), 19 deletions(-) diff --git a/http_handler_internal_test.go b/http_handler_internal_test.go index 882eebfe6..c9a30be2f 100644 --- a/http_handler_internal_test.go +++ b/http_handler_internal_test.go @@ -17,6 +17,7 @@ import ( "github.com/golang-jwt/jwt" "github.com/molecula/featurebase/v3/authn" + "github.com/molecula/featurebase/v3/vprint" "golang.org/x/oauth2" "github.com/molecula/featurebase/v3/authz" @@ -650,29 +651,29 @@ func TestChkAuthN(t *testing.T) { } cases := []struct { - name string - endpoint string - token string - handler http.HandlerFunc - statusCode int + name string + endpoint string + token string + handler http.HandlerFunc + err string }{ { - name: "Valid", - token: validToken, - handler: h.chkAuthN(testingHandler), - statusCode: http.StatusOK, + name: "ValidToken-ButNotForMicrosoft", + token: validToken, + handler: h.chkAuthN(testingHandler), + err: "authenticating: getting groups: getting group membership info", }, { - name: "Invalid", - token: invalidToken, - handler: h.chkAuthN(testingHandler), - statusCode: http.StatusUnauthorized, + name: "Invalid", + token: invalidToken, + handler: h.chkAuthN(testingHandler), + err: "authenticating: parsing bearer token", }, { - name: "Expired", - token: expiredToken, - handler: h.chkAuthN(testingHandler), - statusCode: http.StatusUnauthorized, + name: "Expired", + token: expiredToken, + handler: h.chkAuthN(testingHandler), + err: "authenticating: token is expired", }, } for _, test := range cases { @@ -682,8 +683,13 @@ func TestChkAuthN(t *testing.T) { r.Header.Add("Authorization", test.token) test.handler(w, r) resp := w.Result() - if resp.StatusCode != test.statusCode { - t.Fatalf("expected %v, got %v", test.statusCode, resp.StatusCode) + body, err := ioutil.ReadAll(resp.Body) + if err != nil { + t.Fatal(err) + } + if !strings.HasPrefix(string(body), test.err) { + vprint.VV("body: %s", body) + t.Fatalf("expected error %s, got: %s", test.err, string(body)) } }) } From ef6decf63a6ba5d28bccbdaa15b07197ff763571 Mon Sep 17 00:00:00 2001 From: Samir Patel <48686912+54mir@users.noreply.github.com> Date: Mon, 11 Apr 2022 10:34:46 -0500 Subject: [PATCH 4/4] update handler tests --- authn/authenticate_internal_test.go | 2 ++ http_handler_internal_test.go | 36 ++++++++++++++++++++++++++--- 2 files changed, 35 insertions(+), 3 deletions(-) diff --git a/authn/authenticate_internal_test.go b/authn/authenticate_internal_test.go index 305c0af58..57d7dd846 100644 --- a/authn/authenticate_internal_test.go +++ b/authn/authenticate_internal_test.go @@ -374,6 +374,7 @@ func TestGetGroups(t *testing.T) { } fmt.Fprintf(w, "%s", body) })) + defer srvNext.Close() srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { body, err := json.Marshal( Groups{ @@ -391,6 +392,7 @@ func TestGetGroups(t *testing.T) { } fmt.Fprintf(w, "%s", body) })) + defer srv.Close() a.groupEndpoint = srv.URL for name, test := range map[string]struct { diff --git a/http_handler_internal_test.go b/http_handler_internal_test.go index c9a30be2f..f496cf492 100644 --- a/http_handler_internal_test.go +++ b/http_handler_internal_test.go @@ -5,6 +5,7 @@ import ( "bytes" "encoding/hex" "encoding/json" + "fmt" "io/ioutil" "net/http" "net/http/httptest" @@ -17,7 +18,6 @@ import ( "github.com/golang-jwt/jwt" "github.com/molecula/featurebase/v3/authn" - "github.com/molecula/featurebase/v3/vprint" "golang.org/x/oauth2" "github.com/molecula/featurebase/v3/authz" @@ -191,12 +191,42 @@ func readResponse(w *httptest.ResponseRecorder) ([]byte, error) { func TestAuthentication(t *testing.T) { type evaluate func(w *httptest.ResponseRecorder, data []byte) type endpoint func(w http.ResponseWriter, r *http.Request) + + type Group struct { + GroupID string `json:"id"` + GroupName string `json:"displayName"` + } + + // Groups holds a slice of Group for marshalling from JSON + type Groups struct { + NextLink string `json:"@odata.nextLink"` + Groups []Group `json:"value"` + } + + groupSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + body, err := json.Marshal( + Groups{ + Groups: []Group{ + { + GroupID: "what are you?", + GroupName: "i am a carbon-based bipedal life form descended from an ape", + }, + }, + }, + ) + if err != nil { + t.Fatalf("unexpected error marshalling groups response: %v", err) + } + fmt.Fprintf(w, "%s", body) + })) + defer groupSrv.Close() + var ( ClientId = "e9088663-eb08-41d7-8f65-efb5f54bbb71" ClientSecret = "DEADBEEFDEADBEEFDEADBEEFDEADBEEFDEADBEEFDEADBEEFDEADBEEFDEADBEEF" AuthorizeURL = "https://login.microsoftonline.com/4a137d66-d161-4ae4-b1e6-07e9920874b8/oauth2/v2.0/authorize" TokenURL = "https://login.microsoftonline.com/4a137d66-d161-4ae4-b1e6-07e9920874b8/oauth2/v2.0/token" - GroupEndpointURL = "https://graph.microsoft.com/v1.0/me/transitiveMemberOf/microsoft.graph.group?$count=true" + GroupEndpointURL = groupSrv.URL LogoutURL = "https://login.microsoftonline.com/common/oauth2/v2.0/logout" Scopes = []string{"https://graph.microsoft.com/.default", "offline_access"} SecretKey = "DEADBEEFDEADBEEFDEADBEEFDEADBEEFDEADBEEFDEADBEEFDEADBEEFDEADBEEF" @@ -684,11 +714,11 @@ func TestChkAuthN(t *testing.T) { test.handler(w, r) resp := w.Result() body, err := ioutil.ReadAll(resp.Body) + defer resp.Body.Close() if err != nil { t.Fatal(err) } if !strings.HasPrefix(string(body), test.err) { - vprint.VV("body: %s", body) t.Fatalf("expected error %s, got: %s", test.err, string(body)) } })