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

clean up Polar file: minor functional changes #1113

Merged
merged 10 commits into from
May 25, 2022

Conversation

davepacheco
Copy link
Collaborator

This follows on #1112 but makes some (very small) changes to the policy file:

  • The Database shouldn't bother with roles, and more generally none of the synthetic resources should. I think it's confusing to use roles to define the policy for things where we don't actually support assigning those roles.
  • Organization should have a viewer role. I didn't bother with this initially but at this point it's unnecessarily error-prone not to have it because the definitions of Organization and Project were very subtly different from each other. Now, the definitions of Silo, Organization, and Project are basically identical aside from the parent relationship type.

@davepacheco davepacheco requested a review from plotnick May 24, 2022 22:43
@davepacheco davepacheco changed the base branch from main to polar-cleanup May 24, 2022 22:43
@davepacheco
Copy link
Collaborator Author

I initially thought I could remove the "external-authenticator" role, but this is one that we assign via the database.

@davepacheco davepacheco mentioned this pull request May 25, 2022
69 tasks
Base automatically changed from polar-cleanup to main May 25, 2022 00:59
@davepacheco davepacheco marked this pull request as ready for review May 25, 2022 01:34
Copy link
Contributor

@plotnick plotnick left a comment

Choose a reason for hiding this comment

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

This change makes the roles across the resource hierarchy much more symmetric, which I think helps make them much more understandable.

"collaborator" if "admin";

# Permissions granted directly by roles on this resource
"list_children" if "collaborator";
"read" if "collaborator";
"list_children" if "viewer";
Copy link
Contributor

@plotnick plotnick May 25, 2022

Choose a reason for hiding this comment

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

Great. I mentioned the previous asymmetry in a comment on #1110, glad to see it go away.

Comment on lines +330 to 331
has_permission(actor: AuthenticatedActor, "modify", _resource: Database)
if actor = USER_DB_INIT;
Copy link
Contributor

Choose a reason for hiding this comment

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

Untested, but should work:

Suggested change
has_permission(actor: AuthenticatedActor, "modify", _resource: Database)
if actor = USER_DB_INIT;
has_permission(USER_DB_INIT, "modify", _resource: Database);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Neat! I'll try that in my follow-up where I'm adding some policy tests.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Interestingly, that gave me:

thread 'authz::policy_test::test_iam_roles' panicked at 'initializing Oso: loading Polar (Oso) config

Caused by:
    Invalid rule: has_permission(USER_DB_INIT, "modify", _resource: Database{}); Must match one of the following rule types:
    
    has_permission(actor: Actor{}, _permission: String{}, resource: Resource{});
    	Failed to match because: Parameter `USER_DB_INIT` expects a Actor type constraint.
    
    	USER_DB_INIT: Actor
     at line 331, column 1:
    	331: has_permission(USER_DB_INIT, "modify", _resource: Database);
    	     ^
    ', nexus/src/authz/context.rs:33:54

but if I make it:

has_permission(USER_DB_INIT: AuthenticatedActor, "modify", _resource: Database);

that seems to work.

@davepacheco davepacheco merged commit 7413425 into main May 25, 2022
@davepacheco davepacheco deleted the polar-cleanup-more-functional branch May 25, 2022 23:56
leftwo pushed a commit that referenced this pull request Jan 28, 2024
Crucible changes
Remove a superfluous copy during write serialization (#1087)
Update to progenitor v0.5.0, pull in required Omicron updates (#1115)
Update usdt to v0.5.0 (#1116)
Do not panic on reinitialize of a downstairs client. (#1114)
Bump (tracing-)opentelemetry(-jaeger) (#1113)
Make the Guest -> Upstairs queue fully async (#1086)
Switch to per-block ownership (#1107)
Handle timeout in the client IO task (#1109)
Enforce buffer alignment (#1106)
Block size buffers (#1105)
New dtrace probes and a counter struct in the Upstairs. (#1104)
Implement read decryption offloading (#1089)
Remove Arc + Mutex from Buffer (#1094)
Comment cleanup and rename of DsState::Repair -> Reconcile (#1102)
do not panic the dynamometer for OOB writes (#1101)
Allow dsc to start the downstairs in read-only mode. (#1098)
Use the omicron-zone-package methods for topo sorting (#1099)
Package with topological sorting (#1097)
Fix clippy lints in dsc (#1095)

Propolis changes:
PHD: demote artifact store logs to DEBUG, enable DEBUG on CI (#626)
PHD: fix missing newlines in serial.log (#622)
PHD: fix run_shell_command with multiline commands (#621)
PHD: fix `--artifact-directory` not doing anything (#618)
Update h2 dependency
Update Crucible (and Omicron) dependencies
PHD: refactor guest serial console handling (#615)
phd: add basic "migration-from-base" tests + machinery (#609)
phd: Ensure min disk size fits read-only parents (#611)
phd: automatically fetch `crucible-downstairs` from Buildomat (#604)
Mitigate behavior from illumos#16183
PHD: add guest adapter for WS2022 (#607)
phd: include error cause chain in failure output (#606)
add QEMU pvpanic ISA device (#596)
Add crucible-mem backend
Make crucible opt parsing more terse in standalone
leftwo added a commit that referenced this pull request Jan 29, 2024
Crucible changes
Remove a superfluous copy during write serialization (#1087) Update to
progenitor v0.5.0, pull in required Omicron updates (#1115) Update usdt
to v0.5.0 (#1116)
Do not panic on reinitialize of a downstairs client. (#1114) Bump
(tracing-)opentelemetry(-jaeger) (#1113)
Make the Guest -> Upstairs queue fully async (#1086) Switch to per-block
ownership (#1107)
Handle timeout in the client IO task (#1109)
Enforce buffer alignment (#1106)
Block size buffers (#1105)
New dtrace probes and a counter struct in the Upstairs. (#1104)
Implement read decryption offloading (#1089)
Remove Arc + Mutex from Buffer (#1094)
Comment cleanup and rename of DsState::Repair -> Reconcile (#1102) do
not panic the dynamometer for OOB writes (#1101) Allow dsc to start the
downstairs in read-only mode. (#1098) Use the omicron-zone-package
methods for topo sorting (#1099) Package with topological sorting
(#1097)
Fix clippy lints in dsc (#1095)

Propolis changes:
PHD: demote artifact store logs to DEBUG, enable DEBUG on CI (#626) 
PHD: fix missing newlines in serial.log (#622)
PHD: fix run_shell_command with multiline commands (#621) 
PHD: fix `--artifact-directory` not doing anything (#618) Update h2
dependency
Update Crucible (and Omicron) dependencies
PHD: refactor guest serial console handling (#615) 
phd: add basic "migration-from-base" tests + machinery (#609) 
phd: Ensure min disk size fits read-only parents (#611) 
phd: automatically fetch `crucible-downstairs` from Buildomat (#604)
Mitigate behavior from illumos#16183
PHD: add guest adapter for WS2022 (#607)
phd: include error cause chain in failure output (#606) add QEMU pvpanic
ISA device (#596)
Add crucible-mem backend
Make crucible opt parsing more terse in 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

Successfully merging this pull request may close these issues.

2 participants