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

"Inherited" permissions not included in policy response #1326

Closed
david-crespo opened this issue Jun 30, 2022 · 5 comments
Closed

"Inherited" permissions not included in policy response #1326

david-crespo opened this issue Jun 30, 2022 · 5 comments

Comments

@david-crespo
Copy link
Contributor

When a user has read access on an org (for example) they get read access on projects under that org. But when you pull the policy on a project, they will not show up in there because their role is on the org, not on the project. That means the Access & IAM page in the console for that project can be empty, even while the current user can clearly see the project because they're looking at it.

Not sure what the right solution is here, but it's a big problem on the frontend. The client could pull the org and silo permissions and merge them somehow, but that really feels like API logic. Since a given resources has a pretty short chain of parent resources, pulling the policy on each resource in the chain (e.g., project, org, and silo) might not be too bad and could be done in a single query. Maybe more interesting is figuring out how to represent that in the response in a reasonable way.

@davepacheco
Copy link
Collaborator

Yeah, this gets a little gnarly: when you PUT the policy, do you have to include those inherited role assignments? Do we allow the PUT resource to look very different than the GET one? Or maybe we make it a separate top-level property ("inherited_role_assignments") and model this like a PUT with whatever content-type means "if I leave a property out then I'm not changing it", and we don't allow you to change that property?

I had assumed the API would report the low-level facts and the console would be responsible for synthesizing this view but I can see why that might not be great. I'm curious what other systems do here.

@david-crespo
Copy link
Contributor Author

True, the PUT is a bigger problem.

Just like the console is now pretending role assignment is an exclusive choice between read, write, and admin, we can have the console do that aggregation to see how it feels and then think about pulling it into the API.

@david-crespo
Copy link
Contributor Author

david-crespo commented Jun 30, 2022

Perhaps of interest:

Example

For project Access & IAM page, instead of only pulling <...>/projects/:projectId/policy and showing that, we also pull the policy for the org and the silo, and combine them somehow. This is a bit of design challenge because we want to indicate where a given permission came from, and if a given user has entries at multiple levels we need to show that somehow too. This complexity hints at why all this might be better as API logic — the API can use the same logic it uses to actually resolve the permissions when it decides how to aggregate them into a synthetic policy.

oxidecomputer/console#1024

@davepacheco
Copy link
Collaborator

This complexity hints at why all this might be better as API logic — the API can use the same logic it uses to actually resolve the permissions when it decides how to aggregate them into a synthetic policy.

It seems likely that the console will want to know which permissions came from where in order to display it usefully, right? With separate endpoints, it's clear where each permission came from. Of course, we can also add the source information to the response, too, but that solution feels more complex to me on both sides.

Relatedly: right now, effective role assignments can come from the Project, Organization, Silo, or Fleet. Would we want to show Silo- and Fleet-level permissions here? Even though users don't necessarily even know about Fleets? Maybe this is moot after #1340. Although you might still have Silo admins that can see a Project, and I'm not sure if we want to show them. I ask because having the console make separate requests makes it easy to choose which sources it wants to show. (I guess it could also just filter those out of the aggregated endpoint. But this choice affects API users, too: an ordinary user using the API would see all this stuff about Silo and Fleet admins.)

@david-crespo
Copy link
Contributor Author

Closing as #1573 and oxidecomputer/console#1113 have us handling this on the client for now.

@david-crespo david-crespo closed this as not planned Won't fix, can't repro, duplicate, stale Aug 23, 2022
leftwo pushed a commit that referenced this issue Jun 26, 2024
Added a new package, crucible-dtrace that pulls from buildomat a package
that contains a set of DTrace scripts.  These scripts are extracted into
the global zone at /opt/oxide/crucible_dtrace/

Update Crucible to latest includes these updates:
Clean up dependency checking, fixing space leak (#1372)
Make a DTrace package (#1367)
Use a single context in all messages (#1363)
Remove `DownstairsWork`, because it's redundant (#1371)
Remove `WorkState`, because it's implicit (#1370)
Do work immediately upon receipt of a job, if possible (#1366)
Move 'do work for one job' into a helper function (#1365)
Remove `DownstairsWork` from map when handling it (#1361)
Using `block_in_place` for IO operations (#1357)
update omicron deps; use re-exported dropshot types in oximeter-producer configuration (#1369)
Parameterize more tests (#1364)
Misc cleanup, remove sqlite references. (#1360)
Fix `Extent::close` docstring (#1359)
Make many `Region` functions synchronous (#1356)
Remove `Workstate::Done` (unused) (#1355)
Return a sorted `VecDeque` directly (#1354)
Combine `proc_frame` and `do_work_for` (#1351)
Move `do_work_for` and `do_work` into `ActiveConnection` (#1350)
Support arbitrary Volumes during replace compare (#1349)
Remove the SQLite backend (#1352)
Add a custom timeout for buildomat tests (#1344)
Move `proc_frame` into `ActiveConnection` (#1348)
Remove `UpstairsConnection` from `DownstairsWork` (#1341)
Move Work into ConnectionState (#1340)
Make `ConnectionState` an enum type (#1339)
Parameterize `test_repair.sh` directories (#1345)
Remove `Arc<Mutex<Downstairs>>` (#1338)
Send message to Downstairs directly (#1336)
Consolidate `on_disconnected` and `remove_connection` (#1333)
Move disconnect logic to the Downstairs (#1332)
Remove invalid DTrace probes. (#1335)
Fix outdated comments (#1331)
Use message passing when a new connection starts (#1330)
Move cancellation into Downstairs, using a token to kill IO tasks (#1329)
Make the Downstairs own per-connection state (#1328)
Move remaining local state into a `struct ConnectionState` (#1327)
Consolidate negotiation + IO operations into one loop (#1322)
Allow replacement of a target in a read_only_parent (#1281)
Do all IO through IO tasks (#1321)
Make `reqwest_client` only present if it's used (#1326)
Move negotiation into Downstairs as well (#1320)
Update Rust crate clap to v4.5.4 (#1301)
Reuse a reqwest client when creating Nexus clients (#1317)
Reuse a reqwest client when creating repair client (#1324)
Add % to keep buildomat happy (#1323)
Downstairs task cleanup (#1313)
Update crutest replace test, and mismatch printing. (#1314)
Added more DTrace scripts. (#1309)
Update Rust crate async-trait to 0.1.80 (#1298)
leftwo added a commit that referenced this issue Jun 26, 2024
Update Crucible and Propolis to the latest

Added a new package, crucible-dtrace that pulls from buildomat a package
that contains a set of DTrace scripts. These scripts are extracted into the 
global zone at /opt/oxide/crucible_dtrace/

Crucible latest includes these updates:
Clean up dependency checking, fixing space leak (#1372) Make a DTrace
package (#1367)
Use a single context in all messages (#1363)
Remove `DownstairsWork`, because it's redundant (#1371) Remove
`WorkState`, because it's implicit (#1370)
Do work immediately upon receipt of a job, if possible (#1366) Move 'do
work for one job' into a helper function (#1365) Remove `DownstairsWork`
from map when handling it (#1361) Using `block_in_place` for IO
operations (#1357)
update omicron deps; use re-exported dropshot types in oximeter-producer
configuration (#1369) Parameterize more tests (#1364)
Misc cleanup, remove sqlite references. (#1360)
Fix `Extent::close` docstring (#1359)
Make many `Region` functions synchronous (#1356)
Remove `Workstate::Done` (unused) (#1355)
Return a sorted `VecDeque` directly (#1354)
Combine `proc_frame` and `do_work_for` (#1351)
Move `do_work_for` and `do_work` into `ActiveConnection` (#1350) Support
arbitrary Volumes during replace compare (#1349) Remove the SQLite
backend (#1352)
Add a custom timeout for buildomat tests (#1344)
Move `proc_frame` into `ActiveConnection` (#1348)
Remove `UpstairsConnection` from `DownstairsWork` (#1341) Move Work into
ConnectionState (#1340)
Make `ConnectionState` an enum type (#1339)
Parameterize `test_repair.sh` directories (#1345)
Remove `Arc<Mutex<Downstairs>>` (#1338)
Send message to Downstairs directly (#1336)
Consolidate `on_disconnected` and `remove_connection` (#1333) Move
disconnect logic to the Downstairs (#1332)
Remove invalid DTrace probes. (#1335)
Fix outdated comments (#1331)
Use message passing when a new connection starts (#1330) Move
cancellation into Downstairs, using a token to kill IO tasks (#1329)
Make the Downstairs own per-connection state (#1328) Move remaining
local state into a `struct ConnectionState` (#1327) Consolidate
negotiation + IO operations into one loop (#1322) Allow replacement of a
target in a read_only_parent (#1281) Do all IO through IO tasks (#1321)
Make `reqwest_client` only present if it's used (#1326) Move negotiation
into Downstairs as well (#1320)
Update Rust crate clap to v4.5.4 (#1301)
Reuse a reqwest client when creating Nexus clients (#1317) Reuse a
reqwest client when creating repair client (#1324) Add % to keep
buildomat happy (#1323)
Downstairs task cleanup (#1313)
Update crutest replace test, and mismatch printing. (#1314) Added more
DTrace scripts. (#1309)
Update Rust crate async-trait to 0.1.80 (#1298)

Propolis just has this one update:
Allow boot order config in propolis-standalone
---------

Co-authored-by: Alan Hanson <[email protected]>
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

No branches or pull requests

2 participants