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 375 chain backend imp deny #2187

Draft
wants to merge 2 commits into
base: development/8.1
Choose a base branch
from

Conversation

KazToozs
Copy link

@KazToozs KazToozs commented Nov 27, 2023

Adding new implicitDeny logic to ChainBackend returns.

PLEASE NOTE:
I do not know what the initial requirement/intended functionality of this class is.
Thus, I am unsure of the expected 'isImplicit' result of the policy merging functionality in this interface.
What I can discern from the code is this:

  • The ChainBackend checkPolicies method is called. It returns an array of the authorisation results of the request for each client in the ChainBackend (by calling the checkPolicies functions of these clients).
  • Each element of the array is an array of the authorisation results from the different policies for a client.
  • Currently, if there is a duplicate policy (of the same ARN), the one that has isAllowed === true takes priority. I would've thought it would make sense to have Deny take priority here. This means I would change this line in the _mergePolicies method:
                if (!policyMap[key] || !policyMap[key].isAllowed) {

to this

                if (!policyMap[key] || !policy.isAllowed) {

However, the tests expect the current behaviour. Why?
What would be the expected behaviour from this for the isImplicit variable?
Who would have this context? The code was initially created by Alex Chan after report by @rachedbenmustapha
Thanks for any feedback

@bert-e
Copy link
Contributor

bert-e commented Nov 27, 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 Nov 27, 2023

Waiting for approval

The following approvals are needed before I can proceed with the merge:

  • the author

  • 2 peers

@KazToozs KazToozs force-pushed the improvement/ARSN-375-chain-backend-impDeny branch from e7ae687 to 0abc8c2 Compare November 27, 2023 18:12
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.

2 participants