Skip to content
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.

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

[Tagging] Permissions for Taxonomies List Page and Taxonomy Detail Page #129

Closed
bradenmacdonald opened this issue Sep 22, 2023 · 19 comments · Fixed by openedx/edx-platform#33413

Comments

@bradenmacdonald
Copy link
Contributor

bradenmacdonald commented Sep 22, 2023

Acceptance Criteria

  1. Make sure that the tagging system enforces all of the following permissions. Make changes to openedx_tagging and content_tagging as needed if some of these permissions aren't yet implemented correctly.
  2. Write tests to cover these if they are not yet tested. As much as possible the tests should use these same strings (see example below).

Who can view taxonomies:

includes seeing all tags and metadata. Does not necessarily allow the user to view ObjectTags using the taxonomy though.

  1. Users who are global staff can see all taxonomies (enabled and disabled)
  2. Users who are org-level admins (OrgStaffRole) can see all taxonomies that are linked to their organization (enabled and disabled).
  3. Any user with access to Studio can see enabled taxonomies that are part of “all orgs”.
  4. Only global staff ("instance-level admins") can see non-system taxonomies that are not linked to any organization yet.
  5. Users who are not global staff ("instance-level admins") or org-level staff ("org-level admins") cannot see any disabled taxonomies.
  6. Users with other org-level roles (OrgInstructorRole, OrgContentCreatorRole, or OrgLibraryUserRole) can see any of that org's enabled taxonomies.
  7. Users who are instructors for any course (via CourseStaffRole or CourseInstructorRole) belonging to an org can see that org’s enabled taxonomies.
  8. Users who can view any v2 content libraries belonging to an org (CAN_VIEW_THIS_CONTENT_LIBRARY) can view that org's enabled taxonomies.

This applies to the Taxonomies List Page as well as the Taxonomy Detail Page, though it must be implemented on the backend, not the frontend.

Who can export taxonomies

  1. Any user who can view a taxonomy can export it.
  2. Users who cannot view a taxonomy cannot export it.

Who can edit taxonomies:

  1. For MVP, only global staff ("instance-level superusers") can do the following in general. As well, org-level admins (OrgStaffRole) can do the following for taxonomies that are linked to their org:
    • import a taxonomy
    • enable/disable a taxonomy
    • edit a taxonomy's metadata
    • edit a taxonomy's tags
    • upload a new version of a taxonomy with different tags
    • delete a taxonomy
    • assign a taxonomy to organizations (global staff only)
  2. If an org-level admin imports or creates a taxonomy, it is automatically linked to their org.
  3. No users are allowed to edit system-defined taxonomies.

Who can view ObjectTags

  1. Any user who can view a content object (unit, component, or library component) in Studio can view that content object's tags.
  2. Any user who can not view a content object (unit, component, or library component) in Studio can not view that content object's tags.

Because of how the taxonomy permissions are defined, we don't really need to check if the user has permission to view the Taxonomy. Generally if the user can see the object, they'll be able to see the associated taxonomies. In the future we may extend these permissions to say that you can only view an ObjectTag if you can view the content object and you can view the taxonomy, but for now we only need to check the content object.

Who can edit ObjectTags

  1. Any user who can edit a content object (unit, component, or library component) in Studio can edit that content object's tags.
  2. Any user who can not edit a content object (unit, component, or library component) in Studio can not edit that content object's tags.

Developer Notes

  • These should all be implemented as separate permissions ("can view this taxonomy", "can import a new taxonomy", "can enable/disable this taxonomy") and then use rules to define who has that permission. Do not directly check "roles" in the API - check permissions which are defined by rules which in turn check roles.
  • The can_export_taxonomy permission should be defined as equal to the can_view_taxonomy permission.
  • As much as possible, permission tests in edx-platform should be clearly written using docstrings that are identical to the acceptance criteria above. e.g.
    def test_global_staff_see_all(self):
        """
        Users who are global staff can see all taxonomies (enabled and disabled)
        """
        ... # test the "list taxonomies" API and the "get taxonomy details" API
@bradenmacdonald bradenmacdonald changed the title Permissions for Taxonomies List Page and Taxonomy Detail Page [Tagging] Permissions for Taxonomies List Page and Taxonomy Detail Page Sep 22, 2023
@bradenmacdonald bradenmacdonald added the question ❔ Further information is requested label Sep 22, 2023
@ali-hugo
Copy link

@jmakowski1123 Would you review this ticket's description when you get the chance please? I think you and Jill discussed permissions in some depth.

@ali-hugo
Copy link

@bradenmacdonald I've asked Jenna to review this ticket as she is more up to speed on the permissions as I am. For what it's worth, everything outlined in "Acceptance Criteria" matches my understanding.

Question for product: Is everything here correct? In particular: can anyone who can view a taxonomy export it, or only global staff can export?

I agree with your point on #105 i.e. that if a user can view a taxonomy, they should be able to export/download it.

@jmakowski1123
Copy link

jmakowski1123 commented Sep 25, 2023

  1. Editing permissions:
    For MVP, only global staff ("instance-level superusers") can:
    import a taxonomy
    enable/disable a taxonomy
    edit a taxonomy's metadata
    edit a taxonomy's tags
    upload a new version of a taxonomy with different tags
    delete a taxonomy
    assign a taxonomy to organizations

Does this cover the use case of: I'm an org-level administrator for HarvardX on edX.org and I want to import a taxonomy for HarvardX. Because I'm just part of HarvardX and not edX.org staff, I only have access to HarvardX on the edX.org platform. ...? In other words, are instance-level superusers/global staff equal to org-level admins?

In the context of the new R&P framework edX is working on, the "Org Admin" should also have all editing permissions for taxonomies, right @bryan-kersten? However this role doesn't exist yet, so how does the HarvardX use case above work in practice in the interim? https://docs.google.com/spreadsheets/d/1htsV0eWq5-y96DZ5A245ukfZ4_qeH0KjHVaOyfqD8OA/edit#gid=908503896

  1. Import/export: For MVP, I'm inclined to say the permissions for exporting equal the permissions for importing, since the main/only reason a taxonomy will be exported will be to update it, which is a process limited to admins. We don't want course authors doing that.

@bradenmacdonald
Copy link
Contributor Author

@jmakowski1123 My understanding was that for MVP only global admins (not org admins) would be editing.

In other words, for MVP, if a HarvardX admin wanted to import a new taxonomy, they'd have to ask an edx.org admin to import it for them and link it to HarvardX.

If we need to include org-level admins as editors though it's no problem - but that's not what's shown in the mockups.

However this role doesn't exist yet

I'm surprised it says that because the existing OrgStaffRole is an org-level admin role as far as I know - it lets you view and edit all courses and enrollments and learner activity for the whole org.

@bradenmacdonald
Copy link
Contributor Author

@jmakowski1123 For (2) I think there are other reasons an instructor may wish to download (export) a taxonomy besides editing it and re-uploading it. For example, they may wish to upload the taxonomy to a sandbox or another Open edX instance where they are an admin, or write some scripts that use the taxonomy to tag their content automatically, or train some machine learning models, or other advanced usage like that. However, for MVP we can certainly just say "you can export if you can import" if you want, instead of "you can export if you can view". Either way is just as simple from a technical perspective.

The only thing is I think we talked about letting users export system-defined taxonomies, for advanced use cases like I mentioned, and it doesn't make sense to allow that in this case, since they cannot be imported.

@bradenmacdonald bradenmacdonald removed the question ❔ Further information is requested label Sep 26, 2023
@bradenmacdonald
Copy link
Contributor Author

After discussing this with @jmakowski1123 I have updated it: org-level admins will have broad editing abilities just like global staff, but within their own org. And export perms will be the same as view perms, for the benefit of power users that want to use scripts to manipulate taxonomies or content.

@rpenido
Copy link

rpenido commented Oct 3, 2023

Because of how the taxonomy permissions are defined, we don't really need to check if the user has permission to view the Taxonomy. Generally if the user can see the object, they'll be able to see the associated taxonomies. In the future we may extend these permissions to say that you can only view an ObjectTag if you can view the content object and you can view the taxonomy, but for now we only need to check the content object.

Agreed. The view taxonomy permission is the REST DETAIL one used in the CRUD, not if the user will actually view the ObjectTags within this taxonomy associated with the object.

But should we filter out disabled taxonomies in the GET ObjectTag endpoint?

CC @pomegranited

@bradenmacdonald
Copy link
Contributor Author

@rpenido Yes, I think we should. We keep the data from deleted taxonomies, but we don't display the tags to users.

@rpenido
Copy link

rpenido commented Oct 3, 2023

  1. Any user with access to Studio can see enabled taxonomies that are part of “all orgs”.

Is there an explicit role for studio access? Or this is a user that has any of the following roles:

  • global staff
  • org-level staff
  • org-level roles (OrgInstructorRole, OrgContentCreatorRole, or OrgLibraryUserRole)
  • instructors for any course (via CourseStaffRole or CourseInstructorRole)

If it is the latter, the other rules will cover 3.

  1. Users who are not global staff ("instance-level admins") or org-level staff ("org-level admins") cannot see any disabled taxonomies.

These "users" are from the rules 6, 7 and 8, right? Am I missing something?

@pomegranited
Copy link

@rpenido

Is there an explicit role for studio access?

You're right, I don't think there is an explicit role for Studio access. So the other rules cover #3.

  1. Users who are not global staff ("instance-level admins") or org-level staff ("org-level admins") cannot see any disabled taxonomies.
    These "users" are from the rules 6, 7 and 8, right? Am I missing something?

I'm not sure what you're asking here.. but another way to say this is "5. Users who are not global staff ("instance-level admins") or org-level staff ("org-level admins") can only see enabled taxonomies."

From what I understand from reading this ticket:

  • Anyone can see an enabled taxonomy.
  • Viewing a disabled "all orgs" taxonomy requires global staff ("instance-level admin") access.
  • Viewing a disabled taxonomy with no orgs attached requires global staff ("instance-level admin") access.
  • Viewing a disabled taxonomy with an org attached requires some sort of org permissions, and there's a bunch of different ways these can be determined.

@jmakowski1123 @bradenmacdonald is this ^ correct?

@rpenido
Copy link

rpenido commented Oct 3, 2023

Thank you for the answers, @pomegranited!

Anyone can see an enabled taxonomy.
Who is "anyone" here?

I'm asking because I'm assuming that regular users (who are not part of 6, 7, or 8) will not access the Taxonomy Admin Interface to see a Taxonomy list or detail (these permissions are used in the Taxonomy CRUD). They will probably use the ObjectTag endpoint to see Tags and values applied to a Course, not a Taxonomy Detail or Taxonomy List.

Should we leave the View Taxonomy of enabled taxonomies open or any logged user anyway? This doesn't seem right. We limit instructors to see only taxonomies from their organization, and let the regular user see any enabled taxonomy.

@pomegranited
Copy link

Should we leave the View Taxonomy of enabled taxonomies open or any logged user anyway?

@rpenido I'm not sure.. will have to let the Braden or Jenna confirm.

@rpenido
Copy link

rpenido commented Oct 3, 2023

Ok! Thank you @pomegranited! 😃

@bradenmacdonald
Copy link
Contributor Author

bradenmacdonald commented Oct 4, 2023

@rpenido @pomegranited

Any user with access to Studio can see enabled taxonomies that are part of [“all orgs”]

You are right, there actually is no Studio access role. So for now we can make it so that any registered user can view enabled taxonomies that are part of all orgs. But in the tests and rule definitions, it would be good to put a comment that says our preferred rule would be "Any user with access to Studio can see enabled taxonomies that are part of [“all orgs”]", but because there is currently no "has studio access" permission, we are going with "any registered user" for now.

Users who are not global staff ("instance-level admins") or org-level staff ("org-level admins") cannot see any disabled taxonomies.
These "users" are from the rules 6, 7 and 8, right? Am I missing something?

Yes, this is just emphasizing that the users from 6, 7, 8 as well as 3, can never see disabled taxonomies. Technically this rule isn't necessary because it is implied by the other rules, but I think it's helpful to include it as a separate test, which is why I listed it separately.

@pomegranited

From what I understand from reading this ticket:

  • Anyone can see an enabled taxonomy.

Not really. Regular users (non-admins) can only see enabled taxonomies that are linked to one of their orgs (6, 7, 8) or that are linked to "all orgs".

Viewing a disabled "all orgs" taxonomy requires global staff ("instance-level admin") access.
Viewing a disabled taxonomy with no orgs attached requires global staff ("instance-level admin") access.

Yes

Viewing a disabled taxonomy with an org attached requires some sort of org permissions, and there's a bunch of different ways these can be determined.

Not quite. Only admins can view disabled taxonomies so it's easy to determine who can view a disabled taxonomy with an org attached - only instance-level admins or org-level admins for that specific org. The rules from (6, 7, 8) don't apply to disabled taxonomies.

@rpenido

I'm asking because I'm assuming that regular users (who are not part of 6, 7, or 8) will not access the Taxonomy Admin Interface to see a Taxonomy list or detail (these permissions are used in the Taxonomy CRUD). They will probably use the ObjectTag endpoint to see Tags and values applied to a Course, not a Taxonomy Detail or Taxonomy List.

You are right about the first part - users who aren't part of 6, 7, 8 will not be using taxonomy features at all. However, they also won't be using the ObjectTag endpoints either. (You are right that maybe in the future Learners may "use the ObjectTag endpoint to see Tags and values applied to a Course", but we're not supporting about that use case for the MVP. The MVP is focused only on course authors in Studio.)

Generally only admins and course authors (i.e. 6, 7, 8) will be using the tagging APIs. Learners won't be using these APIs at all. If some random learner uses the API, they would be able to see the enabled "all orgs" taxonomies, but that's only because we don't have a way to properly implement rule 3 ("only users with studio access...").

Just to be clear, this ticket is only talking about API permissions for now, though the UI permissions will be the same.

Should we leave the View Taxonomy of enabled taxonomies open or any logged user anyway? This doesn't seem right. We limit instructors to see only taxonomies from their organization, and let the regular user see any enabled taxonomy.

See previous answers. A random/regular user can only view enabled "all orgs" taxonomies, and that's only because we can't properly implement rule 3 yet. If a taxonomy is linked to an org, only people who are associated with that org (2, 6, 7, 8) can view it; regular users cannot.

@bradenmacdonald
Copy link
Contributor Author

bradenmacdonald commented Oct 4, 2023

Here's another way to describe it, by taxonomy type. Maybe this is clearer?

  • Enabled Taxonomy linked to all orgs - any registered user can view the taxonomy and its tags (3)

  • Disabled Taxonomy linked to all orgs - only global staff can view the taxonomy and its tags (5)

  • Enabled Taxonomy linked to a specific org(s) - only users affiliated with the org(s) (2, 6, 7, 8) can view the taxonomy and its tags. Plus global staff can view it too (1).

  • Disabled Taxonomy linked to a specific org(s) - only org admins from the linked org(s) (2) can view the taxonomy and its tags. Plus global staff can view it too (1).

  • Enabled taxonomy linked to no orgs - only global staff can view it (4)

  • Disabled taxonomy linked to no orgs - only global staff can view it (5)

  • System-defined taxonomies - should act as if they're linked to "all orgs" for the purposes of these rules. Actually maybe we need a migration to link them to all orgs if we don't already have that in place.

@rpenido
Copy link

rpenido commented Oct 8, 2023

Thank you for the clarification!
One doubt about the CourseCreatorRole.
We used the CourseCreatorRole as someone who was an org admin for all orgs. Should we keep it that way?

@rpenido
Copy link

rpenido commented Oct 9, 2023

Thank you for the clarification!
One doubt about the CourseCreatorRole.
We used the CourseCreatorRole as someone who was an org admin for all orgs. Should we keep it that way?

@bradenmacdonald CC @pomegranited
Could you please give me some help here?

@bradenmacdonald
Copy link
Contributor Author

@rpenido
I'm not really sure what you mean. As far as I know, CourseCreatorRole has nothing to do with being an admin. There is no "org admin for all orgs"; there is just OrgStaffRole (org admin for one org) and global staff (admin for the entire instance including all orgs).

Course Creator just means you have permission to create a new course in Studio, and it's a fairly low-level permission that many people have. On some instances it's also disabled, so anyone can create courses without needing special permission.

@rpenido
Copy link

rpenido commented Oct 9, 2023

We were checking permissions using a user in the tests with this role before.

Your comments clarified the issue for me. Thank you @bradenmacdonald !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging a pull request may close this issue.

5 participants