Skip to content

clean up Polar file: minor functional changes #1113

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

Merged
merged 10 commits into from
May 25, 2022
3 changes: 3 additions & 0 deletions nexus/src/authz/api_resources.rs
Original file line number Diff line number Diff line change
Expand Up @@ -391,6 +391,7 @@ impl ApiResourceWithRolesType for Organization {
pub enum OrganizationRoles {
Admin,
Collaborator,
Viewer,
}

impl db::model::DatabaseString for OrganizationRoles {
Expand All @@ -400,13 +401,15 @@ impl db::model::DatabaseString for OrganizationRoles {
match self {
OrganizationRoles::Admin => "admin",
OrganizationRoles::Collaborator => "collaborator",
OrganizationRoles::Viewer => "viewer",
}
}

fn from_database_string(s: &str) -> Result<Self, Self::Error> {
match s {
"admin" => Ok(OrganizationRoles::Admin),
"collaborator" => Ok(OrganizationRoles::Collaborator),
"viewer" => Ok(OrganizationRoles::Viewer),
_ => Err(anyhow!(
"unsupported Organization role from database: {:?}",
s
Expand Down
37 changes: 12 additions & 25 deletions nexus/src/authz/omicron.polar
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ has_role(actor: AuthenticatedActor, role: String, resource: Resource)
# - silo.viewer (can read most resources within the Silo)
# - organization.admin (complete control over an organization)
# - organization.collaborator (can manage Projects)
# - organization.viewer (can read most resources within the Organization)
# - project.admin (complete control over a Project)
# - project.collaborator (can manage all resources within the Project)
# - project.viewer (can read most resources within the Project)
Expand Down Expand Up @@ -164,22 +165,22 @@ resource Organization {
"read",
"create_child",
];
roles = [ "admin", "collaborator" ];
roles = [ "admin", "collaborator", "viewer" ];

# Roles implied by other roles on this resource
"viewer" if "collaborator";
"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.

"read" if "viewer";
"create_child" if "collaborator";
"modify" if "admin";

# Roles implied by roles on this resource's parent (Silo)
relations = { parent_silo: Silo };
"admin" if "collaborator" on "parent_silo";
"read" if "viewer" on "parent_silo";
"list_children" if "viewer" on "parent_silo";
"viewer" if "viewer" on "parent_silo";
}
has_relation(silo: Silo, "parent_silo", organization: Organization)
if organization.silo = silo;
Expand All @@ -206,7 +207,7 @@ resource Project {
# Roles implied by roles on this resource's parent (Organization)
relations = { parent_organization: Organization };
"admin" if "collaborator" on "parent_organization";
"viewer" if "list_children" on "parent_organization";
"viewer" if "viewer" on "parent_organization";
}
has_relation(organization: Organization, "parent_organization", project: Project)
if project.organization = organization;
Expand Down Expand Up @@ -253,7 +254,7 @@ has_relation(user: SiloUser, "silo_user", ssh_key: SshKey)
# of the API path (e.g., "/images") or as an implementation detail of the system
# (in the case of console sessions and "Database"). The policies are
# either statically-defined in this file or driven by role assignments on the
# Fleet.
# Fleet. None of these resources defines their own roles.
#

# Describes the policy for accessing "/images" (in the API)
Expand Down Expand Up @@ -320,25 +321,11 @@ resource Database {
# other general functions.
"modify"
];
roles = [
# All authenticated users get the "user" role, which grants the
# "query" permission. See above.
"user",

# The special "db-init" user gets the "init" role, which grants the
# additional "modify" permission.
"init"
];

# See above.
"query" if "user";

"user" if "init";
"modify" if "init";
}

# All authenticated users have the "user" role on the database.
has_role(_actor: AuthenticatedActor, "user", _resource: Database);
# All authenticated users have the "query" permission on the database.
has_permission(_actor: AuthenticatedActor, "query", _resource: Database);

# The "db-init" user is the only one with the "init" role.
has_role(actor: AuthenticatedActor, "init", _resource: Database)
has_permission(actor: AuthenticatedActor, "modify", _resource: Database)
if actor = USER_DB_INIT;
Comment on lines +330 to 331
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.

56 changes: 55 additions & 1 deletion nexus/src/db/fixed_data/role_builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
use lazy_static::lazy_static;
use omicron_common::api;

#[derive(Clone)]
#[derive(Clone, Debug)]
pub struct RoleBuiltinConfig {
pub resource_type: api::external::ResourceType,
pub role_name: &'static str,
Expand Down Expand Up @@ -67,6 +67,11 @@ lazy_static! {
},
ORGANIZATION_ADMINISTRATOR.clone(),
ORGANIZATION_COLLABORATOR.clone(),
RoleBuiltinConfig {
resource_type: api::external::ResourceType::Organization,
role_name: "viewer",
description: "Organization Viewer",
},
RoleBuiltinConfig {
resource_type: api::external::ResourceType::Project,
role_name: "admin",
Expand All @@ -84,3 +89,52 @@ lazy_static! {
},
];
}

#[cfg(test)]
mod test {
use super::BUILTIN_ROLES;
use crate::authz;
use crate::db::model::DatabaseString;
use omicron_common::api::external::ResourceType;
use strum::IntoEnumIterator;

#[test]
fn test_fixed_role_data() {
// Every role that's defined in the public API as assignable on a
// resource must have a corresponding entry in BUILTIN_ROLES above.
// The reverse is not necessarily true because we have some internal
// roles that are not exposed to end users.
check_public_roles::<authz::FleetRoles>(ResourceType::Fleet);
check_public_roles::<authz::SiloRoles>(ResourceType::Silo);
check_public_roles::<authz::OrganizationRoles>(
ResourceType::Organization,
);
check_public_roles::<authz::ProjectRoles>(ResourceType::Project);
}

fn check_public_roles<T>(resource_type: ResourceType)
where
T: std::fmt::Debug + DatabaseString + IntoEnumIterator,
{
for variant in T::iter() {
let role_name = variant.to_database_string();

let found = BUILTIN_ROLES.iter().find(|role_config| {
role_config.resource_type == resource_type
&& role_config.role_name == role_name
});
if let Some(found_config) = found {
println!(
"variant: {:?} found fixed data {:?}",
variant, found_config
);
} else {
panic!(
"found public role {:?} on {:?} with no corresponding \
built-in role",
role_name, resource_type
);
}
}
}
}
1 change: 1 addition & 0 deletions nexus/tests/integration_tests/roles_builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ async fn test_roles_builtin(cptestctx: &ControlPlaneTestContext) {
("fleet.viewer", "Fleet Viewer"),
("organization.admin", "Organization Administrator"),
("organization.collaborator", "Organization Collaborator"),
("organization.viewer", "Organization Viewer"),
("project.admin", "Project Administrator"),
("project.collaborator", "Project Collaborator"),
("project.viewer", "Project Viewer"),
Expand Down
1 change: 1 addition & 0 deletions nexus/tests/output/authz-roles-organization.txt
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
variant Admin: serialized form = admin
variant Collaborator: serialized form = collaborator
variant Viewer: serialized form = viewer
3 changes: 2 additions & 1 deletion openapi/nexus.json
Original file line number Diff line number Diff line change
Expand Up @@ -6643,7 +6643,8 @@
"type": "string",
"enum": [
"admin",
"collaborator"
"collaborator",
"viewer"
]
},
"OrganizationRolesPolicy": {
Expand Down