[ON HOLD] CEXT-6151: Add ACL admin UI schema and helper to check permissions#426
[ON HOLD] CEXT-6151: Add ACL admin UI schema and helper to check permissions#426oshmyheliuk wants to merge 25 commits into
Conversation
🦋 Changeset detectedLatest commit: d222a25 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Update require() JSDoc to document fail-closed behavior when denyOnError: true. Add test case verifying require() throws AdminUiSdkPermissionError on 401. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Lib-api is designed as generic Commerce API utility library, this PR adds Admin UI SDK code. The precedent is Commerce Events: eventing is wired up during installation (lib-app), the actual API client lives in its own dedicated lib-events package. Admin UI SDK is the same pattern, lib-app owns the config schema and the installation step, but the permission client is a standalone API concern. I'd spawn a new package dedicated for Admin UI SDK. The cost is another package to maintain, but given that Admin UI SDK is still growing and under refinement, doing the split now is lower effort than after more surface has accumulated across lib-app and lib-api. |
| } | ||
|
|
||
| /** Client for checking the current user's Admin UI SDK resource permissions. */ | ||
| export interface AdminUiSdkPermissionClient { |
There was a problem hiding this comment.
Wouldn't be better to do not limit the scope to just permissions? What if we need to add in the future an operation for AdminUiSdk but not related to permissions? We will create a new client?
I'd propose to open the scope of the client, so other concepts fit in:
interface AdminUiSdkClient {
checkPermission()
invalidatePermission()
requirePermission()
}
There was a problem hiding this comment.
We already have an admin ui sdk related operation, but they are using the Commerce Client directly.
https://github.com/adobe/aio-commerce-sdk/blob/main/packages/aio-commerce-lib-app/source/management/installation/admin-ui-sdk/helpers.ts#L36
Probably can be moved to the "new Client" as well.
| } as const; | ||
|
|
||
| /** Wraps an App Builder action handler with an Admin UI SDK permission check. */ | ||
| export function withAdminUiSdkPermission< |
There was a problem hiding this comment.
I like this idea but not sure if we have this kind of wrappers anywhere else
There was a problem hiding this comment.
Yeah, I wasn't too sure whether I should add this. This wrapper is optional, I can remove it.
There was a problem hiding this comment.
Maybe we could just create a require helper function instead of wrapping.
requireAdminUiSdkPermission("Acme_Promotions::edit", client)
Does it make sense change resource as array? In case of multiple permissions are allowed? Or even multiple permissions could be required?
obarcelonap
left a comment
There was a problem hiding this comment.
LGTM but I'd like to hear your opinion about spawning a new package before approving.
|
Creating a new package sounds like a logical step to me, to align with what we already have for eventing/webhooks. |
| return { statusCode: 200, body: { updated: true } }; | ||
| } | ||
|
|
||
| export const main = async (params) => { |
There was a problem hiding this comment.
I overlooked the wrapper I thought it was meant to be used wrapping the whole function so you don't need to create the clients.
export const main = withAdminUiSdkPermission(
"Acme_Promotions::edit",
(params) => {
// protected handler here
},
)| @@ -0,0 +1,111 @@ | |||
| # `@adobe/aio-commerce-lib-admin-ui-sdk` Documentation | |||
There was a problem hiding this comment.
should we add experimental warning
There was a problem hiding this comment.
Added warning to the docs
| }); | ||
| ``` | ||
|
|
||
| In install/uninstall actions where only a subset of operations is needed, prefer `createCustomAdminUiSdkApiClient` to keep the bundle lean: |
There was a problem hiding this comment.
While I understand this is to be more performant I don't think it is an issue in the app-management endpoints since they're not in the critical path of the applications. Therefore I'd remove it for the sake of simplicity.
There was a problem hiding this comment.
I'd argue against removing it. I don't think it adds that much complexity and enables more flexibility to optimize bundle size. Not a blocker for now as it can be re-added at any time, but wanted to leave my opinion. Also it would be consistent with other packages which also have it.
| "CHANGELOG.md", | ||
| "README.md" | ||
| ], | ||
| "scripts": { |
There was a problem hiding this comment.
Missing docs script and @aio-commerce-sdk/config-typedoc devDependency
There was a problem hiding this comment.
Docs script was has been added
There was a problem hiding this comment.
review this docs because claude is flagging some ids may not work with the new validations
docs/usage.md in lib-app still has "my-app::menu" — which now fails the new regex
packages/aio-commerce-lib-app/docs/usage.md:476 still shows a hyphenated ID. The new id regex /^[A-Za-z0-9/:_]+$/ disallows hyphens, so any user copying from the docs
would get an unexpected parse failure. The example needs updating (e.g. "my_app::menu"). If real apps use hyphenated IDs this is potentially a major bump.
There was a problem hiding this comment.
Thanks for spotting this, the docs were updated according to the regex, where hyphens are not allowed.
| "dist", | ||
| "package.json", | ||
| "CHANGELOG.md", | ||
| "README.md" |
There was a problem hiding this comment.
This is missing, could you add a README to the new package? Something like https://github.com/adobe/aio-commerce-sdk/tree/main/packages/aio-commerce-lib-app
| extensionWorkspace: appData.workspaceName, | ||
| }, | ||
| }, | ||
| await adminUiSdkClient |
There was a problem hiding this comment.
I'd probably throw if process.env.__OW_NAMESPACE is not available. I know won't ever happen but using empty string as default feels is not correct.
| }; | ||
|
|
||
| /** Context shared across Admin UI SDK steps. */ | ||
| function createAdminUiSdkClient(params: RuntimeActionParams) { |
There was a problem hiding this comment.
this is very useful function, I feel we have the same for events clients. Should we export it and add it to usage.md?
There was a problem hiding this comment.
If we were to do this I would do it from each respective package, as it has nothing to do with any lib-app concern.
| @@ -0,0 +1,98 @@ | |||
| { | |||
| "name": "@adobe/aio-commerce-lib-admin-ui-sdk", | |||
There was a problem hiding this comment.
In #480 @danipt is suggesting to drop sdk term (see #480 (comment)). I'd rename to this package to @adobe/aio-commerce-lib-admin-ui
There was a problem hiding this comment.
Don't have a strong opinion here, but isn't the Commerce module named backend-uix or something like that? Maybe it's not as explicit as "admin-ui", but would help people correlate them more easily.
| id: aclResourceIdSchema("ACL resource ID"), | ||
| title: nonEmptyStringValueSchema("ACL resource title"), | ||
| parent: v.optional(aclResourceIdSchema("ACL resource parent")), | ||
| sortOrder: v.optional(positiveNumberValueSchema("ACL resource sortOrder")), |
There was a problem hiding this comment.
positiveNumberValueSchema should probably be used in sortOrder of MenuItemSchema
| Add `@adobe/aio-commerce-lib-admin-ui-sdk` library for checking Admin UI SDK ACL resource permissions. | ||
| Add Admin UI SDK API exports to `@adobe/aio-commerce-sdk`. |
There was a problem hiding this comment.
These should be two separate changesets. Otherwise the CHANGELOG of admin-ui-sdk will include this line about "adding exports to aio-commerce-sdk", which should not.
| @@ -0,0 +1,98 @@ | |||
| # `@adobe/aio-commerce-lib-admin-ui-sdk` Documentation | |||
|
|
|||
| > **Experimental:** This package is not yet production-ready. The API may change in future releases. | |||
There was a problem hiding this comment.
Just so that it stands out a bit better as a warning.
| > **Experimental:** This package is not yet production-ready. The API may change in future releases. | |
| > [!WARNING] | |
| > **Experimental**: This package is not yet production-ready. The API may change in future releases. |
| export async function registerExtension( | ||
| httpClient: AdobeCommerceHttpClient, | ||
| params: ExtensionRegistrationParams, | ||
| fetchOptions?: Options, |
There was a problem hiding this comment.
Probably not urgent, but be aware of #462 which changes this type to decouple our methods from ky (which was blocking a major library update). If the other PR merges first you'll need to update it
| extensionName: v.string(), | ||
| extensionTitle: v.string(), | ||
| extensionUrl: v.string(), | ||
| extensionWorkspace: v.string(), | ||
| }); |
There was a problem hiding this comment.
Should we add a bit more strict validation? Or is this done by the Commerce module? Such as v.url for the extensionUrl or some regex patterns/length checks for those that we know. I would argue it's useful to catch errors as early as possible (avoiding network round-trips).
| await httpClient.delete( | ||
| `adminuisdk/extension/${workspaceName}/${extensionName}`, | ||
| fetchOptions, | ||
| ); |
There was a problem hiding this comment.
I did not verify this, but I think that when you want to return Promise<void> you need to use .then(() => {}) with an empty handler, like in here
| "directory": "packages/aio-commerce-lib-admin-ui-sdk" | ||
| }, | ||
| "publishConfig": { | ||
| "exports": { |
There was a problem hiding this comment.
I believe you're missing the root entrypoint of source/index.ts?
| "@aio-commerce-sdk/config-vitest": "workspace:*", | ||
| "@aio-commerce-sdk/scripting-utils": "workspace:*", | ||
| "@aio-commerce-sdk/scripts": "workspace:*", | ||
| "msw": "catalog:", |
There was a problem hiding this comment.
Is this dependency used? I believe it's for integration tests, but I didn't see any of those, only unit.
| import { baseConfig } from "@aio-commerce-sdk/config-vitest/vitest.config.base"; | ||
| import { defineConfig, mergeConfig } from "vitest/config"; | ||
|
|
||
| const BARREL_FILES = ["./source/index.ts"]; |
There was a problem hiding this comment.
This can also use a glob to any index.ts, as there are currently two of them.
| }; | ||
|
|
||
| /** Context shared across Admin UI SDK steps. */ | ||
| function createAdminUiSdkClient(params: RuntimeActionParams) { |
There was a problem hiding this comment.
If we were to do this I would do it from each respective package, as it has nothing to do with any lib-app concern.
There was a problem hiding this comment.
This is also missing the admin-ui-sdk root entrypoint.
|
@obarcelonap @iivvaannxx I extracted the new package creation to a separate pull request: #489. |
Consolidate admin UI package into renamed @adobe/aio-commerce-lib-admin-ui; port ACL permission-checking feature onto main's refined extensions package. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
This doesn't need to target @adobe/aio-commerce-sdk, we don't do updates in that package for anything that changes in their subpaths, because it's "implicit" that they are always 1:1. Because you're updating admin-ui, the metapackage will also release a patch and get updated automatically.
|
Recreated the PR -> #522 |
Description
https://jira.corp.adobe.com/browse/CEXT-6151
Related Issue
https://github.com/magento-commerce/adobe-commerce-backend-uix/pull/352
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: