Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add API for listing silo users #1261

Merged
merged 22 commits into from
Jun 25, 2022
Merged

add API for listing silo users #1261

merged 22 commits into from
Jun 25, 2022

Conversation

davepacheco
Copy link
Collaborator

@davepacheco davepacheco commented Jun 23, 2022

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
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
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
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
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
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
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
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
Copy link
Collaborator Author

davepacheco commented Jun 24, 2022

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
Copy link
Contributor

@david-crespo david-crespo left a comment

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