Skip to content

Conversation

david-crespo
Copy link
Contributor

@david-crespo david-crespo commented Oct 19, 2022

Followup to #1830, especially #1830 (comment), which correctly points out that there is no bound in principle on the number of groups a user can be in, which means groups need their own paginated endpoint.

Why this was a draft, when it was a draft

I made a new view SiloGroupMembership that just contains the group ID. Using the existing Group would require having the SiloGroup model on hand in order to get the display name from it. I only have a SiloGroupMembership model, which is a join table between users and groups and therefore only has the user ID (not needed) and the group ID.

This is goofy and not really like anything else we do. The response should really be a list of Group, not just IDs. I will try changing the datastore function to join on groups so we can do that instead.

@davepacheco
Copy link
Collaborator

Following up on #1830 here: this seems like a fine approach for now. The downside is that we're encoding server-side policy in the client -- i.e., how the roles compose and how that composition applies to various actions. Oso calls this general problem "client-side enforcement" and have support in their Python library for an alternative approach where the server includes in the response body a list of actions that the user is authorized for. That might be worth exploring in the future.

@david-crespo
Copy link
Contributor Author

Wow, extremely relevant links for me. Interestingly, their example doesn't think about it in terms of an API — the server is directly rendering the template, so they don't need to think about how to make the list of authorized actions available to the client. You could add some kind of optional meta tag to the resource response itself, or you could add a /actions endpoint to every resource. Both of those sound pretty ugly, which I guess is why we're doing it client-side for now.

@david-crespo david-crespo marked this pull request as ready for review October 19, 2022 22:01
@david-crespo david-crespo requested review from ahl and jmpesp October 19, 2022 22:09
@davepacheco
Copy link
Collaborator

I think they had a different example (that I can't find right now) where it was part of an API. But yeah, I don't think it's something we should try to deal with right now (but I figured I'd mention it in case someone was like "no really we should do this right now"). I don't think the Rust crate supports the query we need, and I suspect it depends on the "query filtering" work.

.inner_join(sgm::table.on(sgm::silo_group_id.eq(sg::id)))
.filter(sgm::silo_user_id.eq(actor.actor_id()))
.filter(sg::time_deleted.is_null())
.select(SiloGroup::as_returning())
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I confirmed with debug_query that this produces this (I commented out the paginated part because I don't know how to unbox it):

SELECT silo_group.id,
       silo_group.time_created,
       silo_group.time_modified,
       silo_group.silo_id,
       silo_group.external_id
  FROM silo_group
       INNER JOIN silo_group_membership ON
			silo_group_membership.silo_group_id
			= silo_group.id
 WHERE silo_group_membership.silo_user_id = $1
   AND (silo_group.time_deleted IS NULL);

@david-crespo david-crespo enabled auto-merge (squash) October 20, 2022 02:07
@david-crespo david-crespo merged commit e710452 into main Oct 20, 2022
@david-crespo david-crespo deleted the groups-redux branch October 20, 2022 02:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants