forked from openedx/edx-platform
-
Notifications
You must be signed in to change notification settings - Fork 7
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
feat(palm)!: backport instructor APIs conversion to DRF #702
Draft
Agrendalath
wants to merge
6
commits into
opencraft-release/palm.1
Choose a base branch
from
agrendalath/backport-instructor-drf-api-for-roles-palm
base: opencraft-release/palm.1
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
feat(palm)!: backport instructor APIs conversion to DRF #702
Agrendalath
wants to merge
6
commits into
opencraft-release/palm.1
from
agrendalath/backport-instructor-drf-api-for-roles-palm
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Added new authentication classes to be used in DEFAULT_AUTHENTICATION_CLASSES that include observability. This will enable us to see more about the endpoints using the defaults, to help us make choices about changes in the defaults. We make the DRF default of Session and Basic Authentication explicit by setting DEFAULT_AUTHENTICATION_CLASSES explicitly. (cherry picked from commit b9134c6)
Removed BasicAuthentication as a default from DRF endpoints that have not overridden the authentication classes. It appears this is not in use, and was just implicitly a default because it came from DRF's defaults. See DEPR: openedx#33028 (cherry picked from commit 7113624)
By default DRF sets 'DEFAULT_AUTHENTICATION_CLASSES' to: ``` [ 'rest_framework.authentication.SessionAuthentication', 'rest_framework.authentication.BasicAuthentication' ] ``` We also want to allow for JWT Authentication as a valid default auth choice. This will allow users to send JWT tokens in the authorization header to any existing API endpoints and access them. If any APIs have set custom authentication classes, this will not override that. I believe this is a fairly safe change to make since it only adds one authentication class and does not impact authorization of any of the endpoints that might be affected. Note: This change changes the default for both the LMS and CMS because `cms/envs/common.py` imports this value from the LMS. BREAKING CHANGE: For any affected endpoint that also required the user to be authenticated, the endpoint will now return a 401 in place of a 403 when the user is not authenticated. - See [these DRF docs](https://github.com/encode/django-rest-framework/blob/master/docs/api-guide/authentication.md#unauthorized-and-forbidden-responses) for a deeper explanation about why this changes. - Here is [an example endpoint](https://github.com/openedx/edx-platform/blob/b8ecfed67dc0520b8c4d95de3096b35acc083611/openedx/core/djangoapps/embargo/views.py#L20-L21) that does not override defaults and checks for IsAuthenticated. Generally speaking, this is should not be a problem. An issue would appear only if the caller of the endpoint is specifically handling 403s in a way that would be missed for 401s. (cherry picked from commit 7af2b1d)
Adding generic permission class. Added standard authentication classes. (cherry picked from commit 39dd3c0)
* feat: upgrading simple api to drf compatible. (cherry picked from commit 99760f8)
(cherry picked from commit af9ae77)
Agrendalath
changed the title
feat!(palm): :backport instructor DRF APIs
feat(palm)!: :backport instructor DRF APIs
Oct 22, 2024
Agrendalath
changed the title
feat(palm)!: :backport instructor DRF APIs
feat(palm)!: backport instructor DRF APIs
Oct 22, 2024
Agrendalath
changed the title
feat(palm)!: backport instructor DRF APIs
feat(palm)!: backport instructor APIs conversion to DRF
Oct 22, 2024
@Agrendalath What is the purpose of this PR? Should I just ignore it in the context of the upgrade? |
@Cup0fCoffee, it's okay to ignore this one. These commits should be present in Sumac anyway. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
TODO: Add testing instructions.
curl -X POST -d "grant_type=password&client_id=test&client_secret=test&username=<email>&password=<password>&token_type=jwt" http://localhost:18000/oauth2/access_token
http://localhost:18000/courses/course-v1:edX+DemoX+Demo_Course/instructor/api/modify_access
form-data:
http://localhost:18000/courses/course-v1:edX+DemoX+Demo_Course/instructor/api/list_course_role_members
form-data:
Private-ref: BB-9250