From 9f2b888c4f7d83cfbf8242fa3daa3ff7833245e6 Mon Sep 17 00:00:00 2001 From: Seebs Date: Thu, 7 Apr 2022 13:20:00 -0500 Subject: [PATCH 1/7] standardize and correct time range handling If you're wondering how something that simple gets a commit message this long, sit down, because you are in for a ride. The Row, Rows, TopK, and GroupBy(Rows...) commands had three different sets of semantics for from/to ranges. We unify these. Sounds easy, right? The original purpose of this was to address a bug in GroupBy where, if you had multiple queries only one of which used time, we could end up silently returning no results because we tried to do a time query against a non-time field. This was easy to fix; just move a boolean flag from outside a loop to inside the loop so it resets to false on each pass. In the process of trying to test that, I discovered that specifying `from=...` without `to=...` in a Rows in a GroupBy didn't work. Searching around, I discovered that we had three different answers: GroupBy, TopK: unspecified 'to=' is 0 Row: unspecified to is tomorrow Rows: unspecified to is the max time quantum in the field (A time value of 0 is apparently interpreted as January 1st, 0001.) Note that "GroupBy" is really referring to a Rows() command in a GroupBy, it's just that this uses completely different code (because it has to be computing rows potentially matching or restricted to a filter, or provide the rows it generated so they can be used to filter something else). So we fixed that, and made a field method for finding the min/max values (as done in a Rows command that *isn't* in a GroupBy), and tried to use that with viewsByTimeRange. Then I tried to write documentation for this, but the documentation was unclear, and I tried to clear it up. Which caused me to discover that these four different places ALSO differed in when or whether they'd replace a broad query with "just the standard view". So. Round two of the fix: We create a `field.viewsByTimeRange`, which tries to fall back to a standard view when one exists and the specified range covers everything, and treats zero values as non-restrictive, but also picks a narrow range that is actually related to the range of dates in the field. This matters because viewsByTimeRange generates the entire set of views it would need *even if those views don't exist*. We drop one test that was testing Rows specifically to verify that, if you omitted To, we acted as though you'd specified a date two days in the future. That behavior is not now intended, so we drop the test that tries to verify it. Thing that might make this better: Figuring out a way to generate the list of views more cheaply. Right now, we're redoing all the view computation, including producing a sorted list of view names, for every shard. This is excessive, but hard to fix. In particular, there is no trivial way to generate a sorting such that you can take slices of it and have them be the right slices, because we want to skip smaller time quanta when an entire larger parent quantum is included. e.g., if we're including all of April 2022, we don't want to include any of the days for April of 2022, but if we're doing up through April 15th, we want to include the first 15 days of April, but NOT include the whole-month quantum. And so on. Fixing this cleanly is hard and would require a significant design effort. --- executor.go | 87 ++++++++++-------------------------------- executor_test.go | 42 +++++++++++++++++--- field.go | 49 ++++++++++++++++++++++++ field_internal_test.go | 67 ++++++++++++++++++++++++++++++++ 4 files changed, 173 insertions(+), 72 deletions(-) diff --git a/executor.go b/executor.go index 0f52e09d8..ade3b9001 100644 --- a/executor.go +++ b/executor.go @@ -2182,16 +2182,14 @@ func (e *executor) executeTopKShardTime(ctx context.Context, tx Tx, filter *Row, return nil, newNotFoundError(ErrFieldNotFound, field) } - // Check the time quantum. - quantum := f.TimeQuantum() - if quantum == "" { - // ???????? - return nil, nil + views, err := f.viewsByTimeRange(from, to) + if err != nil { + return nil, err } // Fetch fragments. var fragments []*fragment - for _, view := range viewsByTimeRange(viewStandard, from, to, quantum) { + for _, view := range views { f := e.Holder.fragment(index, field, view, shard) if f == nil { continue @@ -3746,48 +3744,9 @@ func (e *executor) executeRowsShard(ctx context.Context, qcx *Qcx, index string, return nil, errors.Wrap(err, "parsing to time") } } - - // Calculate the views for a range as long as some piece of the range - // (from/to) are specified, or if there's no standard view to represent - // all dates. - if !fromTime.IsZero() || !toTime.IsZero() || f.options.NoStandardView { - // If no quantum exists then return an empty result set. - q := f.TimeQuantum() - if q == "" { - return rowIDs, nil - } - - // Get min/max based on existing views. - var vs []string - for _, v := range f.views() { - vs = append(vs, v.name) - } - min, max := minMaxViews(vs, q) - - // If min/max are empty, there were no time views. - if min == "" || max == "" { - return rowIDs, nil - } - - // Convert min/max from string to time.Time. - minTime, err := timeOfView(min, false) - if err != nil { - return rowIDs, errors.Wrapf(err, "getting min time from view: %s", min) - } - if fromTime.IsZero() || fromTime.Before(minTime) { - fromTime = minTime - } - - maxTime, err := timeOfView(max, true) - if err != nil { - return rowIDs, errors.Wrapf(err, "getting max time from view: %s", max) - } - if toTime.IsZero() || toTime.After(maxTime) { - toTime = maxTime - } - - // Determine the views based on the specified time range. - views = viewsByTimeRange(viewStandard, fromTime, toTime, q) + views, err = f.viewsByTimeRange(fromTime, toTime) + if err != nil { + return nil, err } default: return nil, errors.Errorf("%s fields not supported by Rows() query", f.Type()) @@ -4585,21 +4544,12 @@ func (e *executor) executeRowShard(ctx context.Context, qcx *Qcx, index string, return row, err } - // If no quantum exists then return an empty bitmap. - q := f.TimeQuantum() - if q == "" { - return &Row{}, nil - } - - // Set maximum "to" value if only "from" is set. We don't need to worry - // about setting the minimum "from" since it is the zero value if omitted. - if toTime.IsZero() { - // Set the end timestamp to current time + 1 day, in order to account for timezone differences. - toTime = time.Now().AddDate(0, 0, 1) + views, err := f.viewsByTimeRange(fromTime, toTime) + if err != nil { + return nil, err } // Union bitmaps across all time-based views. - views := viewsByTimeRange(viewStandard, fromTime, toTime, q) rows := make([]*Row, 0, len(views)) tx, finisher, err := qcx.GetTx(Txo{Write: !writable, Index: idx, Shard: shard}) defer finisher(&err0) @@ -7856,14 +7806,14 @@ func newGroupByIterator(executor *executor, qcx *Qcx, rowIDs []RowIDs, children idx := holder.Index(index) var ( - fieldName string - viewName string - ok bool - views []string - isTimeField bool + fieldName string + viewName string + ok bool + views []string ) ignorePrev := false for i, call := range children { + var isTimeField bool if fieldName, ok = call.Args["_field"].(string); !ok { return nil, errors.Errorf("%s call must have field with valid (string) field name. Got %v of type %[2]T", call.Name, call.Args["_field"]) } @@ -7907,7 +7857,12 @@ func newGroupByIterator(executor *executor, qcx *Qcx, rowIDs []RowIDs, children } if hasTo || hasFrom { - views = viewsByTimeRange(viewStandard, fromTime, toTime, field.TimeQuantum()) + // Determine the views based on the specified time range. + var err error + views, err = field.viewsByTimeRange(fromTime, toTime) + if err != nil { + return nil, err + } isTimeField = true } else { viewName = viewStandard diff --git a/executor_test.go b/executor_test.go index 6f0e8839d..069f27b7b 100644 --- a/executor_test.go +++ b/executor_test.go @@ -469,21 +469,17 @@ func TestExecutor(t *testing.T) { t.Run("Range", func(t *testing.T) { t.Run("RowIDColumnID", func(t *testing.T) { - // Create a timestamp just out of the current date + 1 day timestamp (default end timestamp). - nextDayExclusive := time.Now().AddDate(0, 0, 2) - - writeQuery := fmt.Sprintf(` + writeQuery := ` Set(2, f=1, 1999-12-31T00:00) Set(3, f=1, 2000-01-01T00:00) Set(4, f=1, 2000-01-02T00:00) Set(5, f=1, 2000-02-01T00:00) Set(6, f=1, 2001-01-01T00:00) Set(7, f=1, 2002-01-01T02:00) - Set(8, f=1, %s) Set(2, f=1, 1999-12-30T00:00) Set(2, f=1, 2002-02-01T00:00) - Set(2, f=10, 2001-01-01T00:00)`, nextDayExclusive.Format("2006-01-02T15:04")) + Set(2, f=10, 2001-01-01T00:00)` readQueries := []string{ `Row(f=1, from=1999-12-31T00:00, to=2002-01-01T03:00)`, `Row(f=1, from=1999-12-31T00:00)`, @@ -5766,6 +5762,7 @@ func TestExecutor_Execute_GroupBy(t *testing.T) { defer c.Close() c.CreateField(t, "i", pilosa.IndexOptions{}, "general") c.CreateField(t, "i", pilosa.IndexOptions{}, "sub") + c.CreateField(t, "i", pilosa.IndexOptions{}, "tq", pilosa.OptFieldTypeTime("YMDH", "0")) c.CreateField(t, "i", pilosa.IndexOptions{}, "v", pilosa.OptFieldTypeInt(0, 1000)) c.ImportBits(t, "i", "general", [][2]uint64{ {10, 0}, @@ -5786,6 +5783,7 @@ func TestExecutor_Execute_GroupBy(t *testing.T) { {110, 2}, {110, 0}, }) + if _, err := c.GetNode(0).API.Query(context.Background(), &pilosa.QueryRequest{Index: "i", Query: `Set(0, v=10)`}); err != nil { t.Fatal(err) } else if _, err := c.GetNode(0).API.Query(context.Background(), &pilosa.QueryRequest{Index: "i", Query: `Set(1, v=100)`}); err != nil { @@ -6198,6 +6196,38 @@ func TestExecutor_Execute_GroupBy(t *testing.T) { ) }) + // Create some time-quantum data: + c.Query(t, "i", "Set(0, tq=1, 2022-01-01T01:01)") + c.Query(t, "i", "Set(1, tq=1, 2021-01-01T01:01)") + t.Run("GroupByWithTime", func(t *testing.T) { + expected := map[string][]pilosa.GroupCount{ + // no time specified + "GroupBy(Rows(tq), Rows(general))": { + {Group: []pilosa.FieldRow{{Field: "tq", RowID: 1}, {Field: "general", RowID: 10}}, Count: 2}, + }, + // time specified but includes all data + "GroupBy(Rows(tq, from=2020-01-01T01:01), Rows(general))": { + {Group: []pilosa.FieldRow{{Field: "tq", RowID: 1}, {Field: "general", RowID: 10}}, Count: 2}, + }, + // same but in a different order + "GroupBy(Rows(general), Rows(tq, from=2020-01-01T01:01))": { + {Group: []pilosa.FieldRow{{Field: "general", RowID: 10}, {Field: "tq", RowID: 1}}, Count: 2}, + }, + // time excludes any data + "GroupBy(Rows(general), Rows(tq, from=2022-01-01T01:01))": { + {Group: []pilosa.FieldRow{{Field: "general", RowID: 10}, {Field: "tq", RowID: 1}}, Count: 1}, + }, + // limit excludes all data + "GroupBy(Rows(general), Rows(tq, from=2023-01-01T01:01))": {}, + } + + for query, want := range expected { + results := c.Query(t, "i", query).Results[0].(*pilosa.GroupCounts).Groups() + t.Logf("query %q", query) + test.CheckGroupBy(t, want, results) + } + + }) } for _, size := range []int{1, 3} { t.Run(fmt.Sprintf("%d_nodes", size), func(t *testing.T) { diff --git a/field.go b/field.go index 8c404068c..df87eea91 100644 --- a/field.go +++ b/field.go @@ -968,6 +968,55 @@ func (f *Field) TimeQuantum() TimeQuantum { return f.options.TimeQuantum } +// viewsByTimeRange is a wrapper on the non-method viewsByTimeRange, which +// computes views for a specific field for a given time range. The difference +// is that, as a Field operation, it can return "standard" for a view that +// covers the whole time range, if the field supports a standard view, and +// can automatically coerce from/to times to match the actual range present +// in the field. +func (f *Field) viewsByTimeRange(from, to time.Time) (views []string, err error) { + // If we can't find time views at all, we'll yield "standard" regardless. + // It's the least-bad answer, I think. + q := f.TimeQuantum() + if q == "" { + return []string{viewStandard}, nil + } + + // Get min/max based on existing views. + var vs []string + for _, v := range f.views() { + vs = append(vs, v.name) + } + min, max := minMaxViews(vs, q) + + // If min/max are empty, there were no time views. + if min == "" || max == "" { + return []string{viewStandard}, nil + } + + wasZero := from.IsZero() && to.IsZero() + // Convert min/max from string to time.Time. + minTime, err := timeOfView(min, false) + if err != nil { + return nil, errors.Wrapf(err, "getting min time from view: %s", min) + } + if from.IsZero() || from.Before(minTime) { + from = minTime + } + + maxTime, err := timeOfView(max, true) + if err != nil { + return nil, errors.Wrapf(err, "getting max time from view: %s", max) + } + if to.IsZero() || to.After(maxTime) { + to = maxTime + } + if (wasZero || (from == minTime && to == maxTime)) && !f.Options().NoStandardView { + return []string{viewStandard}, nil + } + return viewsByTimeRange(viewStandard, from, to, q), nil +} + // RowTime gets the row at the particular time with the granularity specified by // the quantum. func (f *Field) RowTime(tx Tx, rowID uint64, time time.Time, quantum string) (*Row, error) { diff --git a/field_internal_test.go b/field_internal_test.go index fb2f01060..268ca95d6 100644 --- a/field_internal_test.go +++ b/field_internal_test.go @@ -911,3 +911,70 @@ func TestField_SaveMeta(t *testing.T) { t.Fatalf("expected value after reopen to be: %d, got: %d", val, rslt) } } + +func TestFieldViewsByTimeRange(t *testing.T) { + f := OpenField(t, OptFieldTypeTime("YMD", "0", false)) + for _, date := range []string{ + // a handful of YMD parameters describing dates that we could have data for + "2021", + "202112", + "20211229", + "20211230", + "20211231", + "2022", + "202201", + "20220101", + "20220102", + } { + _, err := f.createViewIfNotExists(viewStandard + "_" + date) + if err != nil { + t.Fatalf("creating view for %s: %v", date, err) + } + } + var testCases = []struct { + from, to string + expected []string + }{ + {"", "", []string{"standard"}}, + {"2020-12-31T00:00", "2023-01-03T00:00", []string{"standard"}}, + {"2021-01-01T00:00", "2022-01-01T00:00", []string{"standard_2021"}}, + {"2021-01-01T00:00", "2022-01-02T00:00", []string{"standard_2021", "standard_20220101"}}, + {"", "2022-01-02T00:00", []string{"standard_2021", "standard_20220101"}}, + {"2021-12-01T00:00", "", []string{"standard_202112", "standard_2022"}}, + {"2021-12-30T00:00", "2022-02-01T00:00", []string{"standard_20211230", "standard_20211231", "standard_202201"}}, + } + for _, tc := range testCases { + t.Logf("checking %q to %q", tc.from, tc.to) + var fromTime, toTime time.Time + var err error + if tc.from != "" { + fromTime, err = time.Parse("2006-01-02T15:04", tc.from) + if err != nil { + t.Fatalf("invalid time %q: %v", tc.from, err) + } + } + if tc.to != "" { + toTime, err = time.Parse("2006-01-02T15:04", tc.to) + if err != nil { + t.Fatalf("invalid time %q: %v", tc.to, err) + } + } + views, err := f.viewsByTimeRange(fromTime, toTime) + if err != nil { + t.Fatalf("unexpected error getting views for %s-%s: %v", tc.from, tc.to, err) + } + for i, v := range tc.expected { + if len(views) <= i { + t.Fatalf("expected view %q, didn't get it", v) + } else { + if views[i] != v { + t.Fatalf("expected view %q, got %q", v, views[i]) + } + } + } + if len(views) > len(tc.expected) { + t.Fatalf("unexpected view %q", views[len(tc.expected)]) + } + t.Logf("views: %v", views) + } +} From 74ab06f5d3afd49407a8e7cde9cb42adc830eab4 Mon Sep 17 00:00:00 2001 From: Todd Gruben Date: Mon, 11 Apr 2022 08:58:39 -0500 Subject: [PATCH 2/7] expose etcd ttl timeout period --- ctl/server.go | 1 + 1 file changed, 1 insertion(+) diff --git a/ctl/server.go b/ctl/server.go index 497c0087c..ff2f06862 100644 --- a/ctl/server.go +++ b/ctl/server.go @@ -52,6 +52,7 @@ func BuildServerFlags(cmd *cobra.Command, srv *server.Command) { flags.StringVar(&srv.Config.Etcd.APeerURL, "etcd.advertise-peer-address", srv.Config.Etcd.APeerURL, "Advertise peer address. If not provided, uses the listen peer address.") flags.StringVar(&srv.Config.Etcd.ClusterURL, "etcd.cluster-url", srv.Config.Etcd.ClusterURL, "Cluster URL to join.") flags.StringVar(&srv.Config.Etcd.InitCluster, "etcd.initial-cluster", srv.Config.Etcd.InitCluster, "Initial cluster name1=apurl1,name2=apurl2") + flags.Int64Var(&srv.Config.Etcd.HeartbeatTTL, "etcd.heartbeat-ttl", srv.Config.Etcd.HeartbeatTTL, "Timeout used to determine cluster status") // External postgres database for ExternalLookup flags.StringVar(&srv.Config.LookupDBDSN, "lookup-db-dsn", "", "external (postgres) database DSN to use for ExternalLookup calls") From 66def7678cad25549898dd45c3ee9d0c5433c71b Mon Sep 17 00:00:00 2001 From: reesporte Date: Fri, 8 Apr 2022 10:32:02 -0500 Subject: [PATCH 3/7] Implement ToRowser interface for DistinctTimestamp Because the ToRowser interface was not implemented for DistinctTimestamp, there was a error when using the GRPC endpoint to call Distinct(All(), field=ts). Implementing the ToRowser interface for DistinctTimestamp solves that problem. Related to SUP-210: WebUI, Python - Distinct() does not work for Timestamp field --- executor.go | 28 +++++++++++++++++++++ executor_test.go | 63 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 91 insertions(+) diff --git a/executor.go b/executor.go index ade3b9001..c7b9d884f 100644 --- a/executor.go +++ b/executor.go @@ -1587,6 +1587,34 @@ type DistinctTimestamp struct { Name string } +var _ proto.ToRowser = DistinctTimestamp{} + +// ToRows implements the ToRowser interface. +func (d DistinctTimestamp) ToRows(callback func(*proto.RowResponse) error) error { + for _, ts := range d.Values { + row := &proto.RowResponse{ + Headers: []*proto.ColumnInfo{ + { + Name: d.Name, + Datatype: "timestamp", + }, + }, + Columns: []*proto.ColumnResponse{ + { + ColumnVal: &proto.ColumnResponse_TimestampVal{ + TimestampVal: ts, + }, + }, + }, + } + if err := callback(row); err != nil { + return errors.Wrap(err, "calling callback") + } + } + + return nil +} + // Union returns the union of the values of `d` and `other` func (d *DistinctTimestamp) Union(other DistinctTimestamp) DistinctTimestamp { both := map[string]string{} diff --git a/executor_test.go b/executor_test.go index 069f27b7b..bdcef2ee8 100644 --- a/executor_test.go +++ b/executor_test.go @@ -8546,3 +8546,66 @@ func MinMaxTimestampNodeTester(t *testing.T, numNodes int) { t.Fatalf("incorrect max timestamp val. expected: %v, got %v\n", expected, min) } } + +// DistinctTimestamp ToRows should properly encode the timestamp +func TestDistinctTimestampToRows(t *testing.T) { + d := pilosa.DistinctTimestamp{ + Values: []string{ + "2022-03-24T12:08:37Z", + "2022-03-24T12:08:47Z", + "2022-03-24T12:08:57Z", + }, + Name: "timestamp", + } + + expectedHeaders := []*proto.ColumnInfo{ + { + Name: d.Name, + Datatype: "timestamp", + }, + } + expected := []*proto.RowResponse{ + { + Headers: expectedHeaders, + Columns: []*proto.ColumnResponse{ + { + ColumnVal: &proto.ColumnResponse_TimestampVal{ + TimestampVal: "2022-03-24T12:08:37Z", + }, + }, + }, + }, + { + Headers: expectedHeaders, + Columns: []*proto.ColumnResponse{ + { + ColumnVal: &proto.ColumnResponse_TimestampVal{ + TimestampVal: "2022-03-24T12:08:47Z", + }, + }, + }, + }, + { + Headers: expectedHeaders, + Columns: []*proto.ColumnResponse{ + { + ColumnVal: &proto.ColumnResponse_TimestampVal{ + TimestampVal: "2022-03-24T12:08:57Z", + }, + }, + }, + }, + } + + rows := []*proto.RowResponse{} + d.ToRows(func(r *proto.RowResponse) error { + rows = append(rows, r) + return nil + }) + + for i, row := range rows { + if !reflect.DeepEqual(row, expected[i]) { + t.Errorf("expected %v, got %v", expected[i], row) + } + } +} 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 4/7] 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 5/7] 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 6/7] 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 7/7] 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)) } })