-
Notifications
You must be signed in to change notification settings - Fork 5
docs: add ADR for overall strategy for implementing AuthZ for Course Authoring #208
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
Conversation
|
Thanks for the pull request, @rodmgwgu! 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. |
…Authoring docs: add ADR for overall strategy for implementing AuthZ for Course Authoring
8942ac3 to
7cf19e6
Compare
| the same approach used for Libraries. | ||
| * Course authoring permission enforcement using the new system will be optionally enabled via a | ||
| feature flag. | ||
| * The feature flag can be enabled instance-wide or for specific courses. |
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.
Are we not doing an org-wide flag for this phase? I'm not opposed, just want it to be explicit one way or the other.
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.
Yes, this has been specified here: #212
| This phase focuses on migrating course authoring permissions while maintaining current | ||
| functionality. | ||
|
|
||
| * **Migration Script**: Transform existing role assignments into the new authorization model |
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.
Are we going to support a version of this that works based on changes in the Django admin? That interface is available for people to add waffle flags, so if that make that change without using the script we would be in a place of trying to us RBAC without the permissions being migrated.
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 is further specified here: #213
bmtcril
left a comment
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 a couple of questions for clarity
|
|
||
| A rollback migration process will also be provided to revert from openedx-authz roles back to | ||
| legacy roles if the feature flag is disabled. The rollback will only support roles with exact | ||
| equivalences between systems; non-equivalent roles will be ignored with warnings logged to the |
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.
What is the expected behavior in this scenario?
When the feature flag is enabled, a migration script is expected to run, and when it is disabled, a rollback is expected to be executed. Is this correct?
If the feature flag is enabled again later by an authorized user, will the migration scripts run again?
This is to clarify whether this behavior should be documented here or covered in separate migration or operational documentation.
Also, is it worth mentioning that operators are the ones who can enable or disable this flag?
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.
Thanks for the feedback, I added those details in this other ADR: #212
the flag will be managed either from the Django Admin (superusers), or via a management commant (operators)
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.
Thanks @rodmgwgu
sarina
left a comment
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.
Broadly I think this looks great. I'm wondering if we'd want to have specific release dates/timelines in here (ie what we're targeting for Verawood, that the deprecation is aimed for Xylon, etc)
wgu-taylor-payne
left a comment
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.
Overall this looks good to me. I don't have any unique feedback to add.
dwong2708
left a comment
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.
Looks good to me. Thanks!
bmtcril
left a comment
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 think my questions were answered in the other ADR 👍
This PR adds an ADR specifying the overall implementation strategy for AuthZ for Course Authoring, including:
Further details on the feature flags and migration process will be defined in additional ADRs.
Related issue: #184
Merge checklist:
Check off if complete or not applicable: