IAM-1013 Implement granular payload authorization check for Group Assign{Roles,Identities} handlers

[BarcoMasile]

Description

This PR adds implementation to check if the Assign{Identities,Roles} operation can be performed.
It leverages a BatchCheck that was previously not exposed on the underling client interface of the OpenFGA service.

Changes

• removed tracing from buildGroupMember-like functions
• moved existing OpenFGA tuple building functions under internal/authorization/util.go and introduced functions to build tuple elements for all types in the OpenFGA authorization model
• added CanAssignIdentities and CanAssignRoles methods in groups service
• added tests and adjusted existing after adoption of new functions

[shipperizer]

let's have a chat on Monday on the middleware vs handler placement

I think that if we can reuse what we have is doable it might be cleaner too, if we can't this solution as is will do great

[nsklikas]

2 things:

1. I agree with alessandro that probably the authorization logic should be moved elsewhere
2. Please correct me if I'm wrong, but the logic in this PR is user A can assign roles Rs to group B IF user A can view roles Rs AND user A can edit group B and user A can add users Xs to group B if user A can view users Xs AND user A can edit group B. This does not sound correct. I remember there was a doc about this, but I don't remember if it ever reached an agreement.

[shipperizer]

1. I agree with alessandro that probably the authorization logic should be moved elsewhere

i'll have a bit of a poke around and see how the code looks like, most likely to do it properly it will require a bit of refactoring that i'm not keen on committing right now

2. Please correct me if I'm wrong, but the logic in this PR is user A can assign roles Rs to user B IF user A can view roles Rs and user A can add users Xs to group B if user A can view users Xs. This does not sound correct. I remember there was a doc about this, but I don't remember if it ever reached an agreement.

regarding users seeing group details (can_view on group) might be a security issue, they can already see the name and list them all, this should be enough
on top of that this allows administrators to group users in any way they deem appropriate without them knowing which allows for separation and isolation and without the target identities knowing it

eg: an admin could create a blacklist of users requiring extra safeguarding measures without this group showing up

[BarcoMasile]

2 things:

1. I agree with alessandro that probably the authorization logic should be moved elsewhere
2. Please correct me if I'm wrong, but the logic in this PR is user A can assign roles Rs to user B IF user A can view roles Rs and user A can add users Xs to group B if user A can view users Xs. This does not sound correct. I remember there was a doc about this, but I don't remember if it ever reached an agreement.

@nsklikas
For number 2. see Alessandro's response, I also share that view

For number 1: there are multiple reasons to avoid this logic in the middleware.
Middleware and authorization "Converters" already have multiple if-then-else needed for the specific implementation they need to attend to. That alone makes that logic unfit for a middleware, which must take care of cross-cutting concern (for which you'd have a common interface).

Also, granular payload authorization stems from resource specific behavior/needs, so it's something that (in this case) the Groups service should know about. Keep in mind that the current autz middleware + converters has a lot of logic already, meaning "it's already doing too much".
Having this kind of logic in the middleware would make it grow every time we need to add granular authz on other payloads (making it less maintanable and clear).

Not to mention that the middleware would have to deserialize the payload and be in the know of which type to use, so we'd have less separation of concerns and (especially) more inter module dependencies.

[nsklikas]

Regarding 2, currently the authz logic for these handlers is spread across groups/handlers (calls service and decides on the error code), groups/service (makes a call about the can_view permission) and middleware (makes a call about the can_edit permission and decides on the error code). IMHO our middleware API should change/refactor OR we should agree that from now on the authz middleware is responsible for authz checks only on the URL and authz checks on the payload are done only by the handler/service. If we plan on refactoring the middleware, then I suggest we put a TODO on these changes as they need to be refactored.

[shipperizer]

had a quick go at working our the "middleware" solution 91d5094

a couple of issues (which kinda match concerns of you all)

• converters are not able to block the request flow and abort 91d5094#diff-e8239b84c55230e6b7666cd0ae442164f01229deb913cdebfd0f75fa04359b6aR491, this would need to be implemented first
• current BatchCheck implementation is not wired to accept contextual tuples, which blocks any chance of dealing with IAM-1013 Implement granular payload authorization check for Group Assign{Roles,Identities} handlers #433 (comment)
• mismatch between Tuple and Permission objects prevents any solid work from happening, hampering further the above bulletpoint

IMHO what we should do is:

• right now going out with this current setup
• create an issue to support ContextualTuples in BatchChecks
• create an issue to centralize payloads in a separate package so they are useable without circular import madness
• analyze how to move the payload validation to a middleware
• analyze how to move the payload authorization to the converters, leaning on the payload validation

long story short, as much as i'd like to make the change i don't think we are in a position to do it without a not so minor refactor

i stand by my idea that converters should do more and take care of authorization, be it URI or payload, but code maintainability should be paramount so if we can't make it happen then ain't happening

[BarcoMasile]

Regarding 2, currently the authz logic for these handlers is spread across groups/handlers (calls service and decides on the error code), groups/service (makes a call about the can_view permission) and middleware (makes a call about the can_edit permission and decides on the error code). IMHO our middleware API should change/refactor OR we should agree that from now on the authz middleware is responsible for authz checks only on the URL and authz checks on the payload are done only by the handler/service. If we plan on refactoring the middleware, then I suggest we put a TODO on these changes as they need to be refactored.

@nsklikas I share this point with Nikos, I believe that if logic is in the middleware than it needs to be able to handle things without custom (or "branch" specific) logic implemented with too many if-then-else.
I have to say that my preference is still to have these checks in the handlers, also because we would probably need it mostly for roles and groups, and sharing that logic in the middleware would be overkill. But it's just my opinion.

---

@shipperizer

• converters are not able to block the request flow and abort 91d5094#diff-e8239b84c55230e6b7666cd0ae442164f01229deb913cdebfd0f75fa04359b6aR491, this would need to be implemented first

I don't understand what should be implemented first, if tuples are used to check against OpenFGA than the "new" tuples added to the check should return "false" and the middleware would block with 403. Or I'm missing something.

• current BatchCheck implementation is not wired to accept contextual tuples

Created #436

• mismatch between Tuple and Permission objects prevents any solid work from happening, hampering further the above bulletpoint

This is something that leads me to think we have too many structs for same-purpose implementations, we should take a scan of the code and uniform things.
I won't create an issue for this now but I'll keep this in mind, considering it low low prio.

IMHO what we should do is:

• right now going out with this current setup
• create an issue to support ContextualTuples in BatchChecks ✅
• create an issue to centralize payloads in a separate package so they are useable without circular import madness ✅

Created issue #438

• analyze how to move the payload validation to a middleware
• analyze how to move the payload authorization to the converters, leaning on the payload validation

I'll leave the creation of issues for this to you guys since you're probably going to lead this refinement/development.

i stand by my idea that converters should do more and take care of authorization, be it URI or payload, but code maintainability should be paramount so if we can't make it happen then ain't happening

Agree on code maintanability. As far as my opinion on where to put things, I wrote it in response to Nikos' comments.

So if also @nsklikas agree we could proceed with this.

[shipperizer]

I don't understand what should be implemented first, if tuples are used to check against OpenFGA than the "new" tuples added to the check should return "false" and the middleware would block with 403. Or I'm missing something.

issue is when we are not able to parse the payload therefore we cant build new tuples, i was not able to find an "always fail" tuple

so in case of error we should abort the call, without aborting we would have no tuples to verify which would mean "no permissions to check, go ahead"

[BarcoMasile]

FYI I'm rebasing on main after new pushes arrive.