Loopback-next: @lb/authorization defaults to allow

[JohnPhoto]

Suggestion

I am (basically) using https://github.com/strongloop/loopback4-example-shopping/pull/231/files but I want to implement "OR-logic" for the voters/enforcers (compareId and casbin).

I did not find the actual code for @loopback/authorize in github but i got it from the src-dir in the package.

Use Cases

If a request is made and I am an "admin" in casbin I always want to be allowed to edit someones "order", no matter if my user owns the order or not. And in the same way I want an owner (which is a normal user) to only be allowed to see/edit their own orders and no other orders. I do not see how this is possible when all DENY is instantly kicked out, and all ABSTAIN is defaulted to be allowed access.

Examples / proposal

In the interceptor in authorize-interceptor.ts in @loopback/authorize I change the code to

// ...
    let allowed = false;

    authorizers = authorizers.concat(this.authorizers);
    for (const fn of authorizers) {
      const decision = await fn(authorizationCtx, metadata);

      if (decision === AuthorizationDecision.DENY) { // Someone is really detirmined I should not be here.
        const error = new AuthorizationError('Access denied');
        error.statusCode = 401;
        throw error;
      } else if (decision === AuthorizationDecision.ALLOW){ // Someone says I can come in, but lets check with everyone before we allow access.
        allowed = true;
      }
    }
    if(!allowed){ // No one said I was allowed access, then I should not be here.
      const error = new AuthorizationError('Access denied');
      error.statusCode = 401;
      throw error;
    }
    return next();

Then I change the services/authorizor.ts in my repo to (taken from lb4 shopping example)

// ...
    if (allow) return AuthorizationDecision.ALLOW;
    return AuthorizationDecision.ABSTAIN;

and services/id.compare.authorizors.ts to

// ...
  return userId == currentUser.id ? AuthorizationDecision.ALLOW : AuthorizationDecision.ABSTAIN;

In the end:
Only explicit allows will be allowed access, all denies will be short-circued to deny and if no authorizor takes responsibility your are also defaulted to be kicked out.

Acceptance criteria

TBD - will be filled by the team.

[raymondfeng]

@JohnPhoto Thank you for the quick feedback. Are you proposing to default to DENY if all authorizers vote ABSTAIN? Should we also honor ALLOW and return immediately?

Consider the voting outcome of 3 authorizers:

• Deny, Deny, Deny => Deny
• Allow, Allow, Allow => Allow
• Deny, Allow, Abstain => Allow?
• Allow, Abstain, Deny => Allow?
• Abstain, Deny, Abstain => Deny
• Abstain, Allow, Abstain => Allow
• Abstain, Abstain, Abstain => Deny

[jannyHou]

@JohnPhoto Good to know people are trying the demo in that PoC

For your requirement, I have a workaround in the custom voter, there are more explanations in the code.

But I agree that we should allow more flexible logic when making decision, like OR-logic, or AND-logic, or specify priorities for authorizers/voters.

[JohnPhoto]

@raymondfeng In my use case I would suggest (Deny always wins):

• Deny, Deny, Deny => Deny
• Allow, Allow, Allow => Allow
• Deny, Allow, Abstain => Deny
• Allow, Abstain, Deny => Deny
• Abstain, Deny, Abstain => Deny
• Abstain, Allow, Abstain => Allow
• Abstain, Abstain, Abstain => Deny

Are you proposing to default to DENY if all authorizers vote ABSTAIN? Exactly
Should we also honor ALLOW and return immediately? No thanks, not needed for my case

Basically I am coming from first sentence in https://docs.spring.io/spring-security/site/docs/3.0.x/reference/anonymous.html

@jannyHou I did see your workaround but its hard for me to fit such a "hack" with the logic I have in the other voters. I do like the plan for flexible decision making logic.

[raymondfeng]

I think we can make it more flexible as follows:

• Deny, Deny, Deny => Deny (always)
• Allow, Allow, Allow => Allow (always)
• Abstain, Allow, Abstain => Allow (always)

• Abstain, Deny, Abstain => Deny (always)

• Deny, Allow, Abstain =>

  • Deny (if options. precedence === 'Deny' or not set)
  • Allow (if options. precedence === 'Allow')

• Allow, Abstain, Deny =>

  • Deny (if options. precedence === 'Deny' or not set)
  • Allow (if options. precedence === 'Allow')

• Abstain, Abstain, Abstain

  • Deny (if options.defaultDecision === 'Deny' or not set)
  • Allow (if options.defaultDecision === 'Allow')

[JohnPhoto]

Sounds great to me!

[jannyHou]

Great https://github.com/strongloop/loopback-next/issues/3555#issuecomment-522064300

Just one question: what's the difference between

Deny, Allow, Abstain =>

    Deny (if options. precedence === 'Deny' or not set)
    Allow (if options. precedence === 'Allow' or not set)

and

Allow, Abstain, Deny =>
    Deny (if options. precedence === 'Deny' or not set)
    Allow (if options. precedence === 'Allow' or not set)

?

[raymondfeng]

That just show different order of allow/deny combinations.

[raymondfeng]

@jannyHou I'm working on a PR to implement this proposal.

[dhmlau]

@raymondfeng , since PR #3556 has landed, does it mean this task is done? Thanks.