Conversation
|
Thanks for the pull request, @Kelketek! 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. |
openedx-webhooks
added
open-source-contribution
Kelketek
changed the title
d1b890f to
702805f
Compare
5dbbc5b to
a12c7d7
Compare
|
@pkulkark This is ready for you. |
| def can_read(self, context_key): | ||
| """ Does the user have read access to the given course/library? """ | ||
| if isinstance(context_key, LibraryLocatorV2): | ||
| return authz_api.is_user_allowed( |
There was a problem hiding this comment.
Why not use the [user_has_permission_across_lib_authz_systems](https://github.com/openedx/openedx-platform/blob/master/openedx/core/djangoapps/content_libraries/api/libraries.py#L842) method? That seems to handle both legacy as well as authz grants and additional checks via the bridgekeeper rules behind has_perm.
There was a problem hiding this comment.
@pkulkark I tried that, but it violates the import linting rules. You are not permitted to import higher-level packages from lower-level ones.
I also had to move the service because the LMS isn't permitted to import from the CMS.
There was a problem hiding this comment.
I see. Perhaps we could directly add user.is_authenticated and user.is_active and user.is_staff in that case? Both are plain attribute checks in the bridgekeeper rules. Adding them to the library branch would close the gap where an inactive user with an authz role passes here, and the one where global staff get write on a course but not on a library through the same method.
As for the legacy grants, if we can't add an exception (like this example), could we atleast note in the docstring that this service does not see legacy grants?
There was a problem hiding this comment.
@pkulkark We don't have to worry about the legacy grants, as all of the grant info has been copied over by now and the legacy checks are just pending removal.
The staff check shouldn't be needed, as someone can be non-global staff and still have library access.
The anonymous check isn't necessary-- authz will return False. I've tested this:
In [1]: import openedx_authz.api as authz_api
In [2]: from django.contrib.auth.models import AnonymousUser
In [3]: user = AnonymousUser()
In [4]: from openedx_authz.constants.permissions import EDIT_LIBRARY_CONTENT, VIEW_LIBRARY
In [5]: authz_api.is_user_allowed(user, VIEW_LIBRARY.identifier, 'lib:Test:TL100')
is_active isn't checked by authz, and while in most cases it would probably be checked by the calling code during the request process, I've gone ahead and added a check to be safe.
|
Note: This PR is what makes the permission check in openedx/xblocks-core#291 work in v2 libraries. Since xblocks-contrib is a pip package that operators can bump independently of a platform upgrade, an operator could install the PDF conversion block on a platform that does not yet carry this change. In that state the check falls back to the Django staff flag, which blocks real library authors and lets any staff user write assets into libraries they can only view. Worth saying explicitly on both PRs which one has to land first. |
This has changed now that I made the permissions service more available. Now it requires the permissions service and no longer falls back. |
54a33ba to
14899bf
Compare
| """ | ||
| Service that can provide information about a user's permissions. | ||
|
|
||
| Deprecated. To be replaced by a more general authorization service. |
There was a problem hiding this comment.
"Hmm, this StudioPermissionsService is marked as Deprecated, and now this PR is un-deprecating it. I wonder who marked it as deprecated and what the replacement is."
(looks at git blame)
| def can_read(self, context_key): | ||
| """ Does the user have read access to the given course/library? """ | ||
| if isinstance(context_key, LibraryLocatorV2): | ||
| return self._user.is_active and authz_api.is_user_allowed( | ||
| self._user, | ||
| VIEW_LIBRARY.identifier, | ||
| str(context_key), | ||
| ) | ||
| return has_studio_read_access(self._user, context_key) | ||
|
|
||
| def can_write(self, context_key): | ||
| """ Does the user have write access to the given course/library? """ | ||
| if isinstance(context_key, LibraryLocatorV2): | ||
| return self._user.is_active and authz_api.is_user_allowed( | ||
| self._user, | ||
| EDIT_LIBRARY_CONTENT.identifier, | ||
| str(context_key), | ||
| ) | ||
| return has_studio_write_access(self._user, context_key) |
There was a problem hiding this comment.
I guess we need can_read and can_write for backwards compatibility, but we have much more fine-grained permissions now. Should we update the permissions service to support those fine-grained permissions instead?
There was a problem hiding this comment.
@bradenmacdonald I've pushed a change here that includes much more fine-grained permissions. I'm about to start adding tests for it now. Is this what you had in mind?
There was a problem hiding this comment.
@bradenmacdonald Cool-- I've cleaned it up a bit and added some tests. There was one hitch here-- it looks like the AuthZ perms for courses aren't actually added yet. So I put in a decorator shim to perform fallback behavior until they're working right.
There was a problem hiding this comment.
Hmm, am I missing something? We've been merging a whole bunch of PRs like this which use the new course authoring permissions to make parts of the Studio UI read-only. And I tested those PRs, and they were working as expected, so I thought the fine-grained Studio permissions were implemented.
|
|
||
| def create_library( | ||
| org: str, | ||
| org: Organization, |
There was a problem hiding this comment.
It's a little concerning that mypy hadn't flagged any error here ?
There was a problem hiding this comment.
@bradenmacdonald Yes, it is. And after playing with it for a bit, I can't seem to make it fail. I don't want to detour too far here, but something's up. I tried changing mypy's file includes to globs just to make sure the directory paths weren't insufficient, and I tried switching the types back and forth, but despite the fact I know some files that call this function are in the includes, they aren't considered wrong whether I set it to Organization or string, and whether I comment out the assert below or not.
18d2f1f to
4d4a7ac
Compare
|
@bradenmacdonald Ready for another pass. Thanks! |
4d4a7ac to
9dbbef4
Compare
9dbbef4 to
d05ddf1
Compare
This branch has not been deployed
5 participants
Description
This Pull Request makes the studio permissions service more consistently available, and adds support for Libraries v2 to it.
Supporting information
Internal ticket: https://tasks.opencraft.com/browse/BB-11135
Testing instructions
Other information
To check: Does this change make the service available in the LMS? Do we care if so?