feat: add a paginated organization members endpoint #16835

brettkolodny · 2025-03-06T22:11:49Z

Emyrk · 2025-03-06T23:25:53Z

coderd/coderd.go

+				r.Route("/paginated-members", func(r chi.Router) {
+					r.Get("/", api.paginatedMembers)
+				})


Really unfortunate we cannot just use the /members endpoint without it being a breaking change.

Since organizations are new, I think it's worth considering.

I was originally going to to augment the existing route to be paginated but @aslilac brought up the issue of backwards compatibility. I can circle back with her, and other stakeholders on this

if we were having this discussion like two weeks ago I think I'd agree with you @Emyrk, but 2.20 is out now and marketing organizations as stable. :\

Emyrk · 2025-03-06T23:43:49Z

coderd/database/dbauthz/dbauthz.go

@@ -3581,6 +3581,10 @@ func (q *querier) OrganizationMembers(ctx context.Context, arg database.Organiza
 	return fetchWithPostFilter(q.auth, policy.ActionRead, q.db.OrganizationMembers)(ctx, arg)
 }

+func (q *querier) PaginatedOrganizationMembers(ctx context.Context, arg database.PaginatedOrganizationMembersParams) ([]database.PaginatedOrganizationMembersRow, error) {
+	return fetchWithPostFilter(q.auth, policy.ActionRead, q.db.PaginatedOrganizationMembers)(ctx, arg)


Unfortunately this does not work.

By default, a member can only read themselves, not other org members. So if you were paginating 10 users at a time, and the user was the 12th. Your query would return 10 rows, all would fail the authz check, and you would get an empty set as your return.

You would need to offset = 2 to find yourself, but that is not intuitive.

Pagination is a general problem for authorization engines. The user is requesting "The next 10 elements I can access". If you cannot join the authz data into the query itself then:

Query 10 items, if some are blocked by the authz, fetch some more. Repeat until you get 10 or exhaust the table.

Fetch all elements, then filter (large table this is not doable)

Overfetch by some, say 50 elements. Keep filtering and re-fetching until you get 10.

These solutions kinda suck. So we have 2 methods in our codebase to solve this.

Quick and dirty

The first is the quick solution. We just restrict access to this endpoint to users who can read the entire table (with organization_id = @org_id). This functionally changes the query to only usable by a certain class of users. Regular members now get a Forbidden instead of a list of ["me"]. If we go this route, we just need to fix the page that currently displays just yourself in these cases.

func (q *querier) PaginatedOrganizationMembers(ctx context.Context, arg database.PaginatedOrganizationMembersParams) ([]database.PaginatedOrganizationMembersRow, error) { if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceOrganizationMember.InOrg(arg.OrganizationID)); err != nil { return nil, err } return q.db.PaginatedOrganizationMembers(ctx, arg) }

The correct solution

The more correct solution is to properly filter the list, so if there exists a user who can read a subset of members, this query still works.

A good example is workspaces

coder/coderd/database/dbauthz/dbauthz.go

Lines 3018 to 3024 in 902538c

func (q *querier) GetWorkspaces(ctx context.Context, arg database.GetWorkspacesParams) ([]database.GetWorkspacesRow, error) {

prep, err := prepareSQLFilter(ctx, q.auth, policy.ActionRead, rbac.ResourceWorkspace.Type)

if err != nil {

return nil, xerrors.Errorf("(dev error) prepare sql filter: %w", err)

}

return q.db.GetAuthorizedWorkspaces(ctx, arg, prep)

}

The function prepareSQLFilter actually creates WHERE clause expressions that return the truthy value of an authorize() call for the given user. So we apply the filter in SQL, and you can paginate through the rows you can access.

There is some extra code required, but we have precedent to copy from if we go this route.

I think we should just go the quick and dirty route in this case. We don't have any roles that grant subset access to members. It is also strange to request organization members, and only return a subset.

For workspaces/templates this makes sense, as the resources have access control. However in this case, I think it makes more sense to return all or nothing. And we can update the UI to be in line with this decision.

Thanks for the feedback/explanation! I agree that from a product perspective the quick and dirty route makes the most sense

aslilac · 2025-03-07T18:56:11Z

coderd/coderd.go

+				r.Route("/paginated-members", func(r chi.Router) {
+					r.Get("/", api.paginatedMembers)
+				})


Suggested change

r.Route("/paginated-members", func(r chi.Router) {

r.Get("/", api.paginatedMembers)

})

r.Get("/paginated-members", api.paginatedMembers)

I wouldn't bother with the subrouter here

I saw this was a pattern we were doing in other places so copied that. Will change

aslilac

I had an idea that could avoid breakage, avoid needing an extra endpoint, but that I'm not sure is actually a good idea:

what if we did just update the existing /members route, but only if specify some pagination params?

maybe that's too weird to have it return two different types, but it'd also be unfortunate to have to maintain these two routes independently

or maybe we should just make this breaking change, call it out in the release notes, and I'm over thinking it and being too cautious

aslilac · 2025-03-07T21:52:00Z

coderd/database/dbauthz/dbauthz_test.go


 		check.Args(database.PaginatedOrganizationMembersParams{
-			OrganizationID: uuid.UUID{},
+			OrganizationID: o.ID,
 			LimitOpt:       1,


I think this different check means we also don't need this field any more

Accidentally pushed some test code in the test so not sure which one you were looking at but that's needed.

brettkolodny · 2025-03-07T22:11:23Z

I had an idea that could avoid breakage, avoid needing an extra endpoint, but that I'm not sure is actually a good idea:

what if we did just update the existing /members route, but only if specify some pagination params?

maybe that's too weird to have it return two different types, but it'd also be unfortunate to have to maintain these two routes independently

or maybe we should just make this breaking change, call it out in the release notes, and I'm over thinking it and being too cautious

Steven and I had the same thought, but agreed that it's kinda jank, and also wouldn't play well with the generated docs. For now we are deprecating the route in favor of the new paginated option that can still return all members.

Emyrk · 2025-03-10T13:42:06Z

coderd/database/dbauthz/dbauthz.go

+	if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceOrganizationMember.InOrg(arg.OrganizationID)); err != nil {
+		return nil, err
+	}


Just throw a comment

Suggested change

if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceOrganizationMember.InOrg(arg.OrganizationID)); err != nil {

return nil, err

}

// Required to have permission to read all members in the organization

if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceOrganizationMember.InOrg(arg.OrganizationID)); err != nil {

return nil, err

}

Emyrk · 2025-03-10T13:43:47Z

coderd/database/dbauthz/dbauthz_test.go

+	s.Run("PaginatedOrganizationMembers", s.Subtest(func(db database.Store, check *expects) {
+		o := dbgen.Organization(s.T(), db, database.Organization{})
+
+		check.Args(database.PaginatedOrganizationMembersParams{
+			OrganizationID: o.ID,
+			LimitOpt:       1,


Can you add back the org member in the setup? And check it in the Returns? And set the limit to 0.

That was breaking in the dbmem because limit = 0 was not being treated as "no limit"

Emyrk · 2025-03-10T16:17:06Z

coderd/database/dbmem/dbmem.go

+		if len(tmp) >= int(arg.LimitOpt) {
+			break
+		}


I think this needs to check if the limitOpt is not 0. If it is zero, skip this check. To match the SQL

aslilac

feel free to wait for steven, but it looks good to me 👍

aslilac · 2025-03-10T17:45:23Z

coderd/database/dbauthz/setup_test.go

@@ -503,7 +503,7 @@ func asserts(inputs ...any) []AssertRBAC {
 				// Could be the string type.
 				actionAsString, ok := inputs[i+1].(string)
 				if !ok {
-					panic(fmt.Sprintf("action '%q' not a supported action", actionAsString))
+					panic(fmt.Sprintf("action '%T' not a supported action", inputs[i+1]))


nice catch 😂

brettkolodny added 5 commits March 6, 2025 17:43

feat: add paginated members route

cea4d3c

feat: add request and response types

e97c9db

feat: create new sql query and generate code

34003dc

fix: add dbauthz tests

9edb088

feat: implement endpoint

ef40967

github-actions bot assigned brettkolodny Mar 6, 2025

brettkolodny changed the title ~~Brett/org members pagination backend~~ feat: Add a paginated organization members endpoint Mar 6, 2025

fix: correct endpoint documentation

6aaddaf

brettkolodny changed the title ~~feat: Add a paginated organization members endpoint~~ feat: add a paginated organization members endpoint Mar 6, 2025

brettkolodny added 3 commits March 6, 2025 22:25

fix: lint errors

a724f48

fix: generated docs from new endpoint

4925592

fix: remove unneeded comments

902538c

brettkolodny marked this pull request as ready for review March 6, 2025 22:58

brettkolodny requested a review from Emyrk March 6, 2025 23:26

Emyrk reviewed Mar 6, 2025

View reviewed changes

aslilac reviewed Mar 7, 2025

View reviewed changes

brettkolodny added 3 commits March 7, 2025 20:46

wip

06ff95c

Merge branch 'main' into brett/org-members-pagination-backend

560305d

fix: incorrectly showing type in panic

76a4cbf

aslilac reviewed Mar 7, 2025

View reviewed changes

brettkolodny added 3 commits March 7, 2025 21:56

fix: update docs

737ac9f

fix: change dbauthz test back to proper one

8910df3

fix: un-nest route

0e579a2

brettkolodny requested review from Emyrk and aslilac March 7, 2025 22:49

Emyrk reviewed Mar 10, 2025

View reviewed changes

brettkolodny added 3 commits March 10, 2025 16:00

fix: add comment

73e356c

fix: update test to account for dbmem and check for returned member

24332f4

fix: update test to check for accurate count

7af0b60

Emyrk reviewed Mar 10, 2025

View reviewed changes

fix: make dbmem implementation match expected behaior around 0 limit

cdc0322

brettkolodny requested a review from Emyrk March 10, 2025 16:44

aslilac approved these changes Mar 10, 2025

View reviewed changes

brettkolodny merged commit 8c0350e into main Mar 10, 2025
35 checks passed

brettkolodny deleted the brett/org-members-pagination-backend branch March 10, 2025 18:42

github-actions bot locked and limited conversation to collaborators Mar 10, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add a paginated organization members endpoint #16835

feat: add a paginated organization members endpoint #16835

brettkolodny commented Mar 6, 2025 •

edited

Loading

Emyrk Mar 6, 2025

brettkolodny Mar 7, 2025

aslilac Mar 7, 2025

Emyrk Mar 6, 2025 •

edited

Loading

brettkolodny Mar 7, 2025

aslilac Mar 7, 2025

brettkolodny Mar 7, 2025

aslilac left a comment

aslilac Mar 7, 2025

brettkolodny Mar 7, 2025

brettkolodny commented Mar 7, 2025

Emyrk Mar 10, 2025

Emyrk Mar 10, 2025

Emyrk Mar 10, 2025

aslilac left a comment

aslilac Mar 10, 2025

	func (q *querier) GetWorkspaces(ctx context.Context, arg database.GetWorkspacesParams) ([]database.GetWorkspacesRow, error) {
	prep, err := prepareSQLFilter(ctx, q.auth, policy.ActionRead, rbac.ResourceWorkspace.Type)
	if err != nil {
	return nil, xerrors.Errorf("(dev error) prepare sql filter: %w", err)
	}
	return q.db.GetAuthorizedWorkspaces(ctx, arg, prep)
	}

feat: add a paginated organization members endpoint #16835

feat: add a paginated organization members endpoint #16835

Conversation

brettkolodny commented Mar 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Emyrk Mar 6, 2025 • edited Loading

Choose a reason for hiding this comment

Quick and dirty

The correct solution

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aslilac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brettkolodny commented Mar 7, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aslilac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brettkolodny commented Mar 6, 2025 •

edited

Loading

Emyrk Mar 6, 2025 •

edited

Loading