-
Notifications
You must be signed in to change notification settings - Fork 5
docs: add ADR on Course Authoring Feature Flag Implementation Details #212
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
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,98 @@ | ||
| 0010: AuthZ for Course Authoring - Feature Flag Implementation Details | ||
| ###################################################################### | ||
|
|
||
| Status | ||
| ****** | ||
|
|
||
| **Draft** | ||
|
|
||
| Context | ||
| ******* | ||
|
|
||
| We need to implement the new RBAC functionality for course authoring behind a Waffle flag as a | ||
| beta feature for several key reasons: | ||
|
|
||
| - Enable incremental development without breaking existing functionality | ||
| - Allow large sites to test performance and functionality at small scale before full deployment | ||
| - Mitigate risks associated with the Verawood release timeline | ||
|
|
||
| The feature flag must support enablement at multiple granularity levels: | ||
|
|
||
| - **Course level**: Individual courses | ||
| - **Organization level**: All courses within an organization | ||
| - **Instance level**: All courses across the entire instance | ||
|
|
||
| Detailed implementation research can be found in the | ||
| `Authoring Waffle Flag Implementation Spike`_. | ||
|
|
||
| Decision | ||
| ******** | ||
|
|
||
| **Implementation Approach** | ||
|
|
||
| - Implement as a Waffle flag with multi-level support: | ||
|
|
||
| - **Course level**: Using `WaffleFlagCourseOverrideModel`_ | ||
| - **Organization level**: Using `WaffleFlagOrgOverrideModel`_ | ||
| - **Instance level**: Using standard Waffle Switch | ||
|
|
||
| - **Flag Configuration** | ||
|
|
||
| - Flag name: ``authz.enable_course_authoring`` | ||
| - Management via Django Admin UI or management command | ||
| - Deprecation scheduled after 2 Open edX releases (after Willow) | ||
|
|
||
| **Platform Integration** | ||
|
|
||
| - All openedx-platform code related to course authoring must respect the flag state | ||
| - Multi-course endpoints must handle both legacy and new AuthZ mechanisms simultaneously | ||
| - Bidirectional migration process will be implemented (details in separate ADR) | ||
|
|
||
| **Migration Behavior** | ||
|
|
||
| When the flag state changes, automatic migration occurs immediately: | ||
|
|
||
| - **Flag enabled**: Legacy role assignments migrate to new system and are removed from legacy | ||
| system | ||
| - **Flag disabled**: New system role assignments migrate to legacy system and are removed from | ||
| new system | ||
|
|
||
| *Note: Roles without legacy equivalents remain in the new system and are not migrated* | ||
|
|
||
|
|
||
| Consequences | ||
| ************ | ||
|
|
||
| - Documentation will be created for explaining the feature flag | ||
| - Documentation will be created for comparing roles between the legacy and new AuthZ systems | ||
| - A DEPR ticket will be created to track the deprecation of the feature flag after Willow | ||
|
|
||
| Rejected Alternatives | ||
| ********************* | ||
|
|
||
| **Instance-level only flag implementation** | ||
| Makes testing difficult on large instances with mixed requirements. | ||
|
|
||
| **Django setting implementation** | ||
| Requires deployment and service restart for flag changes, increasing testing complexity and risk. | ||
|
|
||
| **Per-user level implementation** | ||
| Creates inconsistent permission experiences within the same course. Course staff could | ||
| attempt to set permissions in one system for users operating in another system, resulting | ||
| in ineffective permission changes. | ||
|
|
||
| References | ||
| ********** | ||
|
|
||
| * `How to Implement the right toggle type`_ | ||
| * `Authoring Waffle Flag Implementation Spike`_ | ||
|
|
||
| .. _How to Implement the right toggle type: https://docs.openedx.org/projects/edx-toggles/en/ | ||
| latest/how_to/implement_the_right_toggle_type.html#implementing-the-right-toggle-class | ||
| .. _Authoring Waffle Flag Implementation Spike: https://openedx.atlassian.net/wiki/spaces/OEPM/ | ||
| pages/5646221313/Spike+-+RBAC+AuthZ+-+Authoring+Waffle+Flag+Implementation | ||
| .. _WaffleFlagCourseOverrideModel: https://github.com/openedx/openedx-platform/blob/ | ||
| 22485757573d32e3c0cb1c36855d83bcd2b1251d/openedx/core/djangoapps/waffle_utils/models.py#L14 | ||
| .. _WaffleFlagOrgOverrideModel: https://github.com/openedx/openedx-platform/blob/ | ||
| 22485757573d32e3c0cb1c36855d83bcd2b1251d/openedx/core/djangoapps/waffle_utils/models.py#L75 | ||
|
|
||
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m trying to understand this and think through the implications here. My concern is that as we incrementally add new roles and functionality, we may still need to retain legacy roles for areas we haven’t migrated yet.
For example, if we introduce new roles and permissions for managing a course’s advanced settings, which existing legacy roles would be removed or superseded? And do any of those legacy roles still participate in other course permission checks that haven’t yet been replaced?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be covered by this task: openedx/openedx-platform#37909
The compatibility layer should handle this. Now, we were specifying in that ticket to do the compatibility by comparing roles with their equivalents, but this means that this check wouldn't be a standard permission check for Casbin.
Perhaps, we can instead create a permission, like courses.legacy_staff_role_permissions (and others for the rest of existing roles), and check for that instead for the compatibility layer. This seems cleaner and in the future we just stop using that permission whenever the old system is fully deprecated.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one comment: I’d lean toward avoiding the removal of any legacy roles due to the scenario Taylor mentioned. If we do need to remove legacy roles, we may need an intermediate table to store existing legacy roles/permissions, since they could be involved in other permission checks outside of authoring (I’m not sure if this can actually happen).
Another option is to go with an “only code handler” approach, meaning this layer would not impact any rollback migrations. It’s true that this could add more complexity in the code, but once the uncertainty passes, we could deprecate this extra layer.
These are simply ideas to help think through the safest way to address the issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The detail here is that, because we will have ways for going back and forth between the legacy system and the new AuthZ system, if we kept data one side or the other, when going the other way we could have conflicts or unintended access to people that otherwise shouldn't have.
For example: Let's say that user1 has access to course1 in the legacy system. Then we migrate to the new system, and in the new system we remove access to course1 for that user. The we migrate back to the legacy system. If we didn't remove the old data, we would unintentionally give access back to course1 to user1.
That's why I'm proposing having a compatibility layer, in which any old code would still call the old functions, but at the core we would substitute the check for the new system in a compatible way.