Add list of group IDs to `/session/me` response #1830

david-crespo · 2022-10-17T19:44:49Z

This is meant to be a starting point for figuring out how this should work. It's not particularly good.

Problem

In the console we would like to hide or disable certain controls when the user does not have permission to use them. For example, a user with only the viewer role on an organization does not have the ability to create projects in that organization, and we prefer disabling the button (likely with a tooltip on hover) over letting them click the button and find out they can't create when the API comes back with a 403. So I need to know the user's role on the organization when I render the UI.

I plan to use data for the current user plus the policy for a given resource to determine the user's effective role on the resource. "Effective" is key because they could have roles from multiple sources — e.g., their user ID directly, group A they belong to, group B they belong to, or any of those from the policy on a parent resource 😓 — and the effective role from the API's point of view is the strongest role out of all of those.

Everything required to do that currently exists except there is no way of getting the current user's group memberships. This could be its own endpoint or it could be added to /session/me. I did the latter here because I don't really see why anyone would ever want to call one without the other, but I don't feel to strongly about it.

Questions/issues

Policy is implicit in app code

The existing endpoint silo_group_membership_for_user takes an authz::Silo and a user ID, and the auth check is for ListChildren on the silo. Assuming we want any user to be able to list their own groups, we cannot stick to this pattern for the "list by groups" endpoint, because the unprivileged user does not necessarily have any permissions on the silo. What I've done here instead is not have any authz check, pulling the user ID off the opctx. The implicit policy therefore is that any user can fetch their own groups. However, I noticed that this is only the second time we call opctx.authn.actor_required() in any db/datastore file. The first is session delete, which has a helpful comment by @davepacheco:

// We don't do a typical authz check here.  Instead, knowing that every
// user is allowed to delete their own session, the query below filters
// on the session's silo_user_id matching the current actor's id.
//
// We could instead model this more like other authz checks.  That would
// involve fetching the session record from the database, storing the
// associated silo_user_id into the `authz::ConsoleSession`, and having
// an Oso rule saying you can delete a session whose associated silo
// user matches the authenticated actor.  This would be a fair bit more
// complicated and more work at runtime work than what we're doing here.
// The tradeoff is that we're effectively encoding policy here, but it
// seems worth it in this case.

I think this logic also applies to what I've written, which means we should have a similar comment and we're ok.

New `SessionMe` response type

We use the existing User in a number of places, not just /session/me, and I don't think we want to clutter all those up with lists of groups (or maybe we do?) so we need a new response type for /session/me. This isn't that weird and I'm pretty sure we've talked about doing it before, though I can't find that discussion.

Should we list group IDs or `{ id, name }` objects?

For my purposes in the console (matching against a policy) I only need IDs, but I can certainly see us needing names sometime soon. For example, it might make sense to display a list of my groups on /settings/profile, which currently only shows the user's UUID.

david-crespo · 2022-10-17T19:59:03Z

nexus/src/external_api/console_api.rs

+            display_name: user.external_id,
+            silo_id: user.silo_id,
+            group_ids: groups.iter().map(|g| g.silo_group_id).collect(),
+        }))


I think views::SessionMe::new(user, groups) would make sense here, but I struggled to decide where to define that. The obviously place would be right next to the definition of SessionMe in views.rs, but that would be the first time anything in that file refers to DB models. Could be fine, or maybe it offends our sense of propriety. Generally we seem to prefer doing our impl From for these views next to the DB model, which to be honest is not what I prefer. I like to think of the view as "knowing about" the model but the model not knowing about the view.

In fact, there does not seem to be a way to import db into nexus/types/src/external_api/views.rs because it's a different crate and that would introduce a circular dependency. So it basically has to be defined here in the app, but I can't impl a constructor for it because it comes from another crate.

this seems to point to a fn Nexus::session_me(..) -> views::SessionMe, no?

I considered that too. The problem is the app layer methods all return DB models, not views, and it's up to the From impls to turn those into views in the entrypoints. This is a pretty rare case where we're making a view out of two models. I don't think we do that very often. So if it can't go in nexus/src/app, it would basically be a helper that sits right next to the route handler right here, at which point I figure we might as well inline it (leave as is) since it's not going to be used anywhere else.

david-crespo · 2022-10-17T20:38:57Z

nexus/types/src/external_api/views.rs

    pub silo_id: Uuid,
 }

+/// Client view of a [`User`] with some extra stuff that's useful to the console


love a comment that makes you go "are we sure we should be doing this?"

jmpesp · 2022-10-18T16:26:15Z

nexus/src/app/iam.rs

        Ok(db_silo_user)
    }

+    pub async fn silo_user_fetch_groups(


why not also put _for_self somewhere in this function? unless the idea is that it's implicit because of the function signature?

Nope, I must have mixed up some different names and thought I was matching the other one, but I see now it has self in the name. Fixing.

jmpesp · 2022-10-18T16:27:14Z

nexus/tests/integration_tests/console_api.rs

        }
    );

+    // TODO: fails bc user can't pull their own group memberships?


is this comment still true?

jmpesp · 2022-10-18T16:28:48Z

nexus/src/external_api/console_api.rs

+            display_name: user.external_id,
+            silo_id: user.silo_id,
+            group_ids: groups.iter().map(|g| g.silo_group_id).collect(),
+        }))


this seems to point to a fn Nexus::session_me(..) -> views::SessionMe, no?

jmpesp · 2022-10-18T16:30:24Z

I'm leaning towards 🚀 with the idea that we'll attempt putting together Oso policy to describe this at some point, even if it's a tradeoff we don't want to make now.

david-crespo · 2022-10-18T19:29:18Z

nexus/tests/integration_tests/console_api.rs

            id: USER_TEST_UNPRIVILEGED.id(),
            display_name: USER_TEST_UNPRIVILEGED.external_id.clone(),
            silo_id: DEFAULT_SILO.id(),
+            group_ids: vec![],


It would be cool to be able to add the user to a group, then refetch and see the group here. But I see that there is no endpoint for adding a user to a group and modification of group memberships only happens during JIT user create. A next-best thing would be if the built-in privileged and unprivileged users were in groups, so at least this vec would come back non-empty. @jmpesp any thoughts?

In nexus/tests/integration_tests/saml.rs, test_correct_saml_response_with_group_attributes tests that groups are JITed correctly, and test_post_saml_response calls /session/me to validate a successful SAML login. I'm not sure about the built-in users being part of some fake groups when a test(s) could be done there.

I see. In 0e61d09 I modified test_post_saml_response to include groups and assert that the groups are there on /session/me. One downside is that test didn't used to be about groups and now it is. Not sure if that is really a problem.

I think it's not a problem, though if the order of groups returned changes then that assert_eq will fire.

Add a cute helper for that in 52b9f46

ahl · 2022-10-19T17:53:54Z

openapi/nexus.json

+            "type": "string"
+          },
+          "group_ids": {
+            "type": "array",


is this bounded in some way? in general I think we try to avoid responses that are effectively unbounded in terms of their size.

Good point, and bad timing for the auto merge. I might revert it, or I might just make the necessary changes on top. I don't think there is a bound on the size of this list. The problem from the console point of view is that we really do want all the groups for the user.

Our standard approach is to make a separate paginated endpoint. I was trying to avoid that, but it's natural enough: the console can GET /session/me/groups or whatever with a large page size (default is 1000) to try to get all the groups. This would likely cover us for a long time, especially since we can increase the page size up to (I think) 10000 if someone wants to get weird and assign a ton of groups, and it's not too surprising if the console experience degrades somewhat when a user is in 10,000 groups. In this case, the degradation would be that when we determine client-side whether a user has a given permission (by comparing the user's ID and group IDs to a policy response), we will determine wrong if we are missing groups for the user.

On the other hand, this whole rigmarole is a point in favor of moving that logic server-side, letting the console ask for the current user's effective role on any resource.

Another option is to actually limit the number of group memberships and not paginate this.

david-crespo added 2 commits October 17, 2022 14:20

add group IDs to /session/me response

a0b8735

you win this round, clippy

7479c5d

david-crespo requested a review from jmpesp October 17, 2022 19:51

more smarter comment

8acee42

david-crespo commented Oct 17, 2022

View reviewed changes

david-crespo requested a review from davepacheco October 18, 2022 15:04

jmpesp reviewed Oct 18, 2022

View reviewed changes

address PR comments

5f0ae14

david-crespo commented Oct 18, 2022

View reviewed changes

david-crespo added 2 commits October 18, 2022 15:26

let's not be so casual in the comments

2950162

test that session me returns group IDs when it's supposed to

0e61d09

david-crespo mentioned this pull request Oct 18, 2022

Sort IAM role assignments with groups first, then by name oxidecomputer/console#1237

Merged

order agnostic vec equality helper

52b9f46

david-crespo enabled auto-merge (squash) October 19, 2022 16:49

david-crespo merged commit 2ea34d0 into main Oct 19, 2022

david-crespo deleted the session-me-groups branch October 19, 2022 17:48

ahl reviewed Oct 19, 2022

View reviewed changes

david-crespo mentioned this pull request Oct 19, 2022

Current user's groups, paginated this time #1840

Merged

Add list of group IDs to /session/me response #1830

Add list of group IDs to /session/me response #1830

Uh oh!

Conversation

david-crespo commented Oct 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Questions/issues

Policy is implicit in app code

New SessionMe response type

Should we list group IDs or { id, name } objects?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

david-crespo Oct 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

david-crespo Oct 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmpesp commented Oct 18, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Add list of group IDs to `/session/me` response #1830

Add list of group IDs to `/session/me` response #1830

david-crespo commented Oct 17, 2022 •

edited

Loading

New `SessionMe` response type

Should we list group IDs or `{ id, name }` objects?

david-crespo Oct 17, 2022 •

edited

Loading

david-crespo Oct 18, 2022 •

edited

Loading