Enable deletion of Assertion with delete {domain}:assertion.{assertionId} permission #2902
Conversation
…nId} permission Signed-off-by: Takuya Matsumoto <takumats@lycorp.co.jp>
Signed-off-by: Takuya Matsumoto <takumats@lycorp.co.jp>
| @@ -159,7 +159,7 @@ resource Assertion DELETE "/domain/{domainName}/policy/{policyName}/assertion/{a | |||
| Int64 assertionId; //assertion id | |||
| String auditRef (header="Y-Audit-Ref"); //Audit param required(not empty) if domain auditEnabled is true. | |||
| String resourceOwner (header="Athenz-Resource-Owner"); //Resource owner for the request | |||
| authorize ("update", "{domainName}:policy.{policyName}"); | |||
| authenticate; | |||
There was a problem hiding this comment.
please update the comment section for the block to include what type of authorization statements will be carried out within the code.
| @@ -181,7 +181,7 @@ resource Assertion DELETE "/domain/{domainName}/policy/{policyName}/version/{ver | |||
| Int64 assertionId; //assertion id | |||
| String auditRef (header="Y-Audit-Ref"); //Audit param required(not empty) if domain auditEnabled is true. | |||
| String resourceOwner (header="Athenz-Resource-Owner"); //Resource owner for the request | |||
| authorize ("update", "{domainName}:policy.{policyName}"); | |||
| authenticate; | |||
There was a problem hiding this comment.
please update the comment section for the block to include what type of authorization statements will be carried out within the code.
| if (hasAccess(domain, "update", String.format("%s:%s%s", domain.getName(), POLICY_PREFIX, policyName), principal, null) == AccessStatus.ALLOWED) { | ||
| return true; | ||
| } | ||
| if (hasAccess(domain, "delete", String.format("%s:%s%d", domain.getName(), ASSERTION_PREFIX, assertionId), principal, null) == AccessStatus.ALLOWED) { |
There was a problem hiding this comment.
this is the wrong authorization for the command. assertion is not a top level object within the domain and there is no requirement that the id is unique across the domain. It happens to be that way with our implementation but someone else might define their own store implementation and only support unique assertion ids per policy. The authorization would be
authorize("delete", "{domainName}:policy.{policyName}.assertion.{assertionId}" )
| @@ -10589,6 +10607,16 @@ boolean isAllowedDeletePendingMembership(Principal principal, final String domai | |||
| return pendingMember != null && principal.getFullName().equals(pendingMember.getRequestPrincipal()); | |||
| } | |||
|
|
|||
| boolean isAllowedDeleteAssertion(Principal principal, final AthenzDomain domain, final String policyName, final Long assertionId) { | |||
| if (hasAccess(domain, "update", String.format("%s:%s%s", domain.getName(), POLICY_PREFIX, policyName), principal, null) == AccessStatus.ALLOWED) { | |||
There was a problem hiding this comment.
Instead of the String.format please use:
ResourceUtils.policyResourceName(domainName, policyName)
|
Any reason only the delete assertion API was modified? If you're willing to grant someone access to delete assertions in the policy, isn't there a use case to also do the same for the put assertion api? |
Because the authority for PutAssertion is effectively the same as that for updating the policy, we decided not to create a separate permission for PutAssertion. (Creating a PutAssertion permission would introduce a vulnerability by allowing the addition of arbitrary privileges.) |
Description
#2811
I have added a permission that allows only the deletion of a specific assertion. Deleting an assertion can be performed by anyone who has either the “update: {domainName}:policy.{policyName}” or “delete: {domainName}:assertion.{assertionId}” permission.
Contribution Checklist:
Attach Screenshots (Optional)