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

Improvement/arsn 362 implicit deny #2162

Closed
wants to merge 2 commits into from

Conversation

KazToozs
Copy link

@KazToozs KazToozs commented Aug 22, 2023

Adds ImplicitDeny logic to policy checks, where an ImplicitDeny will be sent back in case no policy validates an action, but does not explicitly Deny it either, allowing for bucket policies and other authorization mechanisms to grant permission.

Part of the bucket policy redo epic: https://scality.atlassian.net/jira/software/c/projects/OS/boards/214?modal=detail&selectedIssue=S3C-7756

@bert-e
Copy link
Contributor

bert-e commented Aug 22, 2023

Hello kaztoozs,

My role is to assist you with the merge of this
pull request. Please type @bert-e help to get information
on this process, or consult the user documentation.

Status report is not available.

@bert-e
Copy link
Contributor

bert-e commented Aug 22, 2023

Branches have diverged

This pull request's source branch improvement/ARSN-362-implicit-deny has diverged from
development/8.1 by more than 50 commits.

To avoid any integration risks, please re-synchronize them using one of the
following solutions:

  • Merge origin/development/8.1 into improvement/ARSN-362-implicit-deny
  • Rebase improvement/ARSN-362-implicit-deny onto origin/development/8.1

Note: If you choose to rebase, you may have to ask me to rebuild
integration branches using the reset command.

@@ -324,7 +324,10 @@ export function evaluateAllPolicies(
requestContext: RequestContext,
Copy link
Contributor

Choose a reason for hiding this comment

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

to avoid breaking compatibility, may be better not to touch the prototype of this function : i.e. make the change to the body in a function with a new name, and transform the older function in a simpler wrapper
→ this way existing code using it is not affected, it can decide when they "upgrade" to the new API, and we don't introduce coupling or limit upgrade paths
→ ideally we could even mark the 'old' method as deprecated, though I am not sure what is the impact currently: depending on build flags it could probably fail the build...

Something like this:

/* @deprecated Upgrade to evaluateAllPoliciesV2
 */
export function evaluateAllPolicies(
    requestContext: RequestContext,
    allPolicies: any[],
    log: Logger,
): string {
    return evaluateAllPoliciesV2(requestContext, allPolicies, log).verdict;
}

export function evaluateAllPoliciesV2(
    requestContext: RequestContext,
    allPolicies: any[],
    log: Logger,
): {
    verdict: string;
    isImplicit: boolean;
} {
   ...
}

@KazToozs KazToozs force-pushed the improvement/ARSN-362-implicit-deny branch from 12205d6 to 227b869 Compare August 25, 2023 08:30
@KazToozs KazToozs requested a review from anurag4DSB September 5, 2023 08:05
Copy link
Contributor

@anurag4DSB anurag4DSB left a comment

Choose a reason for hiding this comment

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

Code LGTM

The PR needs to target development/7.70
We need a test to ensure the v1 method only returns verdict for future change prevention

@KazToozs
Copy link
Author

KazToozs commented Sep 6, 2023

@anurag4DSB the tests you're speaking of already exist just above
https://github.com/scality/Arsenal/pull/2162/files#diff-f13d18d50beb84716ae3ca3972ea5222c49ac740e32e4dbac083fba8cb51fda7R1432

Is this what you had in mind?

@KazToozs KazToozs changed the base branch from development/8.1 to development/7.70 September 6, 2023 09:17
@bert-e
Copy link
Contributor

bert-e commented Sep 6, 2023

Incorrect fix version

The Fix Version/s in issue ARSN-362 contains:

  • 7.70.10

Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:

  • 7.70.10

  • 8.1.110

Please check the Fix Version/s of ARSN-362, or the target
branch of this pull request.

Copy link
Contributor

@anurag4DSB anurag4DSB left a comment

Choose a reason for hiding this comment

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

Its worth keeping the v1/v2 consistent across cloudserver, arsenal and vault.

Copy link
Contributor

@anurag4DSB anurag4DSB left a comment

Choose a reason for hiding this comment

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

We need to aim development/7.10 as per the fix versions specified in the epic: https://scality.atlassian.net/browse/S3C-7756

@KazToozs
Copy link
Author

closing and opening new PR to facilitate rebase to 7.10

#2164

@KazToozs KazToozs closed this Sep 11, 2023
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