Skip to content

Conversation

sagilio
Copy link
Member

@sagilio sagilio commented Jul 17, 2021

close #188

@sagilio sagilio added the enhancement Enhancement the exist feature label Jul 17, 2021
@sagilio sagilio self-assigned this Jul 17, 2021
@sagilio sagilio changed the title pref: Support priority deny override model without recursion. perf: Support priority deny override model without recursion. Jul 17, 2021
@sagilio
Copy link
Member Author

sagilio commented Jul 17, 2021

@apiscevs Plz review

@apiscevs
Copy link
Contributor

apiscevs commented Jul 17, 2021

Hi @sagilio ,

thanks for reviewing a refactoring my code.
Approach without a recursion looks good, however there is a one problem.

Code breaks if it did not find a highest level policy, in the result policy result is always false.
Expected result, is that if matching policy were not found on higher tiers, it should dive deeper.

Please use this test to reproduce an issue.

public void TestPriorityExplicitDenyOverrideWithFakeHighTierPolicyModel()
{
    var e = new Enforcer(_testModelFixture.GetNewPriorityExplicitDenyOverrideModel());
    e.BuildRoleLinks();
    TestEnforce(e, "alice", "data2", "write", true);
    // adding higher priority policy for alice, however policy's sub, obj and action are fake and never be matched
    //expected enforcement result should be true, as there is a policy with a lower rank 10, that produces allow result
    e.AddPolicy("5", "fake-subject", "fake-object", "very-fake-action", "allow");
    TestEnforce(e, "alice", "data2", "write", true);
}

I've ran this test using initial solution with recursion, it worked successfully.

@apiscevs
Copy link
Contributor

apiscevs commented Jul 17, 2021

looks like it can be easily fixed if check Effect result

if (hasPriority && isPriorityDenyOverrideEfffet && nowEffect != Effect.Effect.Indeterminate)
{
    if (int.TryParse(policyValues[priorityIndex], out int nowPriority))
    {
        if (priority.HasValue && nowPriority != priority.Value)
        {
            break;
        }
        priority = nowPriority;
    }
}

@hsluoyz hsluoyz requested a review from xcaptain July 17, 2021 13:10
@sagilio
Copy link
Member Author

sagilio commented Jul 17, 2021

@apiscevs You are right, I have added a new test case, please review again.

@sagilio sagilio changed the title perf: Support priority deny override model without recursion. perf: Support priority deny override model without recursion Jul 17, 2021
@apiscevs
Copy link
Contributor

LGTM!
This is way elegant compared to recursion :)

@sagilio sagilio merged commit dfb5ced into casbin:master Jul 17, 2021
@github-actions
Copy link

🎉 This PR is included in version 1.10.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement the exist feature released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Combine priority and deny-override models
2 participants