Skip to content

add API for listing silo users#1261

Merged
davepacheco merged 22 commits into
mainfrom
silo-users-list
Jun 25, 2022
Merged

add API for listing silo users#1261
davepacheco merged 22 commits into
mainfrom
silo-users-list

Conversation

@davepacheco

@davepacheco davepacheco commented Jun 23, 2022

Copy link
Copy Markdown
Collaborator

Fixes #1235.

This change:

  • renames the existing /users (which lists built-in users) to /users_builtin.
  • creates a new /users that lists Silo Users. These objects only have a uuid in them right now. They could eventually have external_id but I think that will depend on Support for SAML as a Silo IdP, part 2 #1210. Presumably we eventually want them to have display names too, but that too depends on figuring out if/how we get this as part of the SAML login. (Since other systems do JIT with SAML and presumably also want to include display names, I'm guessing some SAML IdPs provide this using a mechanism similar to what they do for groups, but I don't really know.)

CC @david-crespo I'm not sure if this change will wind up breaking the console flow you have because there's only an "id" now.

@davepacheco davepacheco mentioned this pull request Jun 23, 2022
69 tasks
@davepacheco davepacheco changed the base branch from main to pagination-cleanup June 24, 2022 04:48
@davepacheco

Copy link
Copy Markdown
Collaborator Author

This is ready for review but I'm leaving this as a draft because it depends on #1265.

@davepacheco davepacheco requested a review from david-crespo June 24, 2022 05:01
@davepacheco

Copy link
Copy Markdown
Collaborator Author

There's one issue I need to fix: currently, only privileged users can list Silo users. I imagine we'll want to make all users able to list users.

@david-crespo

Copy link
Copy Markdown
Contributor

There's one issue I need to fix: currently, only privileged users can list Silo users. I imagine we'll want to make all users able to list users.

Not sure. Any authenticated user? Feels wrong, but I can't think of a good reason why. Are you thinking because anyone can create a project, and therefore be an admin on that project, and then they have to list user to assign roles? Can anyone create a project?

@davepacheco

Copy link
Copy Markdown
Collaborator Author

It's possible today to have (useful) users that cannot create Projects. They could potentially have full control over Projects that were shared with them.

But I feel like "name" and "id" information about all users in the same Silo is both reasonable and probably expected. Without this, you couldn't even see who else has access to the same thing that you have access to.

@david-crespo

Copy link
Copy Markdown
Contributor

Without this, you couldn't even see who else has access to the same thing that you have access to.

Yeah, that makes sense. Wasn't sure whether this is expected. In the GitHub web UI I can't see this on a private repo for which I'm not an admin because they block access to the whole settings view. But it turns out you can still get the list of collaborators from the API:

gh api /repos/oxidecomputer/<repo>/collaborators | jq '.[] | { login, permissions }'

I wonder if there's a distinction to be made between listing users on a particular resource and enumerating all users?

@davepacheco

Copy link
Copy Markdown
Collaborator Author

I wonder if there's a distinction to be made between listing users on a particular resource and enumerating all users?

It's possible but tricky. I imagine we'll eventually add a /users/$uuid endpoint for looking up a specific user. It's not really feasible for that to check whether you and the requested user share any resources in common. We could provide that information when you list the policy (and not if you request the same user explicitly). It'd be a little inconsistent but maybe pragmatic.

It seems surprising to me that people would want to put two people in the same Silo that couldn't know about each other. Wouldn't you use different Silos for that? But if we're not sure, we can raise this with product.

Base automatically changed from pagination-cleanup to main June 24, 2022 16:22
@david-crespo

Copy link
Copy Markdown
Contributor

I was picturing something more at the endpoint level, like you can't get anything out of /users unless you have the right permissions, and then on the /<resource>/policy endpoint the check is simply whether you have read access to the resource. But I agree that it adds a lot of complication for dubious value. Listing users if you are in the silo seems reasonable. We can think about restricting it later if it comes up.

@davepacheco

davepacheco commented Jun 24, 2022

Copy link
Copy Markdown
Collaborator Author

Okay, this version lets all users see all users in their own Silo. I added this endpoint to the allowlist of things without authz test coverage because the test doesn't currently support endpoints that are totally public for all authenticated users. I will file a new ticket for this though because we have a couple of them now.

Edit: #1277.

@davepacheco davepacheco marked this pull request as ready for review June 24, 2022 21:00

@david-crespo david-crespo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Very straightforward.

@davepacheco davepacheco merged commit 6804078 into main Jun 25, 2022
@davepacheco davepacheco deleted the silo-users-list branch June 25, 2022 05:13
leftwo pushed a commit that referenced this pull request Apr 19, 2024
Propolis changes:
Update h2 dependency
Add NPT ops API definitions from illumos#15639
server: return better HTTP errors when not ensured (#649)

Crucible changes:
Make Region test suite generic across backends (#1263)
Remove async from now-synchronous functions (#1264)
Agent update to support cloning. (#1262)
Remove the Active → Faulted transition (#1260)
Avoid race condition in crutest rand-read/write (#1261)
Add Active -> Offline -> Faulted tests (#1257)
Reorganize dummy downstairs tests (#1253)
Switch to unbounded queues (#1256)
Add Upstairs session ID to dtrace stat probe, cleanup closure (#1254)
Panic instead of returning errors in unit tests (#1251)
Add a clone option to downstairs create (#1249)
leftwo added a commit that referenced this pull request Apr 19, 2024
Propolis changes:
Update h2 dependency
Add NPT ops API definitions from illumos#15639
server: return better HTTP errors when not ensured (#649)

Crucible changes:
Make Region test suite generic across backends (#1263) Remove async from
now-synchronous functions (#1264) Agent update to support cloning.
(#1262)
Remove the Active → Faulted transition (#1260)
Avoid race condition in crutest rand-read/write (#1261) Add Active ->
Offline -> Faulted tests (#1257)
Reorganize dummy downstairs tests (#1253)
Switch to unbounded queues (#1256)
Add Upstairs session ID to dtrace stat probe, cleanup closure (#1254)
Panic instead of returning errors in unit tests (#1251) Add a clone
option to downstairs create (#1249)

Co-authored-by: Alan Hanson <alan@oxide.computer>
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.

Endpoint to list silo users

2 participants