[FC-0118] docs: add ADR for standardizing permissions usage#38184
Closed
taimoor-ahmed-1 wants to merge 31 commits intoopenedx:masterfrom
Closed
[FC-0118] docs: add ADR for standardizing permissions usage#38184taimoor-ahmed-1 wants to merge 31 commits intoopenedx:masterfrom
taimoor-ahmed-1 wants to merge 31 commits intoopenedx:masterfrom
Conversation
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @taimoor-ahmed-1! 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. |
Faraz32123
requested review from
Abdul-Muqadim-Arbisoft,
Faraz32123,
bradenmacdonald,
feanil and
wgu-jesse-stewart
…#38088) * refactor: use bumper_utils from xblocks-contrib package * refactor: use video_handlers from xblocks-contrib package * fix: fix test_video_handlers test cases
…dx#38128) Commit generated by workflow `openedx/openedx-platform/.github/workflows/upgrade-one-python-dependency.yml@refs/heads/master` Co-authored-by: bcitro <67378070+bcitro@users.noreply.github.com>
…penedx#38023) * feat: use new version of openedx-core * feat: Use openedx_catalog app, backfill it with all known courses * feat: properly set "created" timestamp on course runs during backfill * fix: better normalization of language codes * feat: keep courses in sync with CourseRun/CatalogCourse * feat: delete CourseRun/CatalogCourse when deleting a course * refactor: course_id -> course_key, run -> run_code, display_name -> title * fix: don't use SplitModulestoreCourseIndex for getting list of all courses
Commit generated by workflow `openedx/openedx-platform/.github/workflows/upgrade-one-python-dependency.yml@refs/heads/master` Co-authored-by: irtazaakram <51848298+irtazaakram@users.noreply.github.com>
This commit removes the following folders: * `xmodule/capa/` * `common/static/applets/capa/` * `common/static/images/capa/` * `common/static/js/capa/` These directories have been moved to `xblocks-contrib/problem` as-is and will now be maintained and used from there. All relevant imports in openedx-platform have been updated to reference the new paths in `xblocks-contrib/problem`. This removes duplication and consolidates CAPA-related code and static assets within the same repository as the Problem XBlock. Part of: openedx#36538
fix: handle duplicate enterprise group name Commit generated by workflow `openedx/openedx-platform/.github/workflows/upgrade-one-python-dependency.yml@refs/heads/master`
…ications (openedx#37039) When the Discussions MFE is enabled via the discussions.enable_discussions_mfe Waffle flag, users encounter noisy error logs like: Unable to retrieve details about Data for course_run course-v1:MITxT+21A.819.2x+3T2022 because Catalog Integration is not enabled This error occurs in deployments where the Discovery service (Catalog Integration) is not configured, but the system still attempts to fetch course duration data for notification filtering. The discussion notification system uses `filter_audit_expired_users_with_no_role` to exclude audit learners whose access has expired. This filtering logic calls: - `get_expected_duration()` to determine course duration - `get_course_run_details()` to fetch weeks_to_complete from Discovery - `check_catalog_integration_and_get_user()` which logs an error when Catalog Integration is disabled This commit modifies `get_expected_duration()` in course_date_signals/utils.py to: 1. Check if Catalog Integration is enabled before attempting to call Discovery 2. Use the default minimum duration (4 weeks) when Discovery is unavailable 3. Maintain existing behavior when Discovery is enabled This commit also adds configurable Django settings `COURSE_DURATION_MIN_WEEKS` (default: 4) and `COURSE_DURATION_MAX_WEEKS` (default: 18) to allow deployments to customize course duration bounds without code changes.
fix: moodle response code mapping Commit generated by workflow `openedx/openedx-platform/.github/workflows/upgrade-one-python-dependency.yml@refs/heads/master`
…onfig This refactoring moves all third-party authentication settings from runtime configuration during AppConfig.ready() to static definitions in lms/envs/common.py. ## Problem The third_party_auth app used an `apply_settings()` function called during Django's app initialization to modify Django settings. This pattern caused issues: 1. Settings were modified after Django initialization when they should be finalized, making debugging difficult 2. Operators couldn't override these settings in their YAML config files because apply_settings() would overwrite their values ## Why the ENABLE_THIRD_PARTY_AUTH conditional was removed The settings are now defined unconditionally because: 1. These settings are inert when social auth backends aren't configured in AUTHENTICATION_BACKENDS - they have no effect 2. ENABLE_THIRD_PARTY_AUTH still controls what matters: registration of authentication backends and exposure of auth URLs 3. This follows standard Django patterns where middleware and settings exist but are no-ops when their feature isn't active ## Enterprise pipeline integration The enterprise pipeline step (handle_enterprise_logistration) is included statically in SOCIAL_AUTH_PIPELINE rather than being inserted dynamically. This step handles its own runtime checks and returns early if enterprise is not configured, making it safe to include always. This avoids complexity with Derived() functions that would need to import Django models at settings load time (before apps are ready). ## Operator impact Operators can now properly override any of these settings in their YAML configuration files, including SOCIAL_AUTH_PIPELINE for custom flows. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…ise pipeline The settings tests were failing when run with CMS settings because the third_party_auth settings (SOCIAL_AUTH_PIPELINE, ExceptionMiddleware, etc.) are now defined only in lms/envs/common.py. Investigation confirmed CMS does not need these settings: - CMS has no social auth URL endpoints (third_party_auth URLs only included in LMS when ENABLE_THIRD_PARTY_AUTH is true) - CMS uses EdxDjangoStrategy for OAuth2 SSO with LMS, not ConfigurationModelStrategy for third-party identity providers - CMS authentication backends are EdXOAuth2 (LMS SSO) and LtiAuthenticationBackend, not social auth backends - The third_party_auth app is only in cms/envs/test.py INSTALLED_APPS to avoid import errors from indirect dependencies (like enterprise) Changes: - Added @skip_unless_lms decorator to SettingsUnitTest class - Added hasattr guard for SOCIAL_AUTH_PIPELINE in apps.py to prevent AttributeError when running under CMS (which doesn't have this setting) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Don't add a new reference to the old FEATURES dictionary and drop an unncessary test. Co-authored-by: Taylor Payne <taylor.payne2@wgu.edu> Co-authored-by: Feanil Patel <feanil@axim.org>
This change adds a new kind of generic user agreement that allows plugins or even the core platform to record a user's acknowledgement of an agreement.
Adds new models and API to store user agreements such as fair use agreements, terms of service, code of conduct etc. that need to be accepted by the user.
Remove Python 3.11 from all CI workflow matrices and update requires-python to >=3.12 in pyproject.toml. Also remove ubuntu-22.04 compatibility include entries from unit-tests.yml that were tied to Python 3.11 testing. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
pylint was upgraded from 3.x to 4.x by make upgrade, which now enforces the 120-char line limit more strictly. - cms/envs/devstack.py, production.py: split long lines in triple-quoted string literals (Markdown text, so Python newlines become spaces — no content change) - test_containers.py: add pylint disable/enable around OLX assertion block where XML attribute strings cannot be split Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
setuptools 82+ removed pkg_resources, which pyfilesystem2 (fs) still uses for namespace package declarations. The constraints.txt pin handles this for full installs, but the static-assets-check workflow only installs requirements/edx/assets.txt. Pre-installing pip-tools.txt ensures setuptools 81.x is in place before the assets install runs. See: PyFilesystem/pyfilesystem2#577 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
And add back and comment the old 3.11 tests for when we drop future major dependencies.
6.7.0 | invite new enterprise admins email Commit generated by workflow `openedx/openedx-platform/.github/workflows/upgrade-one-python-dependency.yml@refs/heads/master`
Currently, authorization logic is implemented inconsistently across views, serializers, and custom access checks. This ADR will define a consistent approach using DRF permission classes, object-level permissions, and queryset scoping where appropriate.
taimoor-ahmed-1
force-pushed
the
docs/ADR-standardize_permissions_usage
branch
from
6dd24d6 to
42b46d4
Compare
taimoor-ahmed-1
requested review from
a team,
farhan,
irtazaakram,
kdmccormick and
salman2013
as code owners
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
15 participants
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.
Description
Currently, authorization logic is implemented inconsistently across views, serializers, and custom access checks. This ADR will define a consistent approach using DRF permission classes, object-level permissions, and queryset scoping where appropriate.
Issue: #38177