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

CORE-210 add can_access action #1604

Merged
merged 8 commits into from
Dec 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,15 @@ The “owner” role of a resource generally will include delete action and acti

Resource types define the set of available actions for all resources of that type. It also defines a set of roles and their associated actions. Roles are useful because it can be cumbersome to deal with granular actions and as a point of extensibility (when new actions are added to resource types, they can be added to roles as well, effectively adding the action to all resources with that role). Creating and maintaining resource types is achieved through [configuration](src/main/resources/reference.conf).

#### Resource Type Configuration
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

Configuration options for resource types are defined in the `resourceTypes` section of the configuration file. Each resource type has the following fields:
* `actionPatterns` - a set of regex patterns that are used to document and validate the actions available for resources of this type
* `roles` - a set of roles that are available for resources of this type. Roles are a collection of actions. Both roles and actions can be assigned to resource policies, but it is highly recommended to use roles because they are easier to change and affect all resources with that role as opposed to updating policies to add new actions.
* `ownerRoleName` - the name of the role that is considered the "owner" role for all resources of this type. All resources must have a policy with this role or have a parent.
* `reuseIds` - whether to allow reusing ids when creating resources of this type. This is important to prevent when using auth domains because users should not be able to delete then recreate a resource in Sam omitting the auth domain. Should be false when using UUIDs for Sam resource ids. Default is false.
* `allowLeaving` - whether to allow users to leave resources of this type, otherwise an owner must remove them. Default is false.
* `prerequisiteAction` - an optional action that must be granted before a user can perform any other actions on a resource of this type. Useful for resources that require some kind of access to a parent resource before accessing a child.

### Public Policies
There are some cases where it is desirable to grant actions or roles to all authenticated users. For example, granting read-only access to public workspaces. In this case a policy can be created that has the appropriate actions or roles and set to public. Resources with public policies show up when listing resources for a user. For this reason it is not always desirable to allow everyone to make public policies. Again, the example is public workspaces. Public workspaces show up for everyone and should be curated.

Expand All @@ -62,7 +71,7 @@ Group - Create, delete, read, list, add/remove users and groups. Nested groups a

### Built In Actions
* read_policies - may read all policies of a resource
* alter_policies - may change any policy of a resource
* alter_policies - may add or change any policy of a resource, use sparingly, prefer share_policy below for more control over policy structure
* delete - may delete a resource
* share_policy::{policy name} - may add/remove members to/from specified policy of a resource
* read_policy::{policy name} - may read specified policy of a resource
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,4 +33,5 @@
<include file="changesets/20240701_add_group_version_and_last_synchronized_version.xml" relativeToChangelogFile="true"/>
<include file="changesets/20240809_sam_user_favorite_resources_table.xml" relativeToChangelogFile="true"/>
<include file="changesets/20240820_descendant_auth_domains.xml" relativeToChangelogFile="true"/>
<include file="changesets/20241205_prereq_action_column.xml" relativeToChangelogFile="true"/>
</databaseChangeLog>
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
<?xml version="1.0" encoding="UTF-8" standalone="no"?>
<databaseChangeLog logicalFilePath="dummy"
xmlns="http://www.liquibase.org/xml/ns/dbchangelog"
xmlns:ext="http://www.liquibase.org/xml/ns/dbchangelog-ext"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://www.liquibase.org/xml/ns/dbchangelog-ext http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-ext.xsd http://www.liquibase.org/xml/ns/dbchangelog http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-3.4.xsd">

<changeSet logicalFilePath="dummy" author="dvoet" id="rt_prereq_action_column">
<addColumn tableName="sam_resource_type">
<column name="prerequisite_action" type="VARCHAR">
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be a FK? I suppose the owner role isn't either so seems fine.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

making it a FK creates some circular headaches when creating resource types: insert the resource type first, then actions, then update the resource type. Not worth it IMO.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious as to why the prerequisite_action is a VARCHAR instead of an ENUM?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the value is not built-in, it comes from configuration

<constraints nullable="true"/>
</column>
</addColumn>
</changeSet>
</databaseChangeLog>
39 changes: 36 additions & 3 deletions src/main/resources/reference.conf
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,9 @@ resourceTypes = {
roleActions = ["compute"]
descendantRoles = {
google-project = ["notebook-user"]
notebook-cluster = ["accessor"]
kubernetes-app = ["accessor"]
persistent-disk = ["accessor"]
}
}
can-catalog = {
Expand Down Expand Up @@ -798,6 +801,9 @@ resourceTypes = {
}
notebook-cluster = {
actionPatterns = {
can_access = {
description = "necessary but not sufficient to perform any action on the notebook cluster"
}
status = {
description = "view notebook cluster status details and configuration"
}
Expand Down Expand Up @@ -826,17 +832,27 @@ resourceTypes = {
}
ownerRoleName = "creator"
roles = {
# the creator role is assigned directly to the notebook-cluster resource and does not include can_access
# but can_access is required to perform any action so must be granted via the parent resource using
# either the manager or accessor role
creator = {
roleActions = ["status", "connect", "delete", "read_policies", "stop_start", "modify", "set_parent", "get_parent"]
}
manager = {
roleActions = ["status", "delete", "read_policies"]
roleActions = ["status", "delete", "read_policies", "can_access"]
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I see you added descendantRoles for accessor but not manager (for both notebook-cluster and kubernetes-app). How is manager assigned to users? Is that assigned directly somewhere by Leo?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

manager is already a descendant role from workspace owner

accessor = {
roleActions = ["can_access"]
}
}
reuseIds = false
prerequisiteAction = "can_access"
}
persistent-disk = {
actionPatterns = {
can_access = {
description = "necessary but not sufficient to perform any action on the persistent disk"
}
read = {
description = "read metadata of persistent disk"
}
Expand All @@ -862,17 +878,27 @@ resourceTypes = {
}
ownerRoleName = "creator"
roles = {
# the creator role is assigned directly to the persistent-disk resource and does not include can_access
# but can_access is required to perform any action so must be granted via the parent resource using
# either the manager or accessor role
creator = {
roleActions = ["read", "attach", "modify", "delete", "read_policies", "set_parent", "get_parent"]
}
manager = {
roleActions = ["delete", "read", "read_policies"]
roleActions = ["delete", "read", "read_policies", "can_access"]
}
accessor = {
roleActions = ["can_access"]
}
}
reuseIds = false
prerequisiteAction = "can_access"
}
kubernetes-app = {
Copy link
Contributor

Choose a reason for hiding this comment

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

does kubernetes-app need prerequisiteAction = "can_access" too?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I assume you mean kubernetes-app-shared and if so, no because all permissions are already inherited. The other resources behave differently because there is a creator role that is assigned directly to a single user.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

actually, you were right, I missed it

actionPatterns = {
can_access = {
description = "necessary but not sufficient to perform any action on the kubernetes app"
}
delete = {
description = "delete kubernetes application"
}
Expand Down Expand Up @@ -901,14 +927,21 @@ resourceTypes = {
}
ownerRoleName = "creator"
roles = {
# the creator role is assigned directly to the kubernetes-app resource and does not include can_access
# but can_access is required to perform any action so must be granted via the parent resource using
# either the manager or accessor role
creator = {
roleActions = ["delete", "connect", "update", "status", "stop", "start", "read_policies", "set_parent"]
}
accessor = {
roleActions = ["can_access"]
}
manager = {
roleActions = ["delete", "status", "read_policies"]
roleActions = ["delete", "status", "read_policies", "can_access"]
}
}
reuseIds = false
prerequisiteAction = "can_access"
}
kubernetes-app-shared = {
actionPatterns = {
Expand Down
13 changes: 11 additions & 2 deletions src/main/resources/swagger/api-docs.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -4354,7 +4354,7 @@ components:
description: specification of a role for a resource
ResourceType:
required:
- actions
- actionPatterns
- name
- ownerRoleName
- roles
Expand All @@ -4363,7 +4363,7 @@ components:
name:
type: string
description: The name of the resource type
actions:
actionPatterns:
type: array
description: List of actions that can be performed on a resource of this
type
Expand All @@ -4378,6 +4378,15 @@ components:
type: string
description: The name of the role that can perform administrative functions
on a resource of this type
reuseIds:
type: boolean
description: Whether resource ids can be reused
allowLeaving:
type: boolean
description: Whether users can leave a resource of this type
prerequisiteAction:
type: string
description: An action that is required but not sufficient to perform any other action on a resource of this type
description: specification of a type of resource
RolesAndActions:
required:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,8 @@ object AppConfig {
config.as[Map[String, ResourceRole]](s"$uqPath.roles").values.toSet,
ResourceRoleName(config.getString(s"$uqPath.ownerRoleName")),
config.getBoolean(s"$uqPath.reuseIds"),
config.as[Option[Boolean]](s"$uqPath.allowLeaving").getOrElse(false)
config.as[Option[Boolean]](s"$uqPath.allowLeaving").getOrElse(false),
config.as[Option[String]](s"$uqPath.prerequisiteAction").map(ResourceAction)
)
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,8 @@ class PostgresAccessPolicyDAO(
resourceTypeToRoles.getOrElse(resourceTypeRecord.id, Set.empty),
resourceTypeRecord.ownerRoleName,
resourceTypeRecord.reuseIds,
resourceTypeRecord.allowLeaving
resourceTypeRecord.allowLeaving,
resourceTypeRecord.prerequisiteAction
)
}

Expand Down Expand Up @@ -434,14 +435,16 @@ class PostgresAccessPolicyDAO(

private def upsertResourceTypes(resourceTypes: Iterable[ResourceType])(implicit session: DBSession): Int = {
val resourceTypeTableColumn = ResourceTypeTable.column
val resourceTypeValues = resourceTypes.map(rt => samsqls"""(${rt.name}, ${rt.ownerRoleName}, ${rt.reuseIds}, ${rt.allowLeaving})""")
samsql"""insert into ${ResourceTypeTable.table} (${resourceTypeTableColumn.name}, ${resourceTypeTableColumn.ownerRoleName}, ${resourceTypeTableColumn.reuseIds}, ${resourceTypeTableColumn.allowLeaving})
val resourceTypeValues =
resourceTypes.map(rt => samsqls"""(${rt.name}, ${rt.ownerRoleName}, ${rt.reuseIds}, ${rt.allowLeaving}, ${rt.prerequisiteAction})""")
samsql"""insert into ${ResourceTypeTable.table} (${resourceTypeTableColumn.name}, ${resourceTypeTableColumn.ownerRoleName}, ${resourceTypeTableColumn.reuseIds}, ${resourceTypeTableColumn.allowLeaving}, ${resourceTypeTableColumn.prerequisiteAction})
values $resourceTypeValues
on conflict (${ResourceTypeTable.column.name})
do update
set ${resourceTypeTableColumn.ownerRoleName} = EXCLUDED.${resourceTypeTableColumn.ownerRoleName},
${resourceTypeTableColumn.reuseIds} = EXCLUDED.${resourceTypeTableColumn.reuseIds},
${resourceTypeTableColumn.allowLeaving} = EXCLUDED.${resourceTypeTableColumn.allowLeaving}""".update().apply()
${resourceTypeTableColumn.allowLeaving} = EXCLUDED.${resourceTypeTableColumn.allowLeaving},
${resourceTypeTableColumn.prerequisiteAction} = EXCLUDED.${resourceTypeTableColumn.prerequisiteAction}""".update().apply()
}

/** @param resource
Expand Down
Loading
Loading