docs: add ADR for defining API for querying permissions from MFEs for current user FC-0099

[rodmgwgu]

ADR for defining an enforcement mechanism for MFEs and other clients, here we discuss about effective ways to achieve this and propose an API endpoint for permission querying.

Merge checklist:
Check off if complete or not applicable:

• Version bumped
• Changelog record added
• Documentation updated (not only docstrings)
• Fixup commits are squashed away
• Unit tests added/updated
• Manual testing instructions provided
• Noted any: Concerns, dependencies, migration issues, deadlines, tickets

[openedx-webhooks]

Thanks for the pull request, @rodmgwgu!

This repository is currently maintained by @openedx/committers-openedx-authz.

Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review.

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

• If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
  • This process (including the steps you'll need to take) is documented here.
• If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

• Dependencies

  This PR must be merged before / after / at the same time as ...

• Blockers

  This PR is waiting for OEP-1234 to be accepted.

• Timeline information

  This PR must be merged by XX date because ...

• Partner information

  This is for a course on edx.org.

• Supporting documentation
• Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

Details

---

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

• Overview of Review Process for Community Contributions
• Pull Request Status Guide
• Making changes to your pull request

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

• The size and impact of the changes that it introduces
• The need for product review
• Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

[bradenmacdonald]

The proposed mechanism sounds fine to me, but I wonder if we should also include a standardized way to include permissions data in "regular" backend API responses.

For example, consider an MFE that is getting a list of libraries, and the user has "edit" permission on some of the libraries but not others. The MFE would first have to GET /api/v1/libraries/ to get the list of libraries, then wait for the response, then collect the IDs of the courses and query this permissions API with a bulk query ("object": "lib:blah1", "object": "lib:other2", "object": "lib:threelib", ...) before it knows which libraries the user can edit.

Because these queries cannot happen simultaneously¹, this means that either there is a delay in showing the edit buttons (so the users see the list of libraries after it's loaded, but none of them show the "Edit" button until the permissions have also loaded, and then there is a flash of change as the edit buttons get rendered once we hear back from the authz API), or we can choose to wait until both things have loaded before showing the list of libraries, which makes the MFE feel slower.

To solve this in the Authoring MFE today (and many other parts of the platform), we use a very simple solution - include the permissions data with the "list objects" API results (example).

So, I think that this ADR should also include a standard recommendation for including a "permissions" object attached to the results of most regular GET REST API requests. i.e. Any "list objects" or "get object" API should include an extra permissions: ... field that lists all the actions the current user is allowed to take on each object.

Footnotes

1. Unless you allow an "act:edit" on "lib:*" query, which returns a list of all libraries the user can edit... ↩

[rodmgwgu]

The proposed mechanism sounds fine to me, but I wonder if we should also include a standardized way to include permissions data in "regular" backend API responses.

For example, consider an MFE that is getting a list of libraries, and the user has "edit" permission on some of the libraries but not others. The MFE would first have to GET /api/v1/libraries/ to get the list of libraries, then wait for the response, then collect the IDs of the courses and query this permissions API with a bulk query ("object": "lib:blah1", "object": "lib:other2", "object": "lib:threelib", ...) before it knows which libraries the user can edit.

Because these queries cannot happen simultaneously1, this means that either there is a delay in showing the edit buttons (so the users see the list of libraries after it's loaded, but none of them show the "Edit" button until the permissions have also loaded, and then there is a flash of change as the edit buttons get rendered once we hear back from the authz API), or we can choose to wait until both things have loaded before showing the list of libraries, which makes the MFE feel slower.

To solve this in the Authoring MFE today (and many other parts of the platform), we use a very simple solution - include the permissions data with the "list objects" API results (example).

So, I think that this ADR should also include a standard recommendation for including a "permissions" object attached to the results of most regular GET REST API requests. i.e. Any "list objects" or "get object" API should include an extra permissions: ... field that lists all the actions the current user is allowed to take on each object.

Footnotes

1. Unless you allow an "act:edit" on "lib:*" query, which returns a list of all libraries the user can edit... ↩

Thanks for this insight, definitely having the permissions information in regular API responses would simplify things going forward.

In the case of libraries, we can keep the existing information in the list objects endpoint and adapt the underlying logic to use the new authz library for evaluation.

I'm not sure how we can generalize this so we have a single standard way of including these permissions information for any kind of resource.

Perhaps, going forward we could standardize this by including in the object a property like:

"authz_permissions": {
  [key: string]: boolean
}

Where key is the "action", and the boolean indicates if it's allowed or not.

What do you think?

[bradenmacdonald]

I like the idea of something like that, yeah. Interested to hear what other people think.

[dcoa]

I just have a question here, I'm not an expert but in my understanding Casbin will evaluate the query as denied and return false, in this kind of scenarios, if so will be difficult to the frontend give an appropriate feedback to the user whether does not have permission or the resource does not exist. Do we have this contemplated?

As much as I understand, based on a small searching and that is explained here https://ridouku.medium.com/casbin-an-alternative-to-validate-user-permissions-fd21fadc2b3a it is needed to wrap the enforcer in a function to make that validations and return the custom error.

[rodmgwgu]

Yes, that's my understanding, Casbin will just validate it as not allowed as it doesn't have knowledge of the actual resources on the database.

If we wanted to validate if, for example, a library exists, we would have to identify that the object we are targeting is a library (by parsing the lib: prefix), and then query the DB. This would add extra DB queries and logic which would impact on performance and complexity for maintainability.

I'm not sure if we really need to differentiate between a permission query on a non-existing resource vs just a false response. Do you see a use case where this would be important? The main use cases I have thought of are related to things like disabling edit buttons and hiding "view details" links.

[bmtcril]

I'm with @rodmgwgu on this, I think this module should be focused on whether or not the permission check passes. I can't think of a case where the front end would be querying for permissions of something that doesn't exist aside from the case of a race condition deleting a resource. Even in that case I don't think it should be authz's responsibility to communicate that information.

[bmtcril]

This looks good to me, thanks for writing it up!

[mariajgrimaldi]

Does this mean that if a user has permission on a scope, they automatically have permission on the object within it? If so, our decision is that scope inheritance will not be managed implicitly - sorry it wasn't part of an ADR, now it is: #76. Instead, checks must explicitly handle containment.

For example:

can(user, act:read, lib:test-lib) OR can(user, act:read, org:OpenedX)

Here, lib:test-lib and org:OpenedX are treated as separate scopes. I’m not sure if this belongs directly in this ADR, but it’s worth noting for clarity. Still I think this ADR covers this case as well, since object is optional.

FYI @bmtcril

[bmtcril]

I agree that the distinction is important and should probably be explicitly covered here as well.

[rodmgwgu]

@mariajgrimaldi does this mean that there won't be any cases where we want to specify both scope and object when checking for permissions with casbin? will scope and object will be the same thing now for casbin going forward?

What I'm understanding is that we will no longer have an "object" here, it will always go into "scope", even if it's an org, lib, or something else:

   [
       {
           "action": "act:read",
           "scope": "lib:test-lib"
       },
       {
           "action": "act:edit",
           "scope": "org:OpenedX"
       }
   ]

Is this correct?

[rodmgwgu]

I updated the doc to reflect these changes and match what is being implemented on #84

[mariajgrimaldi]

@rodmgwgu: that's the level of granularity we're going with for now. It's not set in stone, since the model could later evolve to be more granular. For now, let's note that possibility but focus on describing the current, more feasible approach.

[bmtcril]

@mariajgrimaldi is this a request for a change or can this merge?

[mariajgrimaldi]

Merged! Sorry I didn't see this earlier :)

[mariajgrimaldi]

LGTM! Thanks for the patience :)