V1 APIs for Silos, Identity Providers, and Users #2442

zephraph · 2023-02-27T17:20:07Z

Part of #2028

Implements the APIs for

Silos
Silo users
Identity providers

I've noted a big departure from the original API around silo users that I've taken here that I'd specifically like feedback on.

zephraph · 2023-03-01T19:35:50Z

nexus/src/external_api/http_entrypoints.rs

+#[endpoint {
+    method = GET,
+    path = "/v1/system/users",
+    tags = ["system"],
+}]


cc @davepacheco, @ahl, @jmpesp

This diverges from the initial implementation as discussed here: #1784 (comment).

As per the original implementation this would've been /v1/system/users/all. The problem (as I see it) with that approach is that it's divergent from our other endpoint definitions. The stated challenge we were trying to work around is that there are potentially other ways that we may want to look up a user other than ID. While I appreciate that challenge, I think there are other ways we can go about solving it.

My current suggestion (for looking up a user by external_id) is simply to add a filter to the list endpoint.

GET /v1/system/users?external_id={id}`

This would yield an array with 0 or 1 results. While not strictly as clean from a result perspective, this approach could be added to other endpoints which already implement ID or name lookups as a subroute of the list endpoint without requiring breaking path changes.

Bulk lookups were also mentioned. I feel like this could also be handled via filtering the list endpoint. One could envision an array query parameter like /v1/system/users?ids[]=xxx&ids[]=yyy.

I acknowledge that there may well be a strong counter argument here that having multiple operating modes in a single endpoint makes that endpoints implementation more complex. While acknowledging that, I'd also argue that we're simply talking about different types of filters here. Generally I think we'll want to give more robust filtering capabilities to users and this'll be something we should work towards adding anyway.

All that said, feed back is definitely welcome.

I have the same problem in #2423 with looking up groups by external ID. I agree with Justin that while it's a bit weird, treating it as a list filter is the best way.

I think I initially went with separate endpoints out of my general fear of scalably implementing arbitrary filtering. More specifically, if we choose not to implement arbitrary filtering, and instead implement only a handful of filters that are important (e.g., users in one group, user for an external id, all users, users for a set of ids), will the resulting endpoint restrict users in ways that they'll find surprising and annoying (like: "I can take a set of user ids or a group id, but not both")? I'm still worried about arbitrary filtering but I don't have a strong feeling either way on which makes a cleaner API.

Tangentially: I think we have generally avoided structured query parameters (i.e., where the backend synthesizes more complicated structures from the key-value pairs), but maybe that's just because we haven't had enough need for them. The main downside (to me) is that I don't think there's really a standard for it so it winds up being API-specific instead of predictable or conventional. But we're probably going to need them at some point so we may as well come up with a standard. In this case that could be ids[]=a&ids[]=b (as you suggested) or id=a&id=b or id='%5Ba%2C%20b%5D (filter field is url-encoded JSON) or even filter='%22%7B%5C%22id%5C%22%3A%5B%5C%22a%5C%22%2C%5C%22b%5C%22%5D%7D%22 (filter is url-encoded JSON).

I think I wouldn't worry too much about either of these here.

zephraph · 2023-03-01T19:46:02Z

nexus/src/app/silo.rs

+    pub fn silo_lookup<'a>(
+        &'a self,
+        opctx: &'a OpContext,
+        silo: &'a NameOrId,
+    ) -> LookupResult<lookup::Silo<'a>> {
+        match silo {
+            NameOrId::Id(id) => {
+                let silo =
+                    LookupPath::new(opctx, &self.db_datastore).silo_id(*id);
+                Ok(silo)
+            }
+            NameOrId::Name(name) => {
+                let silo = LookupPath::new(opctx, &self.db_datastore)
+                    .silo_name(Name::ref_cast(name));
+                Ok(silo)
+            }
+        }
+    }


This lookup mechanism differs from the normal lookups in that it doesn't take a selector. Once we've moved over to the V1 API I fully expect we'll simplify the selector implementations anyway so I think it's fine that this one case is slightly different.

I'm doing something similar here:
https://github.com/oxidecomputer/omicron/pull/2455/files#diff-882861bfb859d6d0519f20dfcb9abad82e23d52a6963f921fb144a8d9fc2f273R810

@zephraph

We need this endpoint to power a group detail page in the console where we fetch the group by ID and display its name and the list of users in it. As mentioned in a comment, groups have an `external_id` instead of a `name` and therefore don't implement `ObjectIdentity`, which makes doing a lookup by external ID a little less cute than lookup by name. `external_id` is unique per silo, so looking up by it with the silo implicit is perfectly possible in theory. _However_, unlike with `Name`, we do not (and probably cannot) enforce that the `external_id` of a group is _not_ a UUID, which means our usual technique of falling back to name only once the thing fails to parse as a UUID does not work. If an external ID was a UUID, there would be no way to fetch a group by it because we would always assume it was one of _our_ DB UUIDs. One option would be to add a query param that allows the caller to specify that the given identifier is meant to be treated as the group external ID and not our DB ID. Another option, which we can do in a followup, is what @zephraph suggested in #2442 (comment): implement the `external_id` filter on the list endpoint instead. Because `external_id` is unique per silo, we know this will return either 1 or 0 results. It's a bit odd but it is conceptually consistent and requires no shenanigans.

david-crespo · 2023-03-02T20:49:25Z

Some endpoints are missing deprecated = true:

This list is comprehensive, I went through them all (last one is not even from this PR).

david-crespo · 2023-03-06T16:49:15Z

nexus/src/app/silo.rs

+                saml_identity_provider: NameOrId::Id(_),
+                silo_selector: Some(_),
+            } => Err(Error::invalid_request("when providing provider as an ID, silo should not be specified")),
+            _ => Err(Error::invalid_request("provider should either be a UUID or silo should be specified"))


nittiest of nits

Suggested change

_ => Err(Error::invalid_request("provider should either be a UUID or silo should be specified"))

_ => Err(Error::invalid_request("either provider should be a UUID or silo should be specified"))

I'm 👍 to this, but the message is consistent between all the other lookup functions. Maybe we should make this change across all of them at the same time?

Also makes me wonder if the should just be a lookup error that wraps around these cases 🤔

Yeah, probably do a followup to change them all.

david-crespo · 2023-03-06T17:58:01Z

nexus/src/app/silo.rs

-            .fetch()
-            .await?;
+        // TODO-security: This should likely be fetch_for CreateChild on the silo
+        let (authz_silo, db_silo) = silo_lookup.fetch().await?;


wouldn't that auth check get done when you actually do the creating?

This is a bit of an imperfect process across the API. We do a lot of the authz calls twice. Once at the entrance of the nexus layer and again at the datastore layer. Sometimes the nexus layer has more authz checks because there are logical permissions that must be validated beyond just what the datastore layer would check. @davepacheco has mentioned potentially caching duplicatively authz queries.

The pattern here is fairly typical though. Do an authz check at the entrance of the nexus function.

david-crespo · 2023-03-06T18:17:51Z

nexus/src/app/silo.rs

-            .silo_name(silo_name)
-            .lookup_for(authz::Action::Read)
-            .await?;
+        let (authz_silo,) = silo_lookup.lookup_for(authz::Action::Read).await?;


As discussed in chat with @zephraph, we considered the possibility that this would succeed at looking up a user by ID as long as the specified silo exists, even if the silo doesn't match the user's ID (because the silo is technically not needed for the lookup if we have a user ID to look up directly). But there is an explicit check that the retrieved user's silo ID matches the one passed in:

omicron/nexus/src/app/silo.rs

Lines 156 to 158 in 7be926a

if db_silo_user.silo_id != authz_silo.id() {

return Err(authz_silo_user.not_found());

}

david-crespo

think we got em

zephraph added 7 commits February 27, 2023 12:19

WIP: Silos, identity providers, users

a7d5bc1

WIP: Update some local idp endpoints

a0b960e

Stub out remaining endpoints

2dd33e8

Fixup endpoints

3cbbf9f

Add new saml endpoints

0b5f8b0

Merge branch 'main' into rfd-322-silos

6708d5e

Add silo user endpoints

cdc3c47

zephraph commented Mar 1, 2023

View reviewed changes

zephraph changed the title ~~WIP: V1 APIs for Silos, Identity Providers, and Users~~ V1 APIs for Silos, Identity Providers, and Users Mar 1, 2023

zephraph requested review from ahl, davepacheco and jmpesp March 1, 2023 19:38

zephraph marked this pull request as ready for review March 1, 2023 19:39

zephraph commented Mar 1, 2023

View reviewed changes

zephraph added 2 commits March 1, 2023 15:44

Merge branch 'main' into rfd-322-silos

a8a56ef

Add Update idp tests

b120d3f

david-crespo mentioned this pull request Mar 2, 2023

/v1/groups/{group} #2455

Merged

zephraph added 4 commits March 3, 2023 13:13

Merge branch 'main' into rfd-322-silos

d8d7267

Add deprecations

970e4fa

Revert perm changes for now

45276dd

Fix failing tests due to permission issues

52478a2

david-crespo reviewed Mar 6, 2023

View reviewed changes

david-crespo approved these changes Mar 6, 2023

View reviewed changes

zephraph merged commit 457eb58 into main Mar 6, 2023

zephraph deleted the rfd-322-silos branch March 6, 2023 18:48

	_ => Err(Error::invalid_request("provider should either be a UUID or silo should be specified"))
	_ => Err(Error::invalid_request("either provider should be a UUID or silo should be specified"))

	if db_silo_user.silo_id != authz_silo.id() {
	return Err(authz_silo_user.not_found());
	}

V1 APIs for Silos, Identity Providers, and Users #2442

V1 APIs for Silos, Identity Providers, and Users #2442

Uh oh!

Conversation

zephraph commented Feb 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

david-crespo commented Mar 2, 2023 • edited by zephraph Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

david-crespo Mar 6, 2023 • 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!

david-crespo left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

zephraph commented Feb 27, 2023 •

edited

Loading

david-crespo commented Mar 2, 2023 •

edited by zephraph

Loading

david-crespo Mar 6, 2023 •

edited

Loading