Skip to content

[FC-0099] refactor!: consider global scope as wildcard managed by ScopeData #132

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mariajgrimaldi merged 8 commits into from
Nov 5, 2025
Merged

[FC-0099] refactor!: consider global scope as wildcard managed by ScopeData #132

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
38a0573
refactor: consider global scope wildcard when instantiating scope
mariajgrimaldi Oct 31, 2025
5bc85e1
docs: update docstring with latest changes
mariajgrimaldi Oct 31, 2025
5e8453d
test: add a test case for wildcard scope
mariajgrimaldi Oct 31, 2025
4c1577a
docs: update scope docstring
mariajgrimaldi Oct 31, 2025
eb540c5
docs: Apply suggestion from @rodmgwgu
mariajgrimaldi Nov 3, 2025
685ebb1
refactor: use global namespace instead of sc
mariajgrimaldi Nov 4, 2025
cf2fa62
refactor: change generic to global wildecard constant
mariajgrimaldi Nov 5, 2025
e802390
docs: update docs for next release
mariajgrimaldi Nov 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions openedx_authz/api/data.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,14 @@ def __call__(cls, *args, **kwargs):
if cls is not ScopeData:
return super().__call__(*args, **kwargs)

# When working with global scopes, we can't determine subclass with an external_key since
# a global scope it's not attached to a specific resource type. So we only use * as an
Comment thread
mariajgrimaldi marked this conversation as resolved.
Outdated
# an external_key to mean generic scope which maps to base ScopeData class.
# The only remaining issue is that internally the namespace key used in policies will be
# The generic scope namespace (sc^*), so we need to handle that case here.
if kwargs.get("external_key") == GENERIC_SCOPE_WILDCARD:
Copy link
Copy Markdown
Member Author

@mariajgrimaldi mariajgrimaldi Oct 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generic or global?

Copy link
Copy Markdown
Contributor

@rodmgwgu rodmgwgu Oct 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is a question on naming, I think global is clearer.

Copy link
Copy Markdown
Member Author

@mariajgrimaldi mariajgrimaldi Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I initially used generic scope cause this scope can be specialized into other scopes: content libraries, courses... But this might be too ambiguous. It might make sense making it global when not specialized?

Copy link
Copy Markdown
Contributor

@bmtcril bmtcril Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Global makes more sense to me 👍

return super().__call__(*args, **kwargs)

if "namespaced_key" in kwargs:
scope_cls = cls.get_subclass_by_namespaced_key(kwargs["namespaced_key"])
return super(ScopeMeta, scope_cls).__call__(*args, **kwargs)
Expand Down Expand Up @@ -303,6 +311,11 @@ class ScopeData(AuthZData, metaclass=ScopeMeta):
'sc^generic_scope'
"""

# The 'sc' namespace is used for generic scopes that aren't tied to a specific resource type.
# This base class supports:
# 1. Global scopes (external_key='*') that apply across all resource types
# 2. Custom generic scopes that don't map to specific domain objects
# Subclasses like ContentLibraryData ('lib') represent concrete resource types.
NAMESPACE: ClassVar[str] = "sc"
Copy link
Copy Markdown
Member Author

@mariajgrimaldi mariajgrimaldi Oct 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bmtcril: remember we talked about the usage of this generic scope? I think we found one!

I was thinking of changing NAMESPACE to an empty string cause I'm not sure how descriptive is sc. What do you think?

FYI @MaferMazu @BryanttV

Copy link
Copy Markdown
Contributor

@rodmgwgu rodmgwgu Oct 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What did "sc" stand for?

Copy link
Copy Markdown
Contributor

@rodmgwgu rodmgwgu Oct 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could change it to something related to "anonymous", as if it's supposed to identify scopes that don't map to specific objects, then they are "anonymous"?

Thinking on the usage of a namespaced key, it would look something like:

"anon^*"

Otherwise if we use an empty string, it woud be like "^*" right?

Won't this cause confusion because of the "missing" namespace?

Copy link
Copy Markdown
Member Author

@mariajgrimaldi mariajgrimaldi Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sc means scope 🥲, not very explicit though. Initially was only to have base definitions for all the other scopes, but now we can use it for global scopes so it might make sense to change this namespace.

Won't this cause confusion because of the "missing" namespace?

I think it will! Considering what I said here, https://github.com/openedx/openedx-authz/pull/132/files#r2486208488, instead of anonymous, should we use global or some synonym?

Copy link
Copy Markdown
Contributor

@bmtcril bmtcril Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think global makes the most sense, but whatever we use needs to be something that will never get a real value of the same name, right?

Copy link
Copy Markdown
Member Author

@mariajgrimaldi mariajgrimaldi Nov 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly! What about these?

  1. global: global^* (++1 most votes)
  2. instance: instance^*

In all cases, having something like can_create_libraries in NAMESPACE^* would mean I can create any number of libraries in my Open edX instance. This only makes sense for the * external key, right? It wouldn’t make sense for a specific key or would it also apply there? If so, then maybe we won't get values with the same name?

Copy link
Copy Markdown
Contributor

@rodmgwgu rodmgwgu Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding here is that in the case of the can_create_libraries permission, global^* Would mean that it applies to anything, not only libraries, but in practice can_create_libraries will only be evaluated on lib objects.

But, if at some point we add some more generic permissions, like can_create, that could apply that more than one domain object, then we could define more specific things, like global^*:DemoX:CSPROB, that would apply on anything on the org DemoX that is called CSPROB (courses, libs, etc). But this is a whole different level of complexity that I don't think we need now.

Copy link
Copy Markdown
Member Author

@mariajgrimaldi mariajgrimaldi Nov 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thing with can_create_libraries is that we can't actually evaluate it with a concrete library, but only against the instance (not sure how to call it) so it could be similar to can_create but only checked in context of libraries - hopefully I'm not getting all of this mixed up (so please correct me if I'm wrong!) 😅

that would apply on anything on the org DemoX that is called CSPROB (courses, libs, etc). But this is a whole different level of complexity that I don't think we need now.

I totally agree. Do you think global^*:DemoX:CSPROB wouldn't work for that can_create case? Oh I understand now! We should consider also cases without the *!

Copy link
Copy Markdown
Contributor

@rodmgwgu rodmgwgu Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh you are right, for can_create_libraries it doesn't make sense to specify a concrete library, so global makes sense there. It would make sense for something like can_edit_library instead.


@classmethod
Expand Down
21 changes: 21 additions & 0 deletions openedx_authz/tests/api/test_data.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,6 +345,27 @@ def test_empty_external_key_raises_value_error(self):
with self.assertRaises(ValueError):
SubjectData(external_key="")

def test_scope_data_with_wildcard_external_key(self):
"""Test that ScopeData instantiated with wildcard (*) returns base ScopeData.

When using the global scope wildcard '*', the metaclass should return a base
ScopeData instance rather than attempting subclass determination.

Expected Result:
- ScopeData(external_key='*') creates base ScopeData instance
- namespaced_key is 'sc^*'
- No subclass determination occurs
"""
scope = ScopeData(external_key="*")

expected_namespaced = f"{ScopeData.NAMESPACE}{ScopeData.SEPARATOR}*"

self.assertIsInstance(scope, ScopeData)
# Ensure it's exactly ScopeData, not a subclass
self.assertEqual(type(scope), ScopeData)
self.assertEqual(scope.external_key, "*")
self.assertEqual(scope.namespaced_key, expected_namespaced)


@ddt
class TestDataRepresentation(TestCase):
Expand Down