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

CORE-210 add can_access action #1604

merged 8 commits into from
Dec 10, 2024

Conversation

dvoet
Copy link
Collaborator

@dvoet dvoet commented Dec 5, 2024

DataBiosphere/leonardo#4808 must merge first

Ticket: https://broadworkbench.atlassian.net/browse/CORE-210

When a user loses access to a workspace, they need to lose all access to Leonardo compute resources as well. We will do this with a new concept in Sam called a prerequisite action. A prerequisite action is an action on a resource that is required but not sufficient to perform any other action on the same resource. In the case of Leo resources (notebook clusters, persistent disks and kubernetes apps), this action, can_access, is passed down from the containing workspace through the can-compute role which includes the accessor Leo role. It is also passed down through the workspace owner role which includes the manager role on the Leo resource. The net effect is that in order to do anything on these resources, the caller must be a writer with compute access or an owner of a workspace. If the caller loses that access, the Leo resources in the workspace continue to exist but are inaccessible by the caller. If the caller regains access, they regain access to the Leo resources as well.


PR checklist

  • I've followed the instructions if I've made any changes to the API, especially if they're breaking changes
  • I've filled out the Security Risk Assessment (requires Broad Internal network access) and attached the result to the JIRA ticket

@dvoet dvoet requested a review from marctalbott December 5, 2024 16:05
@dvoet dvoet requested a review from a team as a code owner December 5, 2024 16:05
@dvoet dvoet requested a review from davidangb December 5, 2024 16:05
Copy link
Contributor

@davidangb davidangb left a comment

Choose a reason for hiding this comment

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

👍 with one question inline

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


<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.

@dvoet dvoet requested review from LizBaldo and davidangb December 9, 2024 16:24
Comment on lines +65 to +66
def apply(rs: ResultSet, label: String): ResourceAction = nullSafe(rs.getString(label), ResourceAction)
def apply(rs: ResultSet, index: Int): ResourceAction = nullSafe(rs.getString(index), ResourceAction)
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 changes in this file were sparked because prerequisiteAction is optional and the prior code resulted in these functions returning ResourceAction(null) which is not valid. I fixed all the other functions in this file as well just in case.

Copy link
Contributor

@davidangb davidangb left a comment

Choose a reason for hiding this comment

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

one question about kubernetes-app config inline.

Is there any call for prerequisiteAction to be an array of actions instead of just a single action? That's probably too complicated, but if we want multiple we should do it now so we don't have to change the models later.

proactive 👍

}
}
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

@dvoet
Copy link
Collaborator Author

dvoet commented Dec 9, 2024

Is there any call for prerequisiteAction to be an array

I thought about this and could not decide if it should be implemented as AND (must have all prereqs) or OR (any prereq will suffice). So I kept is simple :)

I don't think updating the models later will be much of a problem

@@ -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.

❤️


<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.

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

@dvoet dvoet merged commit 77c2554 into develop Dec 10, 2024
26 checks passed
@dvoet dvoet deleted the connect_prereq branch December 10, 2024 19:21
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.

4 participants