[FC-0099] feat: add casbin model configuration (CONF) #49
Conversation
openedx-webhooks
added
open-source-contribution
|
Thanks for the pull request, @BryanttV! This repository is currently maintained by 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 approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo 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:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere 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: 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:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
|
The full thread discussion is here.
|
|
Taking the above into account, this is the proposed model, which in fact is just some changes in the matcher from the original proposal: [request_definition]
r = sub, act, obj, scope
[policy_definition]
p = sub, act, obj, eft
[role_definition]
g = _, _, _
g2 = _, _
[policy_effect]
e = some(where (p.eft == allow)) && !some(where (p.eft == deny))
[matchers]
m = (g(r.sub, p.sub, r.scope) || g(r.sub, p.sub, "*")) && keyMatch(r.obj, p.obj) && g2(p.act, r.act)
|
mariajgrimaldi
changed the title
| [request_definition] | ||
| r = sub, act, obj, scope | ||
|
|
||
| [policy_definition] | ||
| p = sub, act, obj, eft |
There was a problem hiding this comment.
After thinking about it, I now lean toward not using obj at all. Here is why:
- Every request and policy are explicitly scoped. Here are some examples: can I edit course 123? can I edit under org 123? or can I edit globally? This follows our note about not modeling containment ourselves here [FC-0099] docs: ADR proposing authorization foundation for openedx ecosystem #31 (comment)
- If these are scoped, what value does
objadd? If I can edit a course, there is a policy with my username, the role with the edit permission, and the course scope. If I can edit org-wide, the role has an org scope, and so on. If you think about it, we actually don't need the object. objseems useful only for exceptions. For example, org-wide edit rights except for one course. I still think we can model that withoutobj.
So at least for the MVP, I don't think we need to model the object. Maybe in the future when we need more granularity at the object level we might need to although I can't think of a use case at the moment. Do you have one in mind? Maybe I'm not seeing it.
One concern I do have about not modeling objects, although they seem unnecessary, is how difficult it would be to migrate to a model with objects if we get to need them in the future.
What do you think?
There was a problem hiding this comment.
I see your point, and it makes sense that the scope already covers most of the use cases we care about in the MVP. Removing obj definitely simplifies the model and avoids redundancy.
However, I wonder how we could model exceptions (e.g., org-wide edit rights except for one course) without on obj?
There was a problem hiding this comment.
Mmmm I'm not sure. If we add an exception for a single course, the system won't pick it up as part of the org policy cause those would be different scopes. I think we have two options to model this:
- We completely drop support for exceptions for the MVP and consider these policies as an allowlist -> I'm not sure about this, I'm concerned about the future re-work.
- We leave the door open for future support by allowing passing scopes as: containers (orgs can contain multiple courses) or object-level (single course or library). When using containers (like orgs), we must pass the object in the assignment - this would be easy to read. When using object-level, we only use the object and ignore the scope in the assignment - also easy to read. The object in the container won't be useful, but it works as documentation and future exceptions support via dynamic policies, for example. ([FC-0099] docs: ADR proposing authorization foundation for openedx ecosystem #31 (comment))
There was a problem hiding this comment.
@mariajgrimaldi, I was testing a new model using your proposed approach without object in this branch.
What do you think about this? What I don’t like is having to specify the scope in both the role assignments (g) and the policies (p)., e.g.
# Bob is org admin for OpenedX (org scope)
g, user:bob, role:org_admin, org:OpenedX
# Org admin: can manage all resources within org OpenedX
p, role:org_admin, act:manage, org:OpenedX, allowThere was a problem hiding this comment.
I'd need to think about it more but I don't want to block this specific PR. Is this something we can refine as we go? Before the integration with the rest of the ecosystem, of course.
There was a problem hiding this comment.
Here's my quick analysis of the examples you shared. Maybe it helps explain why I'm still a bit confused while reviewing this again:
There was a problem hiding this comment.
Hi @mariajgrimaldi, thank you for your analysis. Yes, I really think the proposed model is confusing, and handling the scope both in the policy and in the role definitions might not be the best way to approach it. I’ve been working on another approach, based on your comments about not using an object, and I think it could be better structured.
Basically, the scope would be removed from the role definitions and used only in the policy. This branch has all the details, and I’d like you to review it. The main changes are:
- There is no object (
obj) - The scope exists only in the policy (
p) - The role definition (
g) no longer includes the scope
I’ll take a similar example to the previous one but using the new model:
# Organization-level permissions
p, role:org_admin, act:manage, org:OpenedX, allow
# All library edit permissions
p, role:library_author, act:edit, lib:*, allow
# Organization administrators
g, user:alice, role:org_admin
# Library authors
g, user:mary, role:library_authorAn example request would be:
user:alice, act:manage, org:OpenedX # True
user:alice, act:manage, org:MIT # False
user:mary, act:edit, lib:math-basics # True
user:mary, act:edit, lib:science-101 # TrueI think the relationship is much clearer this way. Also, exceptions could be included. For example, Mary can edit all libraries except one specific library:
p, role:library_author, act:edit, lib:restricted, denySo this would result in no permission:
user:mary, act:edit, lib:restricted # FalseWhat do you think of this proposal? Personally, I see it as an improvement, but I’d like to know if maybe I’m overlooking something important to consider.
There was a problem hiding this comment.
That’s definitely an improvement!
I made a few tweaks to address my main concern, which is that default roles shouldn't have access to specific scopes like org:OpenedX. Here's what I got:
[request_definition]
# Request format: subject (user), action, scope_id (specific resource being accessed)
r = sub, act, scope_id
[policy_definition]
# Policy format: subject (role), action, scope_type (pattern), effect (allow/deny)
p = sub, act, scope_type, eft
[role_definition]
# g: user-to-role assignments with scope (user, role, scope_id)
# g2: action hierarchies (parent_action, child_action)
g = _, _, _
g2 = _, _
[policy_effect]
# Deny overrides: if any policy says deny, result is deny; otherwise allow if any policy says allow
e = some(where (p.eft == allow)) && !some(where (p.eft == deny))
[matchers]
# Match if: user has role in scope AND scope matches pattern AND (action matches OR action inherits via g2)
m = g(r.sub, p.sub, r.scope_id) && keyMatch(r.scope_id, p.scope_type) && (r.act == p.act || g2(p.act, r.act))
UPDATE: the problem here ^ would be that we're closing the door for resources that are not necessarily scopes. For example: an asset within a course (course scope, resource asset). A valid response to this could be we won't support it, though. We still haven't made this decision.
Another more flexible option could be:
[request_definition]
r = sub, act, object_id, scope_id # Specific resource being accessed under the scope. I have to have the permission on the scope with the namespaced-object
[policy_definition]
p = sub, act, object_type, scope_type, eft # Generic template for roles definitions: all courses within an org, all libraries within an org, anything within an org, etc
[role_definition]
g = _, _, _ # Same as before
g2 = _, _
[policy_effect]
e = some(where (p.eft == allow)) && !some(where (p.eft == deny))
[matchers]
m = g(r.sub, p.sub, r.scope_id) && keyMatch(r.object_id, p.object_type) && keyMatch(r.scope_id, p.scope_type) && (r.act == p.act || g2(p.act, r.act))
Regarding this matcher, I think there might be something missing in the role definition cause we're not associating the object_id in any g type. This one is similar to our original proposal, but with a different naming approach.
I don’t want to hold this up any longer. What I like about option 2 is that we can move to option 1 pretty easily, and the other way around as well. If you agree, we could go with your proposal (option 1) now and switch to option 2 later if needed.
What do you think? Do you think this could work?
There was a problem hiding this comment.
Hi again, @mariajgrimaldi. Thank you very much for your comments and suggestions.
I definitely think that proposal N1 you mentioned is better structured, especially considering the concern about default roles. I’ve been testing this new model, and it works very well for our MVP needs. Based on the current use cases, it’s not necessary to include an object (obj), only the scope, which also makes the modeling simpler.
From your proposal, I added the following to the matcher:
- g(r.sub, p.sub, r.scope)
+ (g(r.sub, p.sub, r.scope) || g(r.sub, p.sub, '*'))This covers cases where we need to assign permissions across all libraries:
p, role:library_admin, manage_library, lib:*, allow
g, user:steve, role:library_admin, *
This way, requests like the following are allowed without having to assign each specific library:
user:steve manage_library lib:123
user:steve manage_library lib:xyz
What do you think about this?
I’m currently updating the tests to reflect the changes in the model.
There was a problem hiding this comment.
That's great, @BryanttV! Thanks for trying out my suggestions. Let's use this as our base model, and if we need more coverage later on, we can build on it.
mariajgrimaldi
left a comment
mariajgrimaldi
left a comment
There was a problem hiding this comment.
More detailed comments about the implementation. This is looking good! Thank you so much :)
Should we share this on the slack channel to get more reviews?
| help = ( | ||
| "Test Casbin enforcement policies using model.conf, policy.csv, and request.txt. " | ||
| "Supports interactive mode for custom testing." | ||
| ) |
There was a problem hiding this comment.
More than testing I see this as a simulation for a real environment to test whether our model is working properly. Can we change the wording to reflect that? Also I think we need to change requests.txt for requests.sample.
There was a problem hiding this comment.
Yes, I updated the command. What do you think?
| [request_definition] | ||
| r = sub, act, obj, scope | ||
|
|
||
| [policy_definition] | ||
| p = sub, act, obj, eft |
There was a problem hiding this comment.
Mmmm I'm not sure. If we add an exception for a single course, the system won't pick it up as part of the org policy cause those would be different scopes. I think we have two options to model this:
- We completely drop support for exceptions for the MVP and consider these policies as an allowlist -> I'm not sure about this, I'm concerned about the future re-work.
- We leave the door open for future support by allowing passing scopes as: containers (orgs can contain multiple courses) or object-level (single course or library). When using containers (like orgs), we must pass the object in the assignment - this would be easy to read. When using object-level, we only use the object and ignore the scope in the assignment - also easy to read. The object in the container won't be useful, but it works as documentation and future exceptions support via dynamic policies, for example. ([FC-0099] docs: ADR proposing authorization foundation for openedx ecosystem #31 (comment))
mariajgrimaldi
changed the title
BryanttV
force-pushed
the
bav/casbin-model
branch
from
6b9a161 to
2683c77
Compare
bmtcril
left a comment
bmtcril
left a comment
There was a problem hiding this comment.
Just a couple of questions in the files currently. I think we will need to sanitize inputs somehow since we're taking in user strings for things like usernames, orgs, course keys, library keys, etc. that may have asterisks or other unexpected characters in them. I'd love to see some test cases around that and make sure those cases all work as expected.
| # - Scope determines which role assignments are considered | ||
| # - "*" = global scope (system-wide roles) | ||
| # - "org:OpenedX" = organization-scoped roles | ||
| # - "course:course-v1:..." = course-scoped roles |
There was a problem hiding this comment.
If there is a lib based scope we should add it here too
There was a problem hiding this comment.
I assume that the course-v1 isn't explicitly needed or validated? For instance ccx courses have a different prefix, and I'm sure someday there will be a course-v2. 🙂 It might be good to add tests for those things to make sure they continue to work since people always forget about ccx.
There was a problem hiding this comment.
- I updated the comments to mention the
libscope. - Correct, the prefix in this case would be
course-v1, but another scope could becourse-v2or any other type of prefix we might need.
There was a problem hiding this comment.
@BryanttV Thanks for this. I was testing the model, and it appears to be very good.
A suggestion: for clearer testing, it may be better to test not for roles, but for features, and use generic wording.
What I tested
Policy
# Action inherence (action grouping)
g2, act:manage, act:delete
g2, act:delete, act:edit
g2, act:edit, act:read
# Role definition and wildcard for resource
p, role:manager, act:manage, *, allow
p, role:org_manager, act:manage, *, allow
p, role:lib_manager, act:manage, lib:*, allow
# Role assignment, wildcard for scope, resource grouping via scope
g, user:admin, role:manager, *
g, user:org_admin, role:org_manager, org:MIT
g, user:org_lib_admin, role:lib_manager, org:MIT
#Granular assignment
p, user:editor, act:edit, *, allow
p, user:specific_lib_manager, act:manage, lib:specific, allow
#Deny access
p, user:admin, act:manage, lib:restricted-content, deny
#Custom role (role inheritance)
g, role:my_custom, role:manager, org:openedx
p, role:my_custom, act:draw, *, allow
g, user:creative_manager_openedx, role:my_custom, org:openedx
Requests with comments
# The user:org_admin only has access to org:MIT
user:org_admin, act:manage, something, null #false
user:org_admin, act:manage, something, org:MIT #true
# The user:org_lib_admin only has managment over resources lib:* and org:MIT
user:org_lib_admin, act:manage, something, org:MIT #false
user:org_lib_admin, act:manage, lib:some_lib, org:MIT #true
# Testing the action inherance (grouping)
user:org_admin, act:delete, something, org:MIT #true
user:org_admin, act:read, something, org:MIT #true
# Granular permissions
# The user:editor can edit all
user:editor, act:edit, something, null #true
# The user:specific_lib_manager, can manage lib:specific
user:specific_lib_manager, act:edit, lib, null #true
user:specific_lib_manager, act:edit, lib:specific, null #false
# Exceptions and deny access
user:admin, act:edit, lib:restricted-content, org:x #false
# Role inheritance
# user:creative_manager_openedx has role:my_custom which inherits from role:manager
user:creative_manager_openedx, act:read, something, org:openedx
Requests ready to copy and paste
user:org_admin, act:manage, something, null
user:org_admin, act:manage, something, org:MIT
user:org_lib_admin, act:manage, something, org:MIT
user:org_lib_admin, act:manage, lib:some_lib, org:MIT
user:org_admin, act:delete, something, org:MIT
user:org_admin, act:read, something, org:MIT
user:editor, act:edit, something, null
user:specific_lib_manager, act:edit, lib, null
user:specific_lib_manager, act:edit, lib:specific, null
user:admin, act:edit, lib:restricted-content, org:x
user:creative_manager_openedx, act:read, something, org:openedx
I tested using https://casbin.org/editor/ (and the PyCasbin option).
Notes about what we are supporting:
- Resource Grouping ✅ (Note: we are checking the policy, but to know if a resource belongs to a group (for example, org), the request is the protagonist)
- Role Inheritance Across the Scopes
⚠️ (It works, but I had problems when I wanted to inherit a role with a wildcard scope) - Namespace with Wildcard Support ✅
- System-Wide Roles ✅
- Granularity of Permissions ✅ (Note: for granularity assignments, the policy starts with p instead of g)
- Exceptions / Negative Rules ✅
- Scoped Assignments ✅
- Action Grouping ✅
Details on what I encounter that fails:
In my test, I had:
#Custom role (role inheritance)
g, role:my_custom, role:manager, org:openedx
p, role:my_custom, act:draw, *, allow
g, user:creative_manager_openedx, role:my_custom, org:openedx
If I change the first line to a g, role:my_custom, role:manager, *, it doesn't work well (The request: user:creative_manager_openedx, act:manage, something, org:openedx returns false, and it should return true).
And we'll probably need those inheritances, not only for future custom roles, but for mapping that a library_admin inherits from library_author, and this inherits from library_collaborator, and so on.
| # obj = object selector - supports multiple patterns via keyMatch: | ||
| # - Exact ID: "course:course-v1:OpenedX+Demo+2024" | ||
| # - Namespace wildcard: "course:*" (matches all courses) | ||
| # - Prefix patterns: "course:course-v1:OpenedX+*" (matches all OpenedX courses) |
There was a problem hiding this comment.
[nit - nice to have] Does this look right?
course:course-v1:OpenedX+DemoX+DemoCourse
lib:lib:DemoX:CSPROB
Now that I see it in the conf, it looks off to me. Maybe we can consider something different:
course:<course-v1:OpenedX+DemoX+DemoCourse>
lib:<lib:DemoX:CSPROB>
...
course:<course_id...regex>
What do you think? I like it since it clearly separates what the namespace is and what the ID is.
There was a problem hiding this comment.
Yes, it might look a bit confusing. However, in the case of using <>, I suppose we would also have to change the definitions of role:<>, act:<>, user:<>, etc., to keep things consistent.
Another option would be to use something like this (I saw this suggestion somewhere else but can’t find it anymore):
If the scope doesn’t have a namespace, we include one:
org:OpenedX
If the scope already has a namespace, we leave it as is:
course-v1:OpenedX+DemoX+DemoCourse
lib:Openedx:CSPROB
Another option could be replacing the : with another distinguishing character, like @:
user@alice
org@OpenedX
course@course-v1:OpenedX+DemoX+DemoCourse
lib@lib:DemoX:CSPROB
For now, I think we could stick with the current format, and if needed, we can adopt a different notation later. What do you think?
There was a problem hiding this comment.
As @bmtcril suggested, let's use this for now, but please open an issue in the repo to review the points mentioned in his comment.
|
Thank you so much for this! I'm still reviewing this, but looks good so far. I do still have some questions though:
|
|
@MaferMazu: thanks for testing this out! Could you please attach the requests you made so we can rerun them with our new model? Thanks! |
| - Display of loaded policies, role assignments, and action grouping rules | ||
|
|
||
| Example usage: | ||
| python manage.py lms enforcement --policy-file-path /path/to/authz.policy |
There was a problem hiding this comment.
[nit] Can we run this outside the lms container? I think it'd be useful to be able to run this kind of command in an isolated environment without all that having an lms container running involves.
There was a problem hiding this comment.
I changed the test.py file to be this and can access the django shell without any issues - I removed the plugin_settings for convenience:
"""
Test settings for openedx_authz plugin.
"""
from django.conf import settings
import os
from openedx_authz import ROOT_DIRECTORY
# Add Casbin configuration
CASBIN_MODEL = os.path.join(ROOT_DIRECTORY, "engine", "config", "model.conf")
# Redis host and port are temporarily loaded here for the MVP
REDIS_HOST = "redis"
REDIS_PORT = 6379
DATABASES = {
"default": {
"ENGINE": "django.db.backends.sqlite3",
"NAME": "default.db",
"USER": "",
"PASSWORD": "",
"HOST": "",
"PORT": "",
}
}
INSTALLED_APPS = (
"django.contrib.admin",
"django.contrib.auth",
"django.contrib.contenttypes",
"django.contrib.messages",
"django.contrib.sessions",
"openedx_authz.apps.OpenedxAuthzConfig",
"casbin_adapter.apps.CasbinAdapterConfig",
)
MIDDLEWARE = [
"django.contrib.sessions.middleware.SessionMiddleware",
"django.contrib.auth.middleware.AuthenticationMiddleware",
"django.contrib.messages.middleware.MessageMiddleware",
]
TEMPLATES = [
{
"BACKEND": "django.template.backends.django.DjangoTemplates",
"DIRS": [],
"APP_DIRS": True,
"OPTIONS": {
"context_processors": [
"django.template.context_processors.debug",
"django.template.context_processors.request",
"django.contrib.auth.context_processors.auth",
"django.contrib.messages.context_processors.messages",
],
},
},
]
SECRET_KEY = "test-secret-key"
There was a problem hiding this comment.
Yes, I hadn’t tried it before, but with the current configuration, it’s also possible to run the command outside the container.
|
@mariajgrimaldi I tested using https://casbin.org/editor/ (and the PyCasbin option). Requests with comments Requests ready to copy and paste |
|
Just a reminder, we need to model complex scopes per role. We expect custom roles that grant permissions to different resources with different scopes. The system should support a single role assignment that allows, for example: To be clear, this is illustrative language, a role groups permissions, not roles, so please do not read this as role nesting. |
|
Thanks for the annotation, @gviedma-aulasneo! Right now we're focusing on immediate needs for the MVP, so the model in this PR is minimal and doesn't cover the use cases you mentioned. Still, we believe Casbin is flexible enough to handle them later. Our plan is to start with this minimal base model and extend it for more complex cases, while keeping it simple and maintaining compatibility with the current rules. We might test some things out based on this model, but I personally wouldn't like to block this effort to validate those use cases. Does that sound right to you? Let me know! |
|
@mariajgrimaldi thanks, that works for me. This clarification came from a sync with @MaferMazu yesterday, she was unsure whether we could accommodate those cases, so I wanted to raise it early. If you are confident that the MVP model and Casbin can extend to cover them, I share your opinion of not blocking this PR and validating those scenarios later, given the short timeline. |
chore: add base and test requirements feat: add model.conf of casbin test: include test suite for enforcement
BryanttV
force-pushed
the
bav/casbin-model
branch
from
ddaa056 to
7c3bf0d
Compare
| # This model supports: | ||
| # - Scoped role assignments (user roles tied to specific contexts) | ||
| # - Action grouping (manage → read/write/delete to reduce duplication) | ||
| # - Action grouping (manage → read/write/edit/delete to reduce duplication) |
There was a problem hiding this comment.
What's the difference between write and edit here? Is write just create + edit?
There was a problem hiding this comment.
Actually, this example can also be seen like this:
# manage implies edit and delete
["g2", "act:manage", "act:edit"],
["g2", "act:manage", "act:delete"],
# edit implies read and write
["g2", "act:edit", "act:read"],
["g2", "act:edit", "act:write"],by inheritance manage would include all permissions
There was a problem hiding this comment.
Ah thank you, I think I missed that there could be multiple levels of inheritance
mariajgrimaldi
left a comment
mariajgrimaldi
left a comment
There was a problem hiding this comment.
Thank you so much for addressing all of my comments! I think this is good enough for our base model, we can keep building on it if needed.
Great job!
bmtcril
left a comment
bmtcril
left a comment
There was a problem hiding this comment.
I agree that this is plenty for this PR and we should get it merged and iterate. I'd also like to get some clarity around namespacing and ownership of namespaces for existing cases and the extensions since there is some possibility of collisions and unintended issues there.
|
@mariajgrimaldi @bmtcril, I just created an issue to clarify the namespace: #65 |
github-project-automation
bot
moved this from Waiting on Author
to Done
in Contributions
6 participants
Description
This PR adds the Casbin model. This model (
model.conf) defines how requests and policies are structured and evaluated. The built model for Open edX takes into account the following points:Supporting Information
model.confHow to Test
This command creates a Casbin enforcer using the
model.confconfiguration by default (or a custom model file) and a user-specified policy file, then provides an interactive mode for testing authorization enforcement requests.