Conversation
Replace isAdminUser gate with userPermissions.hasViewPermissions(ResourceEntity.TYPE) in CustomPropertiesLandingPage, CustomPropertiesPanel, and ProfilePage so non-admin users holding a type-resource policy can access the AI-mode Custom Properties panel. Split the single raw EditAll read in CustomPropertiesDetailPage into three derived flags (canCreate, canDelete, canEditAll) via getDerivedPermissionFlags, matching the backend's per-operation authorization on TypeResource. Add Playwright permission tests: dataConsumer (no type access) sees no nav item; non-admin with full type access sees all entity cards and all action buttons. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
🔄 Playwright impact map auto-refreshedThis PR touched specs or UI source that changed the source→spec routing map. I regenerated What is this file? It is the auto-generated half of Playwright's PR planner. It routes "if source X changes, run specs Y" by walking spec imports and cross-referencing What if I want to regenerate locally instead? Run this before pushing your next change to skip the bot commit: python3 .github/scripts/generate_playwright_impact_map.py
git add .github/playwright/impact-map.generated.json
git commit --amend --no-edit # or a separate commit |
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
🔴 Playwright Results — workflow failedValidated commit ✅ 4492 passed · ❌ 4 failed · 🟡 13 flaky · ⏭️ 1 skipped · 🧰 0 lifecycle flaky ❌ Action needed: test(s) failed on every attempt against this PR’s validated commit — see Genuine Failures below. These are test failures, not CI budget or infrastructure issues. PerformanceBlocking targets: ✅ met · Optimization targets: 🟡 in progress Shard-job maxima below are not the full workflow wall time; the linked run includes build, fixture, planning, and reporting. 🕒 Full workflow signal wall (to summary) 38m 19s ⏱️ Max setup 4m 12s · max shard execution 22m 0s · max shard-job elapsed before upload 25m 38s · reporting 29s 🌐 218.74 requests/attempt · 2.24 app boots/UI scenario · 33.41% common-shard skew Optimization targets still in progress:
Genuine Failures (failed on all attempts)❌
|
Rohit0301
added
the
safe to test
… use UserClass/PolicyClass/RolesClass helpers Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🔄 Playwright impact map auto-refreshedThis PR touched specs or UI source that changed the source→spec routing map. I regenerated What is this file? It is the auto-generated half of Playwright's PR planner. It routes "if source X changes, run specs Y" by walking spec imports and cross-referencing What if I want to regenerate locally instead? Run this before pushing your next change to skip the bot commit: python3 .github/scripts/generate_playwright_impact_map.py
git add .github/playwright/impact-map.generated.json
git commit --amend --no-edit # or a separate commit |
Code Review
|
| Compact |
|
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source
| {(canEditAll || canDelete) && ( | ||
| <Box direction="row" gap={1}> | ||
| <Button | ||
| aria-label={t('label.edit')} | ||
| color="tertiary" | ||
| iconLeading={Edit} | ||
| size="xs" | ||
| onPress={() => onEditProperty(property)} | ||
| /> | ||
| <Button | ||
| aria-label={t('label.delete')} | ||
| color="tertiary-destructive" | ||
| iconLeading={Delete} | ||
| size="xs" | ||
| onPress={() => setPropertyToDelete(property)} | ||
| /> | ||
| {canEditAll && ( | ||
| <Button | ||
| aria-label={t('label.edit')} | ||
| color="tertiary" | ||
| iconLeading={Edit} | ||
| size="xs" | ||
| onPress={() => onEditProperty(property)} | ||
| /> | ||
| )} | ||
| {canDelete && ( | ||
| <Button | ||
| aria-label={t('label.delete')} | ||
| color="tertiary-destructive" |
There was a problem hiding this comment.
⚠️ Bug: canDelete missing from renderRow useCallback deps
This commit changes the delete-button gate from canEditAll to canDelete (lines 239, 250), but renderRow's useCallback dependency array (line 265) still only lists [canEditAll, onEditProperty, t] — canDelete is not included. permission starts as DEFAULT_ENTITY_PERMISSION (all false) and is populated asynchronously after getEntityPermission resolves. For a user granted Delete but not EditAll/Create, canEditAll never changes value across the async update, so the memoized renderRow is never recreated and captures the stale canDelete = false, hiding the Delete button despite valid permission. Add canDelete (and canCreate if used) to the dependency array.
Include canDelete in renderRow's useCallback dependency array so the row re-renders when delete permission resolves.:
[canDelete, canEditAll, onEditProperty, t]
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
| // User has Create → Add button visible. | ||
| await expect(page.getByTestId('add-custom-property-btn')).not.toBeVisible(); | ||
|
|
||
| // User has EditAll + Delete → Edit and Delete buttons visible on the seeded row. | ||
| const row = page.locator('tr').filter({ hasText: customPropertyName }); | ||
| await expect(row).toBeVisible(); | ||
| await expect(row.getByRole('button', { name: 'Edit' })).not.toBeVisible(); | ||
| await expect(row.getByRole('button', { name: 'Delete' })).not.toBeVisible(); |
There was a problem hiding this comment.
💡 Quality: Stale comments contradict assertions in ViewAll-only test
In the ViewAll-only describe block the comments were copied from the full-access test and now contradict the code: line 770 says "User has Create → Add button visible" immediately before expect(...).not.toBeVisible(), and line 773 says "Edit and Delete buttons visible" before asserting they are not visible. Update the comments to reflect that a ViewAll-only user has no action buttons, to avoid misleading future readers.
Was this helpful? React with 👍 / 👎
❌ UI Checkstyle Failed❌ ESLint + Prettier + Organise Imports (src)One or more source files have linting or formatting issues. Affected files
❌ Playwright - Guardrails + ESLint + Prettier + Organise ImportsEither a Playwright test file has linting/formatting issues, or a guardrail check failed: ESLint rule unit tests, a new guardrail violation, a stale suppression entry (its violation was fixed but the baseline was not pruned), a blanket Affected files
🔍 ESLint findings in this PR's files — 0 error(s), 1 warning(s)Errors block the build. Warnings do not yet — they are rules whose backlog is still 0 error(s), 1 warning(s) across 1 changed file(s).
All findings
Fix locally (fast - only checks files changed in this branch): make ui-checkstyle-changed |
1 participant
Describe your changes:
Fixes #32413
The AI-mode Custom Properties panel (
CustomPropertiesPanel) was gating all entity-type cards and the sidebar nav item onisAdminUser, ignoring the backend's policy-engine permission on thetyperesource. Non-admin users with a validtype-resource policy could callPUT /api/v1/metadata/types/{id}successfully via the API but saw an empty panel in the UI.Additionally,
CustomPropertiesDetailPagereadpermission[Operation.EditAll]directly (raw access, banned byopenmetadata-permissions/no-raw-permission-access) and collapsed all three actions into one flag, mismatching the backend's per-operation authorization.Changes (AI-mode panel only — legacy Settings route untouched):
CustomPropertiesLandingPage.tsx/CustomPropertiesPanel.tsx: ReplaceisAdminUser-based filter withuserPermissions.hasViewPermissions(ResourceEntity.TYPE, permissions). Users withViewAllorViewBasicontypesee all entity-type cards; others see nothing.ProfilePage.tsx: GateWORKSPACE_NAV_ITEMS(the "Custom Properties" sidebar entry) on the same permission, so users withouttypeview access never see the nav item.CustomPropertiesDetailPage.tsx: Replace rawpermission[Operation.EditAll]withgetDerivedPermissionFlags(permission)and split into three semantically correct flags —canCreate || canEditAllfor Add,canEditAllfor Edit,canDelete || canEditAllfor Delete — matching whatTypeResourceevaluates on the backend.Type of change:
High-level design:
The backend authorizes custom property management via
MetadataOperation.CREATEon thetyperesource (TypeResource.addOrUpdateProperty). Thetyperesource is entirely separate from data-entity resources (Table,Dashboard, etc.) —EditAllonTabledoes not grant any permission ontype.The fix has two layers:
ProfilePage+CustomPropertiesLandingPage+CustomPropertiesPanel): usesuserPermissions.hasViewPermissions(ResourceEntity.TYPE, permissions)— the same utility already used for Services, Teams, and other settings categories — to control whether the nav item and entity cards render.CustomPropertiesDetailPage):getEntityPermission(ResourceEntity.TYPE, entityType.id)already fetched the correctly-scopedOperationPermission;getDerivedPermissionFlagsderives named flags from it without needing the resource type again. Three flags replace the singlehasEditPermissionboolean.No legacy code (
GlobalSettingsClassBase,SettingsRouter,CustomPropertiesPageV1) was changed.Tests:
Use cases covered
typepermissions → "Custom Properties" nav item absent from AI profile sidebar[Create, Delete, EditAll, ViewAll]ontype→ nav visible, all entity-type cards shown, Add + Edit + Delete buttons visible on Table detail pageViewAllonly ontype(read-only) → nav visible, landing shows cards, zero action buttons (documented in plan; manual-verified scenario)EditAllonTabledata resource but notypewrite permissions → no action buttons (data-resource grants do not bleed into type-resource checks)Unit tests
PermissionDerivation.test.ts; the UI wiring is exercised by the Playwright tests below.Backend integration tests
Ingestion integration tests
Playwright (UI) tests
openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/AIMode/CustomPropertiesPanel.spec.tsCustom Properties Panel — user without type permissions(dataConsumer storageState)Custom Properties Panel — non-admin user with type permissions(creates policy/role/user via admin API inbeforeAll, tears down inafterAll)Manual testing performed
Resources: type,Operations: [Create, Delete, EditAll, ViewAll],Effect: AllowEditAllonTable+ViewAllon All (notypewrite) — nav visible, detail page accessible, zero action buttons confirmedtypepermissions) — "Custom Properties" nav item absentUI screen recording / screenshots:
TODO: attach screen recording — opening as draft until recording is added.
Checklist:
Fixes <issue-number>: <short explanation>Fixes #32413above.