Skip to content

feat: make can_tag_object's course check authorization-service aware - #39112

Open
alezconsultant wants to merge 3 commits into
openedx:masterfrom
alezconsultant:alezconsultant/795-add--can-change-tag-authz-branch
Open

alezconsultant wants to merge 3 commits into
openedx:masterfrom
alezconsultant:alezconsultant/795-add--can-change-tag-authz-branch

Conversation

@alezconsultant

@alezconsultant alezconsultant commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

Description

Makes oel_tagging.can_tag_object's course check authorization-service aware
(openedx/openedx-core#795). can_change_object_tag_objectid in
content_tagging/rules.py — the predicate every direct has_perm('oel_tagging.can_tag_object', ...) caller resolves through — already checked the new openedx-authz service for Content
Library objects, but for course objects it only ever checked the legacy has_studio_write_access
role, regardless of whether that course had been switched to the new authorization service.
ObjectTagOrgView already had the correct, toggle-aware
behavior, but only inside that one view class — every other caller, including the CBE
create-criterion endpoint (openedx-core#665), silently got the wrong (legacy-only) answer for
switched-over courses. This closes that gap by giving the shared predicate the same
toggle-aware branch ObjectTagOrgView already had, then removing the view's own now-duplicate
override so the toggle check lives in one place.

Changes

  • content_tagging/rules.py: can_change_object_tag_objectid gets a new branch, checked
    after the existing Content Library case and before the legacy has_studio_write_access
    fallback. It calls should_use_course_authz_for_object(object_id) (already used by
    ObjectTagOrgView, from content_tagging/auth.py); when the course has been switched, it
    returns authz_api.is_user_allowed(user.username, COURSES_MANAGE_TAGS.identifier, str(course_key)) directly, with no fallback to legacy roles or org-admin access — an
    exclusive switch, matching ObjectTagOrgView's existing behavior exactly. Docstring updated
    to describe the new branch.
  • content_tagging/rest_api/v1/views.py: ObjectTagOrgView.ensure_user_has_can_tag_object_permissions
    deleted. Verified the parent ObjectTagView.ensure_user_has_can_tag_object_permissions
    (openedx-core) already resolves through user.has_perm("oel_tagging.can_tag_object", ...),
    so this override became pure duplication of the same logic now living in rules.py.
    get_permissions and _authz_check are unchanged — _authz_check's docstring now
    explains why: ensure_has_view_object_tag_permission (the separate, untouched view
    permission) and ObjectTagTaxonomyOrgFilterBackend.filter_queryset both still read it
    directly, so only the tagging-permission override became redundant, not this.
  • content_tagging/tests/test_rules.py: new TestRulesCourseAuthzPermissions, covering the
    ticket's acceptance criteria directly against the predicate: a course switched to authz
    grants access via authz_api.is_user_allowed alone; a course not yet switched keeps resolving through the
    legacy check unaffected; Content Library objects are
    unaffected.

Tests

  • Course switched, user holds the permission only via authz (no legacy role) → allowed,
    is_user_allowed called with the right identifier/scope.
  • Course switched, user has a real legacy CourseStaffRole but authz denies → denied; proves
    the switch is exclusive, not an OR with a legacy course-level role.
  • Course switched, user has org-level admin access but authz denies → denied; proves the
    exclusivity holds against org-admin fallback too, not just the course-level role.
  • Course not switched, user has a legacy role → allowed, unchanged from today, and
    is_user_allowed is never even called.
  • Content Library object, course-authz flag on → unchanged, still resolves through
    MANAGE_LIBRARY_TAGS (regression guard on the untouched branch).

Verification

  • pytest openedx/core/djangoapps/content_tagging/tests/test_rules.py openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py --no-cov:
    575 passed, 0 failed.
  • pylint on both changed source files and the changed test file: clean.

Manual testing

Manual testing notes for #795

Since #665 hasn't been merged, it isn't
possible to test that endpoint for now. The best approach I came up with is to test the branch
from the CLI.

  1. Create a new course at http://apps.local.openedx.io:2001/authoring/home.
  2. Create a waffle flag override for the course, authz.enable_course_authoring, and set it to
    Force Off and enabled, on the Django Admin page:
    http://studio.local.openedx.io:8001/admin/waffle_utils/waffleflagcourseoverridemodel/.
    (If you add a new user to a course that already has the flag Force On, it will automatically
    grant that user the authz permission instead of a legacy one.)
  3. Create a new user at http://studio.local.openedx.io:8001/admin/auth/user/add/.
  4. Add this new user to the course you created.
  5. Change the flag for this course to Force On to enable the new authz flow.
  6. Run this command to verify the branch that forces the new flow to be used:
docker exec tutor_dev-cms-1 python manage.py cms shell
from django.contrib.auth import get_user_model
from openedx.core.djangoapps.content_tagging.rules import can_change_object_tag_objectid

U = get_user_model()
authz_user = U.objects.get(username='authz_user')    # user with the new (authz) permissions
legacy_user = U.objects.get(username='legacy_user')  # user with the old (legacy) permissions
COURSE = 'course-v1:testcourse+34+34'  # replace with your course id

print(can_change_object_tag_objectid(authz_user, COURSE))   # expect True
print(can_change_object_tag_objectid(legacy_user, COURSE))  # expect False

To test ObjectTagOrgView itself, curl or Postman is needed, since a user with the old (legacy)
permissions can't even load the page for a course that uses the override flag.

To get the auth header (written with Claude's help):

COOKIES=/tmp/lms_cookies.txt; rm -f "$COOKIES"
curl -s -c "$COOKIES" "http://local.openedx.io:8000/login" -o /dev/null
CSRFTOKEN=$(grep csrftoken "$COOKIES" | awk '{print $NF}')
curl -s -b "$COOKIES" -c "$COOKIES" -H "X-CSRFToken: $CSRFTOKEN" -H "Referer: http://local.openedx.io:8000/login" \
  --data-urlencode "email=authz_user@test.com" --data-urlencode "password=test" \
  "http://local.openedx.io:8000/api/user/v1/account/login_session/" -o /dev/null -w "login status: %{http_code}\n"

AUTH_HEADER="Authorization: JWT $(grep "edx-jwt-cookie-header-payload" "$COOKIES" | awk '{print $NF}').$(grep "edx-jwt-cookie-signature" "$COOKIES" | awk '{print $NF}')"
CMS_URL="http://studio.local.openedx.io:8001"

For authz_user, expected 200:

curl -s -H "$AUTH_HEADER" -H "Content-Type: application/json" -X PUT "$CMS_URL/api/content_tagging/v1/object_tags/course-v1:testcourse+34+34/" -d '{"tagsData": [{"taxonomy": 36, "tags": ["Tag795"]}]}' -w "\nstatus: %{http_code}\n"

For legacy_user, expected 403. Log in again first with legacy_user's credentials (repeat
the auth-header block above with email=legacy_user@test.com, so $AUTH_HEADER reflects
legacy_user, not authz_user), then:

curl -s -H "$AUTH_HEADER" -H "Content-Type: application/json" -X PUT "$CMS_URL/api/content_tagging/v1/object_tags/course-v1:testcourse+34+34/" -d '{"tagsData": [{"taxonomy": 36, "tags": ["Tag795"]}]}' -w "\nstatus: %{http_code}\n"

Related to openedx/openedx-core#795 and openedx/openedx-core#665

🤖 Generated with Claude Code

can_change_object_tag_objectid in content_tagging/rules.py -- the predicate every direct
has_perm('oel_tagging.can_tag_object', ...) caller resolves through -- already checked the
new openedx-authz service for Content Library objects, but for course objects it only ever
checked the legacy has_studio_write_access role, regardless of whether that course had been
switched to the new authorization service. ObjectTagOrgView (Studio's own tag-editing
endpoint) already had the correct, toggle-aware behavior, but only inside that one view
class, so every other caller (including the CBE create-criterion endpoint, openedx-core#665)
silently got the wrong, legacy-only answer for switched-over courses.

Add a branch to can_change_object_tag_objectid, checked after the Content Library case and
before the legacy fallback: when should_use_course_authz_for_object says the course has been
switched, return authz_api.is_user_allowed directly, with no
fallback to legacy roles or org-admin access -- an exclusive switch, matching
ObjectTagOrgView's existing behavior exactly.

Delete ObjectTagOrgView.ensure_user_has_can_tag_object_permissions: the parent
ObjectTagView's own version (openedx-core) already resolves through
has_perm(oel_tagging.can_tag_object, ...), so this override had become pure duplication of
logic that now lives in rules.py. get_permissions and _authz_check stay, since
ensure_has_view_object_tag_permission (the separate, untouched view permission) and
ObjectTagTaxonomyOrgFilterBackend.filter_queryset both still read them directly.

Related to openedx/openedx-core#795

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Sep 16, 2026
@openedx-webhooks

Copy link
Copy Markdown

Thanks for the pull request, @alezconsultant!

This repository is currently maintained by @openedx/wg-maintenance-openedx-platform-oncall.

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:

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.

@github-project-automation github-project-automation Bot moved this to Needs Triage in Contributions Sep 16, 2026
@mphilbrick211 mphilbrick211 moved this from Needs Triage to Ready for Review in Contributions Sep 17, 2026

@mgwozdz-unicon mgwozdz-unicon left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks pretty good. Just a few things came out of the review Claude and I did together:

Requested changes

1. content_tagging/rest_api/v1/views.py:209-214: the docstring addition to _authz_check explains why it's kept even though the sibling override was deleted, which is genuinely useful since openedx-core#795's own Technical Details section suggested deleting _authz_check, get_permissions, and ensure_user_has_can_tag_object_permissions together. So I'd keep the substance, but the wording references "the now-deleted ensure_user_has_can_tag_object_permissions override", and a future reader who never saw that method won't have any context for that phrase. Can you reword it to state the current dependency only, for example: "Kept: ensure_has_view_object_tag_permission below and ObjectTagTaxonomyOrgFilterBackend.filter_queryset (filters.py) still read this directly."

2. PR Description's Manual testing section: this needs more detail to be reproducible. It doesn't name the waffle flag (AUTHZ_COURSE_AUTHORING_FLAG), doesn't say how it was toggled per course, doesn't name the specific screen or URL used, and doesn't say concretely what "tagging succeeded" looked like. Can you add those specifics, along the lines of: which flag, how you set the per-course override, which screen you used (the Course Team tag editor), and what confirmed success (for example, tags saved and visible again after a reload, with no 403).

3. content_tagging/tests/test_rules.py:812, test_course_switched_org_admin_only_role_denied: this duplicates test_course_switched_legacy_only_role_denied right above it; both exercise the exact same code path. Can you remove it?

4. Test coverage for objects within a course: the updated docstring in rules.py now explicitly claims the new branch covers "a course (or an object within one)", i.e. an xblock inside a switched course, and the code supports that (should_use_course_authz_for_object resolves a BlockUsageLocator down to its course key). None of the new tests in TestRulesCourseAuthzPermissions pass an xblock object_id though, only a bare course key. Since this is now a documented claim, can you add a test case for it?

Nits

1. content_tagging/rules.py:220-225: this docstring addition reads as two short paragraphs where one would do, for example: "For a course (or object within one) switched to openedx-authz, this requires courses.manage_tags through that service alone, with no fallback to studio write or org-admin access; for any other course, xblock, etc. not yet switched, those checks still apply exactly as before." Not blocking, just a length nit.

2. content_tagging/rules.py:245-246: the inline comment above the new branch ("Once a course is switched to openedx-authz, it is the sole source of truth for this permission...") restates the docstring paragraph directly above it in the same function almost word for word. Worth dropping one of the two.

@alezconsultant

alezconsultant commented Sep 18, 2026

Copy link
Copy Markdown
Contributor Author

@mgwozdz-unicon

  1. Docstring fixed.
  2. Updated PR manual testing section to better describe testing flow
  3. Remove redundant test
  4. Add xblock object_id to test in test_xblock_in_switched_course_authz_only_role_allowed
  5. Nits fixed.

@mgwozdz-unicon mgwozdz-unicon left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you for fixing those things. My final nit is just that I think the remaining lines of the comment on 209-212 of views.py could probably be deleted entirely, but feel free to keep it if you want.

@alezconsultant

Copy link
Copy Markdown
Contributor Author

@mgwozdz-unicon i think it's okay to remove it, done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

open-source-contribution PR author is not from Axim or 2U

Projects

Status: Ready for Review

Development

Successfully merging this pull request may close these issues.

4 participants