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 2 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! |
| 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
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