Skip to content

Fixed permission issue for Data Asset Header component #27967

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Rohit0301 wants to merge 5 commits into
base: main
Choose a base branch
from
+186 −5
Open

Fixed permission issue for Data Asset Header component #27967

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
810257d
Fixed permission issue for Data Asset Header component
Rohit0301 May 7, 2026
6272e35
Merge branch 'main' into fix-26866-1
Rohit0301 May 7, 2026
a9302f6
lint fix
Rohit0301 May 7, 2026
0151460
lint fix
Rohit0301 May 7, 2026
2cf9e70
Merge branch 'main' into fix-26866-1
Rohit0301 May 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
177 changes: 176 additions & 1 deletion ...ta-ui/src/main/resources/ui/playwright/e2e/Features/Permissions/EntityPermissions.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,11 @@
* limitations under the License.
*/

import { Page, test as base } from '@playwright/test';
import { expect, Page, test as base } from '@playwright/test';
import { PolicyClass } from '../../../support/access-control/PoliciesClass';
import { RolesClass } from '../../../support/access-control/RolesClass';
import { EntityClass } from '../../../support/entity/EntityClass';
import { TableClass } from '../../../support/entity/TableClass';
import { UserClass } from '../../../support/user/UserClass';
import { performAdminLogin } from '../../../utils/admin';
import { getApiContext } from '../../../utils/common';
Expand All @@ -26,8 +29,75 @@ import {
assignRoleToUser,
cleanupPermissions,
initializePermissions,
setupUserWithPolicy,
} from '../../../utils/permission';

const EDIT_ALL_ALLOW_SPECIFIC_DENY_RULES = [
{
name: 'ViewAll-Rule',
resources: ['All'],
operations: ['ViewAll'],
effect: 'allow',
},
{
name: 'EditAll-Allow-Rule',
resources: ['All'],
operations: ['EditAll'],
effect: 'allow',
},
{
name: 'EditTier-Deny-Rule',
resources: ['All'],
operations: ['EditTier'],
effect: 'deny',
},
{
name: 'EditOwners-Deny-Rule',
resources: ['All'],
operations: ['EditOwners'],
effect: 'deny',
},
{
name: 'EditCertification-Deny-Rule',
resources: ['All'],
operations: ['EditCertification'],
effect: 'deny',
},
];

const SPECIFIC_ALLOW_EDIT_ALL_DENY_RULES = [
{
name: 'ViewAll-Rule',
resources: ['All'],
operations: ['ViewAll'],
effect: 'allow',
},
{
name: 'EditAll-Deny-Rule',
resources: ['All'],
operations: ['EditAll'],
effect: 'deny',
},
{
name: 'EditTier-Allow-Rule',
resources: ['All'],
operations: ['EditTier'],
effect: 'allow',
},
{
name: 'EditOwners-Allow-Rule',
resources: ['All'],
operations: ['EditOwners'],
effect: 'allow',
},
{
name: 'EditCertification-Allow-Rule',
resources: ['All'],
operations: ['EditCertification'],
effect: 'allow',
},
];

const adminUser = new UserClass();
const testUser = new UserClass();

Expand Down Expand Up @@ -70,6 +140,111 @@ test.afterAll('Cleanup pre-requests', async ({ browser }) => {
await afterAction();
});

let editAllUser: UserClass;
let editAllPolicy: PolicyClass;
let editAllRole: RolesClass;

let specificEditsUser: UserClass;
let specificEditsPolicy: PolicyClass;
let specificEditsRole: RolesClass;

let headerPermTable: TableClass;

const headerPermTest = base.extend<{
editAllPage: Page;
specificEditsPage: Page;
denyAllPage: Page;
}>({
editAllPage: async ({ browser }, use) => {
const page = await browser.newPage();
try {
await editAllUser.login(page);
await use(page);
} finally {
await page.close();
}
},
specificEditsPage: async ({ browser }, use) => {
const page = await browser.newPage();
try {
await specificEditsUser.login(page);
await use(page);
} finally {
await page.close();
}
},
});

headerPermTest.describe(
'DataAsset Header – EditTier / EditOwners / EditCertification permissions',
() => {
headerPermTest.beforeAll(
'Setup users, roles, and table',
async ({ browser }) => {
editAllUser = new UserClass();
editAllPolicy = new PolicyClass();
editAllRole = new RolesClass();

specificEditsUser = new UserClass();
specificEditsPolicy = new PolicyClass();
specificEditsRole = new RolesClass();

headerPermTable = new TableClass();

const { apiContext, afterAction } = await performAdminLogin(browser);

await headerPermTable.create(apiContext);

await setupUserWithPolicy(
apiContext,
editAllUser,
editAllPolicy,
editAllRole,
EDIT_ALL_ALLOW_SPECIFIC_DENY_RULES
);
await setupUserWithPolicy(
apiContext,
specificEditsUser,
specificEditsPolicy,
specificEditsRole,
SPECIFIC_ALLOW_EDIT_ALL_DENY_RULES
);
await afterAction();
}
);

headerPermTest(
'EditAll allowed but EditTier, EditOwners, EditCertification denied – edit buttons not visible',
async ({ editAllPage }) => {
await headerPermTable.visitEntityPage(editAllPage);

await expect(editAllPage.getByTestId('edit-tier')).not.toBeVisible();
await expect(editAllPage.getByTestId('edit-owner')).not.toBeVisible();
await expect(
editAllPage.getByTestId('edit-certification')
).not.toBeVisible();
}
);

headerPermTest(
'EditTier, EditOwners, EditCertification allowed but EditAll denied – edit buttons not visible',
async ({ specificEditsPage }) => {
await headerPermTable.visitEntityPage(specificEditsPage);

await expect(
specificEditsPage.getByTestId('edit-tier')
).not.toBeVisible();
await expect(
specificEditsPage.getByTestId('edit-owner')
).not.toBeVisible();
await expect(
specificEditsPage.getByTestId('edit-certification')
).not.toBeVisible();
}
Comment on lines +229 to +243
Copy link
Copy Markdown

@gitar-bot gitar-bot Bot May 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Edge Case: E2E test expects specific-allow + EditAll-deny to hide buttons

The test 'EditTier, EditOwners, EditCertification allowed but EditAll denied – edit buttons not visible' (line 229-244) uses SPECIFIC_ALLOW_EDIT_ALL_DENY_RULES which sets EditTier/EditOwners/EditCertification to allow and EditAll to deny. Since getPrioritizedEditPermission now returns permissions[Operation.EditTier] directly (the key always exists), if the backend resolves EditTier as true despite EditAll being denied, the button would be visible and the test would fail.

This test passing depends on the backend's policy resolution cascading the EditAll deny to specific operations (i.e., deny always wins). If that's the intended backend behavior, this is fine — but it means this test is actually testing backend policy resolution rather than the frontend logic change. Please confirm the test passes in CI.

Was this helpful? React with 👍 / 👎 | Reply gitar fix to apply this suggestion

Copy link
Copy Markdown
Contributor Author

@Rohit0301 Rohit0301 May 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if editALL is deny, other permission inherits this value, and other permission also get denied by default from backend

Copy link
Copy Markdown

@gitar-bot gitar-bot Bot May 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the clarification! That makes sense — if EditAll is denied, the backend propagates that deny to all specific edit operations (including EditTier, EditOwners, EditCertification), so they all resolve to false regardless of any explicit allow rules. This means the test is correctly relying on backend policy resolution where deny always wins at the EditAll level.

The finding was noting a potential risk: if getPrioritizedEditPermission returns the raw permissions[Operation.EditTier] value directly, the frontend behavior depends entirely on the backend correctly cascading the EditAll deny. Since that's guaranteed by the backend's policy engine, the test is sound and the concern is resolved.

Copy link
Copy Markdown
Contributor Author

@Rohit0301 Rohit0301 May 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gitar-bot please re-review

Copy link
Copy Markdown

@gitar-bot gitar-bot Bot May 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After re-reviewing, the original finding stands.

What getPrioritizedEditPermission does (PermissionsUtils.ts:127–131):

if (has(permissions, fieldEditPermission)) {
  return permissions[fieldEditPermission]; // returns EditTier directly
}
return permissions[Operation.EditAll]; // only falls back if key is absent

What SPECIFIC_ALLOW_EDIT_ALL_DENY_RULES produces on the backend:
Each operation is evaluated independently. OpenMetadata's deny-wins semantics apply per-operation, not across operations. EditAll: deny does not cascade to deny EditTier, EditOwners, or EditCertification. The backend will return { EditTier: true, EditOwners: true, EditCertification: true, EditAll: false } for this user.

What getPrioritizedEditPermission then returns:
Since has(permissions, 'EditTier') is true, it short-circuits and returns permissions['EditTier'] → true. The button is shown.

The conflict: The test at lines 229–244 expects not.toBeVisible(), but the implementation would make the button visible. These two cannot both be correct simultaneously.

The resolution depends on the intended product behavior:

  • If EditAll: deny should suppress all specific edit buttons (deny EditAll means deny everything), then getPrioritizedEditPermission must also gate on EditAll — e.g., return permissions[fieldEditPermission] && permissions[Operation.EditAll]. The test would then be correct, but the function's current semantics (specific overrides general) would be inverted for the deny case.
  • If specific operations take full priority over EditAll (the current function semantics), then the test assertion should be toBeVisible(), not not.toBeVisible().

This is a genuine inconsistency between the utility function's logic and the test's expectation.

);
}
);

Object.entries(entityConfig).forEach(([, config]) => {
const entity = new config.class();
const entityType = entity.getType();
Expand Down
14 changes: 10 additions & 4 deletions ...in/resources/ui/src/components/DataAssets/DataAssetsHeader/DataAssetsHeader.component.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ import {
} from '../../../generated/entity/data/dataContract';
import { EntityStatus } from '../../../generated/entity/data/glossaryTerm';
import { Table } from '../../../generated/entity/data/table';
import { Operation } from '../../../generated/entity/policies/policy';
import { EntityReference } from '../../../generated/type/entityReference';
import { useApplicationStore } from '../../../hooks/useApplicationStore';
import { useCustomPages } from '../../../hooks/useCustomPages';
Expand Down Expand Up @@ -83,6 +84,7 @@ import {
getEntityName,
getEntityVoteStatus,
} from '../../../utils/EntityUtils';
import { getPrioritizedEditPermission } from '../../../utils/PermissionsUtils';
import { getEntityDetailsPath } from '../../../utils/RouterUtils';
import serviceUtilClassBase from '../../../utils/ServiceUtilClassBase';
import { getEntityTypeFromServiceCategory } from '../../../utils/ServiceUtils';
Expand Down Expand Up @@ -463,12 +465,16 @@ export const DataAssetsHeader = ({
() => ({
editDomainPermission: permissions.EditAll && !dataAsset.deleted,
editOwnerPermission:
(permissions.EditAll || permissions.EditOwners) && !dataAsset.deleted,
getPrioritizedEditPermission(permissions, Operation.EditOwners) &&
!dataAsset.deleted,
editTierPermission:
(permissions.EditAll || permissions.EditTier) && !dataAsset.deleted,
editCertificationPermission:
(permissions.EditAll || permissions.EditCertification) &&
getPrioritizedEditPermission(permissions, Operation.EditTier) &&
!dataAsset.deleted,
editCertificationPermission:
getPrioritizedEditPermission(
permissions,
Operation.EditCertification
) && !dataAsset.deleted,
}),
[permissions, dataAsset]
);
Expand Down
Loading