add ability to list all groups #612

joshuarubin · 2017-10-17T23:02:28Z

This is to satisfy #594

I'm not sure if the api.swagger.yaml file is complete/correct since the gen-swagger.sh script produces the uncommitted api.swagger.json file.

The API is also a little strange as the GET /warden/groups endpoint already existed and previously required a member param (and GET /warden/groups/<id> refers to the group id, not the member id).

Now GET /warden/groups OPTIONALLY uses the member param. If it exists, the behavior is the same as before so there are no API regressions.

However, if member does not exist, then it is assumed that the user wants to list all groups. In this case, the required policy resource changes (from rn:hydra:warden:groups:<member> to rn:hydra:warden:groups). Additionally, the limit and offset parameters are used if they exist. limit defaults to 500 if not specified (the same as GET /policies).

Signed-off-by: Joshua Rubin <[email protected]>

aeneasr · 2017-10-18T08:06:13Z

Hey @joshuarubin the way you solved it is perfect. It is common to have a resource endpoints /resources which lists everything, and then query options like /resources?owner=1234 to request a subset (similar to limit & offset).

The only thing I would like to change is to remove rn:hydra:warden:groups:<member> and only do rn:hydra:warden:groups AC.

I will now review your code changes and leave comments where they fit.

aeneasr · 2017-10-18T08:23:48Z

Damn, halfway through the review I noticed that you made this against the 0.9.x branch. I would much rather see it done against the 0.10.0 one, is that an issue for you?

joshuarubin · 2017-10-18T08:29:50Z

We would like to be able to get this functionality ASAP which would be less risky if we can stay on 0.9.x. However, I don't think it would be too hard to cherry pick the commits and get it working on master. If we can merge this into 0.9.x, I'll set up another PR to get it into master.

aeneasr · 2017-10-18T08:08:05Z

docs/api.swagger.yaml

@@ -1376,8 +1376,8 @@ paths:
  /warden/groups:
    get:
      description: >-
-        The subject making the request needs to be assigned to a policy
-        containing:
+        The subject making the request, if member is specified, needs to be


As discussed in my comment, I'd like to revert this and replace this with a policy check against rn:hydra:warden:groups

aeneasr · 2017-10-18T08:14:12Z

docs/api.swagger.yaml

-        The subject making the request needs to be assigned to a policy
-        containing:
+        The subject making the request, if member is specified, needs to be
+        assigned to a policy containing:


As discussed in my comment, replace this with one check to rn:hydra:warden:groups

aeneasr · 2017-10-18T08:16:38Z

docs/api.swagger.yaml

@@ -1403,8 +1420,8 @@ paths:
      tags:
        - warden
        - groups
-      summary: Find group IDs by member
-      operationId: findGroupsByMember
+      summary: Find group IDs


In fact, we're receiving the full group payload (no longer only their IDs). This isn't your fault, but mine since I forgot to update the header title. But since you're already at it, maybe you could change that too ... :)

This applies to the 0.10.x branch - for the 0.9.x branch this is correct.

aeneasr · 2017-10-18T08:17:25Z

warden/group/handler.go

@@ -40,11 +41,11 @@ func (h *Handler) SetRoutes(r *httprouter.Router) {
 	r.DELETE(GroupsHandlerPath+"/:id/members", h.RemoveGroupMembers)
 }

-// swagger:route GET /warden/groups warden groups findGroupsByMember
+// swagger:route GET /warden/groups warden groups findGroups


This should actually be renamed to listGroups

aeneasr · 2017-10-18T08:17:36Z

warden/group/handler.go

 //
-// Find group IDs by member
+// Find group IDs


List groups

For 0.9.x this should be List group IDs

aeneasr · 2017-10-18T08:26:16Z

warden/group/manager_sql.go

@@ -123,6 +125,28 @@ func (m *SQLManager) RemoveGroupMembers(group string, subjects []string) error {
 	return nil
 }

+func (m *SQLManager) ListGroups(limit, offset int64) ([]string, error) {
+	if limit < 0 {


I think it should be fine to set limit to 0 (or 1) if it's below 0

aeneasr · 2017-10-18T08:26:24Z

warden/group/manager_sql.go

+		return nil, errors.New("limit can't be less than 0")
+	}
+
+	if offset < 0 {


I think it should be fine to set offset to 0 if it's below 0

aeneasr · 2017-10-18T08:29:17Z

warden/group/manager_test.go

@@ -85,6 +85,8 @@ func connectToPG() {
 }

 func TestManagers(t *testing.T) {
+	t.Parallel()


This is nice, I totally did not know about that flag. That way, postgres and mysql will be run alongside. This should be everywhere. Thank you!

#617

aeneasr · 2017-10-18T08:29:40Z

warden/group/manager_sql.go

+		limit = 500
+	}
+
+	if err := m.DB.Select(&q, m.DB.Rebind("SELECT id from hydra_warden_group ORDER BY id LIMIT ? OFFSET ?"), limit, offset); err != nil {


I'm not sure about the order clause here, what it's goal?

ORDER BY is required to ensure that the paging is consistent.
https://www.postgresql.org/docs/8.2/static/queries-limit.html

Oh, I did not know that! Then this is fine!

aeneasr · 2017-10-18T08:30:13Z

warden/group/manager_memory.go

+		res = append(res, g.ID)
+	}
+
+	sort.Strings(res)


Sorting makes sense here because maps don't have an order.

aeneasr · 2017-10-18T08:31:50Z

We would like to be able to get this functionality ASAP which would be less risky if we can stay on 0.9.x. However, I don't think it would be too hard to cherry pick the commits and get it working on master. If we can merge this into 0.9.x, I'll set up another PR to get it into master.

I would love to see that. If you find the time, I'd also love to see that for #605 :)

Signed-off-by: Joshua Rubin <[email protected]>

joshuarubin · 2017-10-18T17:19:34Z

OK. Updated with changes from review. Still don't know how to generate the api.swagger.yaml so I hope it's correct.

aeneasr · 2017-10-19T07:28:59Z

Nice! I'll go over the changes one more time. Regarding swagger, check out this script from the 0.10.x branch. Please make sure to use the latest stable version of go-swagger.

aeneasr

Just a couple of more things, then good to go

aeneasr · 2017-10-19T07:30:35Z

warden/group/handler.go

@@ -40,11 +41,11 @@ func (h *Handler) SetRoutes(r *httprouter.Router) {
 	r.DELETE(GroupsHandlerPath+"/:id/members", h.RemoveGroupMembers)
 }

-// swagger:route GET /warden/groups warden groups findGroupsByMember
+// swagger:route GET /warden/groups warden groups findGroups


aeneasr · 2017-10-19T07:30:41Z

warden/group/handler.go

 //
-// Find group IDs by member
+// Find group IDs


aeneasr · 2017-10-19T07:30:59Z

warden/group/handler.go

 //
-// The subject making the request needs to be assigned to a policy containing:
+// The subject making the request, if member is specified, needs to be assigned to a policy containing:


This line and the following policy can be removed, as discussed in the comments

aeneasr · 2017-10-19T07:31:06Z

warden/group/handler.go

@@ -66,24 +77,65 @@ func (h *Handler) SetRoutes(r *httprouter.Router) {
 //       oauth2: hydra.groups
 //
 //     Responses:
-//       200: findGroupsByMemberResponse
+//       200: findGroupsResponse


aeneasr · 2017-10-19T07:31:59Z

warden/group/handler.go

+		return
+	}
+
+	offset, err := intFromQuery(r, "offset")


If you supply a default value, no error checking is requried and you can save the limit == 0 check!

Signed-off-by: Joshua Rubin <[email protected]>

joshuarubin · 2017-10-19T16:22:50Z

OK. Updated again.

aeneasr

I missed one thing, which is unfortunately ever so important. After that I hope we're good to go!

aeneasr · 2017-10-21T08:26:50Z

warden/group/manager_sql.go

+		offset = 0
+	}
+
+	var q []string


If no results are returned, this will be nil which in turn will return null in the JSON response (see https://play.golang.org/p/7H6y1t2mdd ). To avoid this please initialize the slice (q := []string{})

aeneasr · 2017-10-21T08:27:28Z

warden/group/manager_memory.go

+		return nil, nil
+	}
+
+	var res []string


If no results are returned, this will be nil which in turn will return null in the JSON response (see https://play.golang.org/p/7H6y1t2mdd ). To avoid this please initialize the slice (q := []string{})

Signed-off-by: Joshua Rubin <[email protected]>

joshuarubin · 2017-10-23T17:09:33Z

Nice catch. I think everything should be ready now.

aeneasr · 2017-10-23T19:33:28Z

Thank you!

aeneasr · 2017-10-23T19:34:28Z

https://github.com/ory/hydra/releases/tag/v0.9.16

* vendor: support dep (#606) * fix go-jose Signed-off-by: Joshua Rubin <[email protected]> * add Gopkg.toml Signed-off-by: Joshua Rubin <[email protected]> * add tests for dep Signed-off-by: Joshua Rubin <[email protected]> * fix glide build Signed-off-by: Joshua Rubin <[email protected]> * warden/groups: add ability to list all groups (#612) * add ability to list all groups Signed-off-by: Joshua Rubin <[email protected]> * update based on review comments Signed-off-by: Joshua Rubin <[email protected]> * a few more updates from review Signed-off-by: Joshua Rubin <[email protected]> * ensure group lists dont return nil Signed-off-by: Joshua Rubin <[email protected]> * support es256 Signed-off-by: Joshua Rubin <[email protected]> * update jwk tests Signed-off-by: Joshua Rubin <[email protected]>

add ability to list all groups

cefb7da

Signed-off-by: Joshua Rubin <[email protected]>

aeneasr reviewed Oct 18, 2017

View reviewed changes

aeneasr mentioned this pull request Oct 18, 2017

sql/postgres: wherever limit/offset is used, include ORDER BY clause #619

Closed

update based on review comments

2e07eb8

Signed-off-by: Joshua Rubin <[email protected]>

aeneasr reviewed Oct 19, 2017

View reviewed changes

a few more updates from review

df204a6

Signed-off-by: Joshua Rubin <[email protected]>

aeneasr requested changes Oct 21, 2017

View reviewed changes

ensure group lists dont return nil

6216ba6

Signed-off-by: Joshua Rubin <[email protected]>

aeneasr merged commit e248e83 into ory:v0.9.x Oct 23, 2017

add ability to list all groups #612

add ability to list all groups #612

Conversation

joshuarubin commented Oct 17, 2017

aeneasr commented Oct 18, 2017

aeneasr commented Oct 18, 2017

joshuarubin commented Oct 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aeneasr Oct 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aeneasr commented Oct 18, 2017

joshuarubin commented Oct 18, 2017

aeneasr commented Oct 19, 2017 • edited Loading

aeneasr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshuarubin commented Oct 19, 2017

aeneasr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshuarubin commented Oct 23, 2017

aeneasr commented Oct 23, 2017

aeneasr commented Oct 23, 2017

aeneasr Oct 18, 2017 •

edited

Loading

aeneasr commented Oct 19, 2017 •

edited

Loading