Conversation
openedx-webhooks
added
open-source-contribution
|
Thanks for the pull request, @dcoa! This repository is currently unmaintained. 🔘 Find a technical reviewerTo get help with finding a technical reviewer, reach out to the community contributions project manager for this PR:
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. |
| # Packs the library and installs it into a frontend-platform/frontend-build MFE | ||
| # and a frontend-base site, then builds both. This is what keeps the package | ||
| # genuinely consumable by both architectures. | ||
| integration: |
There was a problem hiding this comment.
my intention here is garantee the library works for both making the install and build process in consumers. Let me know what do you think, if it is okay or may be too much for the size of the app.
|
|
||
| // On frontend-platform | ||
| import { getAuthenticatedHttpClient } from '@edx/frontend-platform/auth'; | ||
| configureAuthz({ getHttpClient: getAuthenticatedHttpClient, baseUrl }); |
There was a problem hiding this comment.
This is a proposal, another option could be add a provider. Let me know what do you think or if there is a better approach. :)
arbrandes
left a comment
arbrandes
left a comment
There was a problem hiding this comment.
I have several small concerns around the branching/publication strategy (basically, the repo doesn't follow the specs given in openedx/openedx-proposals#815), but that is minor. The main question is about the existence of this package in the first place: see inline comments.
| The long-term home for this functionality is most likely ``frontend-base``. What | ||
| makes a standalone package the right answer *now* is sequencing, not destination: | ||
| absorbing it today would force the coupling described above onto every MFE that | ||
| has not yet migrated. |
There was a problem hiding this comment.
My main question is: besides frontend-app-authoring, which MFEs/apps are going to need this functionality until Xylon is released?
If it's just frontend-app-authoring, I think we should just build this into frontend-base - but just for frontend-base apps to use. Legacy MFEs don't get it until they're converted to frontend-base.
There was a problem hiding this comment.
We actually started implementing this in frontend-base, but the feedback there was to extract it into a shared library instead: openedx/frontend-base#269 (comment).
For the current scope, we need this functionality in:
frontend-app-authoring, since we're working with course and library roles.frontend-app-admin-console.
Among frontend-base apps, I know this will also be important for the Instructor Dashboard.
As far as I know, that's the planned scope for Willow (@BryanttV, am I missing anything?).
There was a problem hiding this comment.
Ok, let's assume Instructor Dashboard will also need it (though I'd still like to know whether it actually needs it). That means two MFEs and one app. It makes the argument for this repo stronger.
However, there are two drawbacks I see:
- The API is by necessity different from what we're used to, precisely because this is not included in frontend-base or frontend-platform. It is a departure from our architectural philosophy of bundling basic functionality into one or the other.
- Once frontend-platform goes away, we'll want to bring this into frontend-base to stick with the philosophy, which will change the API again. It would be less work if it were already in frontend-base to begin with.
I'm open to being convinced, but I think right now we should stick to the status quo: either we implement this only in frontend-base, or we implement it both in frontend-base and frontend-platform. Trying to do both at the same time is never going to be neat (see https://github.com/openedx/frontend-base-compat, which is only acceptable because it has a set expiration date and it doesn't mess with the green-field API).
There was a problem hiding this comment.
I'm not opposed to there being a repo for this instead of bundling it in frontend-base, but if the only reason to make it standalone is to make it work with both frontend-base and frontend-platform I'm not on board.
I think the deciding question for me is: does it make sense for this to be a standalone package in a frontend-base only world?
There was a problem hiding this comment.
does it make sense for this to be a standalone package in a frontend-base only world?
Yeah, this boils it down.
I'll be glad to stand corrected, but even though openedx-authz is itself a separate package, if I understand it correctly it is very much a core thing, not meant to be replaced or pluggable. If this assumption is correct, then there's not much reason to have a separate frontend-authz.
| * Consumers must wire the library explicitly at startup. Forgetting | ||
| ``configureAuthz()`` is a runtime error rather than a compile-time one. |
There was a problem hiding this comment.
This is a departure from how auth works, currently: it's pretty much transparent to MFEs.
Unless of course by "consumers" what is meant here is frontend-base and frontend-platform.
5 participants
Description
Sets up this repository as a standalone package for
openedx-authzpermission validation: build and release tooling, a dual-architecture integration harness, and the ADR recording why the package exists at all.This PR is scaffolding only —
src/index.tsis still an empty barrel. The hooks and API client land in a follow-up, on top of the contract established here.The package is intended to be cosumend for MFEs either migrated or not to
frontend-basedocs/decisions/0001-standalone-package-for-authz-validation.rsthas the full reasoning, including the rejected alternatives and the intended retirement path — this is expected to be absorbed into frontend-base once enough MFEs have migrated, so the public API is kept deliberately small to make that a move rather than a rewrite.