Fixes #27487: restore import/export buttons when EditAll uses conditional policies#27488
Fixes #27487: restore import/export buttons when EditAll uses conditional policies#27488miantalha45 wants to merge 13 commits intoopen-metadata:mainfrom
Conversation
|
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! |
There was a problem hiding this comment.
Pull request overview
Restores visibility of Glossary Import/Export actions for users whose effective permissions depend on conditional policy evaluation, by considering entity-scoped permissions (with entity context) in addition to resource-scoped global permissions.
Changes:
- Update
importExportPermissionscomputation to fall back from resource-levelglobalPermissionsto entity-levelpermissionsfromGenericProvidercontext.
| const importExportPermissions = useMemo( | ||
| () => | ||
| checkPermission( | ||
| Operation.All, | ||
| ResourceEntity.GLOSSARY_TERM, | ||
| globalPermissions | ||
| ) || | ||
| checkPermission( | ||
| Operation.EditAll, | ||
| ResourceEntity.GLOSSARY_TERM, | ||
| globalPermissions | ||
| ), | ||
| [globalPermissions] | ||
| ) || | ||
| permissions[Operation.All] || | ||
| permissions[Operation.EditAll], | ||
| [globalPermissions, permissions] |
|
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}
| describe('import/export visibility with conditional policies', () => { | ||
| it('should show import/export when globalPermissions denies but entity-level permissions allow (isOwner condition satisfied)', async () => { | ||
| // Simulate a conditional policy: resource-level check returns false because | ||
| // the backend cannot evaluate isOwner() without entity context. | ||
| mockGlossaryTermPermission.All = false; | ||
| mockGlossaryTermPermission.EditAll = false; | ||
|
|
||
| // Entity-level permissions are fetched with the glossary ID so the backend | ||
| // correctly evaluates isOwner() and returns Allow. | ||
| mockContext.type = EntityType.GLOSSARY; | ||
| mockContext.permissions = { ...DEFAULT_ENTITY_PERMISSION, EditAll: true }; | ||
|
|
||
| render( | ||
| <GlossaryHeader | ||
| updateVote={mockOnUpdateVote} |
There was a problem hiding this comment.
💡 Quality: Mutable shared test state not reset between tests
The new describe block mutates module-level mockGlossaryTermPermission and mockContext (lines 330-336, 358-365) without resetting them afterward. While this matches the existing pattern in the file, the new tests depend on and further propagate leaked state. For example, the first new test (line 327) inherits mockContext.type from whatever the previous test set it to (line 233 sets it to GLOSSARY_TERM), then overrides it to GLOSSARY at line 335 — which then leaks into the second new test.
Currently this doesn't cause failures because test execution order is deterministic and these are the last tests, but it's fragile. Adding a beforeEach or afterEach in the new describe block to snapshot and restore the shared objects would make these tests order-independent.
Suggested fix:
describe('import/export visibility with conditional policies', () => {
let origPermission: typeof mockGlossaryTermPermission;
let origContext: typeof mockContext;
beforeEach(() => {
origPermission = { ...mockGlossaryTermPermission };
origContext = { ...mockContext };
});
afterEach(() => {
Object.assign(mockGlossaryTermPermission, origPermission);
Object.assign(mockContext, origContext);
});
// ... tests
});
Was this helpful? React with 👍 / 👎 | Reply gitar fix to apply this suggestion
aji-aju
added
the
safe to test
There was a problem hiding this comment.
Pull request overview
This PR fixes Glossary header Manage menu visibility for Import/Export when permissions are granted via conditional policies (e.g., isOwner()), by falling back to entity-level permissions (fetched with entity context) when resource-level/global permissions cannot be evaluated.
Changes:
- Update
importExportPermissionscomputation to OR global (resource-level) permission checks with entity-levelpermissionsfromGenericProvidercontext. - Add unit tests covering the conditional-policy scenario where global permissions deny but entity-level permissions allow.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| openmetadata-ui/src/main/resources/ui/src/components/Glossary/GlossaryHeader/GlossaryHeader.component.tsx | Adds entity-level permission fallback so Import/Export is shown when conditional policies evaluate to Allow with entity context. |
| openmetadata-ui/src/main/resources/ui/src/components/Glossary/GlossaryHeader/GlossaryHeader.test.tsx | Adds regression tests validating Import/Export visibility behavior under conditional policies. |
Jest test CoverageUI tests summary
|
|
@miantalha45 I have updated the branch to trigger CI runs |
🟡 Playwright Results — all passed (17 flaky)✅ 3956 passed · ❌ 0 failed · 🟡 17 flaky · ⏭️ 86 skipped
🟡 17 flaky test(s) (passed on retry)
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
|
@aji-aju Please check this PR and let me know if any changes are required. |
Code Review 👍 Approved with suggestions 0 resolved / 1 findingsRestores missing import/export buttons in the EditAll interface under conditional policies. Please reset the module-level mocks in the new test block to prevent shared state leakage between tests. 💡 Quality: Mutable shared test state not reset between testsThe new Currently this doesn't cause failures because test execution order is deterministic and these are the last tests, but it's fragile. Adding a Suggested fix🤖 Prompt for agentsOptionsDisplay: compact → Showing less information. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
|
Problem
Users with a Data Producer role (EditAll on glossaryTerm with conditions
like
isOwner(),hasDomain(), ormatchTeam()) could not see theImport/Export options in the Glossary header manage menu, even when the
condition was satisfied (e.g. the user was the owner of the glossary).
Root Cause
importExportPermissionswas computed solely fromglobalPermissions(resource-level, no entity context). The backend cannot evaluate
isOwner()at the resource level and returnsConditionalAllow, whichthe frontend maps to
false.Fix
Added a fallback to the entity-level
permissionsobject fromGenericProvidercontext. These permissions are fetched with the specificglossary entity ID, so the backend can correctly evaluate conditions and
return
Allowwhen the user satisfies them.Test
EditAllonglossaryTerm+ conditionisOwner()Fixes #27487
Summary by Gitar
This will update automatically on new commits.