Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@
import org.openmetadata.sdk.client.OpenMetadataClient;
import org.openmetadata.sdk.models.ListParams;
import org.openmetadata.sdk.models.ListResponse;
import org.openmetadata.service.Entity;
import org.openmetadata.service.resources.policies.PolicyResource;

/**
Expand Down Expand Up @@ -478,6 +479,61 @@ void test_createPolicyWithMultipleOperations(TestNamespace ns) {
assertTrue(policy.getRules().get(0).getOperations().size() >= 3);
}

/**
* Reproduces the server behaviour behind “ViewBasic disappears after save” for Classification
* policies (GitHub #27591) when a rule includes both {@code ViewAll} and {@code ViewBasic}:
* {@link org.openmetadata.service.jdbi3.PolicyRepository#filterRedundantOperations} drops {@code
* ViewBasic} as redundant. This test should pass before and after the UI fix; it locks the
* contract the UI was not reflecting immediately.
*/
@Test
void test_createPolicyClassificationViewBasicOmittedWhenRedundantWithViewAll(
TestNamespace ns) {
Rule rule =
new Rule()
.withName("classificationViewAllAndBasic")
.withResources(List.of(Entity.CLASSIFICATION))
.withOperations(
List.of(MetadataOperation.VIEW_ALL, MetadataOperation.VIEW_BASIC))
.withEffect(Effect.ALLOW);
CreatePolicy create =
new CreatePolicy()
.withName(ns.prefix("classificationRedundantViewOpsPolicy"))
.withRules(List.of(rule))
.withDescription("Repro: VIEW_BASIC normalized away when VIEW_ALL is present");
Policy policy = createEntity(create);
List<MetadataOperation> operations = policy.getRules().get(0).getOperations();
assertTrue(operations.contains(MetadataOperation.VIEW_ALL));
assertFalse(
operations.contains(MetadataOperation.VIEW_BASIC),
"VIEW_BASIC is removed as redundant when VIEW_ALL is present (PolicyRepository policy)");
}

/**
* Regression: explicit {@code ViewBasic} alone on the classification resource must round-trip
* (GitHub #27591). Fails if the API incorrectly drops a standalone VIEW_BASIC rule.
*/
@Test
void test_createPolicyClassificationViewBasicPersists(TestNamespace ns) {
Rule rule =
new Rule()
.withName("classificationViewBasicOnly")
.withResources(List.of(Entity.CLASSIFICATION))
.withOperations(List.of(MetadataOperation.VIEW_BASIC))
.withEffect(Effect.ALLOW);
CreatePolicy create =
new CreatePolicy()
.withName(ns.prefix("classificationViewBasicPolicy"))
.withRules(List.of(rule))
.withDescription("VIEW_BASIC only on classification resource");
Policy policy = createEntity(create);
assertNotNull(policy.getRules());
assertTrue(
policy.getRules().get(0).getOperations().contains(MetadataOperation.VIEW_BASIC),
"VIEW_BASIC must remain when it is the only view operation for classification");
assertEquals(1, policy.getRules().get(0).getOperations().size());
}

@Test
void test_createPolicyWithOwner(TestNamespace ns) {
OpenMetadataClient client = SdkClients.adminClient();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import brandClassBase from '../../../utils/BrandData/BrandClassBase';
import { getField } from '../../../utils/formUtils';
import { translateWithNestedKeys } from '../../../utils/i18next/LocalUtil';
import { getPath, getPolicyWithFqnPath } from '../../../utils/RouterUtils';
import { filterRedundantPolicyOperations } from '../../../utils/PolicyRuleUtils';
import { showErrorToast } from '../../../utils/ToastUtils';
import RuleForm from '../RuleForm/RuleForm';

Expand All @@ -56,17 +57,22 @@ const AddPolicyPage = () => {
effect: Effect.Allow,
});
const [isSaveLoading, setIsSaveLoading] = useState<boolean>(false);
const [form] = Form.useForm();

const handleCancel = () => {
navigate(policiesPath);
};

const handleSubmit = async () => {
const { condition, ...rest } = { ...ruleData, name: trim(ruleData.name) };
const rulePayload = {
...rest,
operations: filterRedundantPolicyOperations(rest.operations),
};
const data: CreatePolicy = {
name: trim(name),
description,
rules: [condition ? { ...rest, condition } : rest],
rules: [condition ? { ...rulePayload, condition } : rulePayload],
};

setIsSaveLoading(true);
Expand Down Expand Up @@ -140,6 +146,7 @@ const AddPolicyPage = () => {
</Typography.Paragraph>
<Form
data-testid="policy-form"
form={form}
id="policy-form"
initialValues={{
ruleEffect: ruleData.effect,
Expand All @@ -166,7 +173,11 @@ const AddPolicyPage = () => {
entity: t('label.rule'),
})}
</Divider>
<RuleForm ruleData={ruleData} setRuleData={setRuleData} />
<RuleForm
form={form}
ruleData={ruleData}
setRuleData={setRuleData}
/>

<Space align="center" className="w-full justify-end">
<Button
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import {
getPolicyWithFqnPath,
getSettingPath,
} from '../../../utils/RouterUtils';
import { filterRedundantPolicyOperations } from '../../../utils/PolicyRuleUtils';
import { showErrorToast } from '../../../utils/ToastUtils';
import RuleForm from '../RuleForm/RuleForm';

Expand All @@ -45,6 +46,7 @@ const AddRulePage = () => {
const { fqn } = useFqn();
const [isLoading, setLoading] = useState<boolean>(false);
const [policy, setPolicy] = useState<Policy>({} as Policy);
const [form] = Form.useForm();
const [ruleData, setRuleData] = useState<Rule>({
name: '',
description: '',
Expand Down Expand Up @@ -100,9 +102,16 @@ const AddRulePage = () => {

const handleSubmit = async () => {
const { condition, ...rest } = { ...ruleData, name: trim(ruleData.name) };
const ruleToAdd = {
...rest,
operations: filterRedundantPolicyOperations(rest.operations),
};
const patch = compare(policy, {
...policy,
rules: [...policy.rules, condition ? { ...rest, condition } : rest],
rules: [
...policy.rules,
condition ? { ...ruleToAdd, condition } : ruleToAdd,
],
});
try {
const data = await patchPolicy(patch, policy.id);
Expand Down Expand Up @@ -154,13 +163,14 @@ const AddRulePage = () => {
</Typography.Paragraph>
<Form
data-testid="rule-form"
form={form}
id="rule-form"
initialValues={{
ruleEffect: ruleData.effect,
}}
layout="vertical"
onFinish={handleSubmit}>
<RuleForm ruleData={ruleData} setRuleData={setRuleData} />
<RuleForm form={form} ruleData={ruleData} setRuleData={setRuleData} />
<Space align="center" className="w-full justify-end">
<Button data-testid="cancel-btn" type="link" onClick={handleBack}>
{t('label.cancel')}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import {
getPolicyWithFqnPath,
getSettingPath,
} from '../../../utils/RouterUtils';
import { filterRedundantPolicyOperations } from '../../../utils/PolicyRuleUtils';
import { showErrorToast } from '../../../utils/ToastUtils';
import RuleForm from '../RuleForm/RuleForm';

Expand All @@ -55,6 +56,7 @@ const EditRulePage = () => {
const [isLoading, setIsLoading] = useState<boolean>(false);
const [policy, setPolicy] = useState<Policy>({} as Policy);
const [ruleData, setRuleData] = useState<Rule>(InitialData);
const [form] = Form.useForm();

const selectedRuleRef = useRef<Rule | undefined>(InitialData);

Expand Down Expand Up @@ -91,8 +93,16 @@ const EditRulePage = () => {
if (data) {
setPolicy(data);
const selectedRule = data.rules.find((rule) => rule.name === ruleName);
selectedRuleRef.current = selectedRule;
setRuleData(selectedRule ?? InitialData);
const nextRule = selectedRule
? {
...selectedRule,
operations: filterRedundantPolicyOperations(
selectedRule.operations ?? []
),
}
: undefined;
selectedRuleRef.current = nextRule;
setRuleData(nextRule ?? InitialData);
} else {
setPolicy({} as Policy);
}
Expand All @@ -115,8 +125,14 @@ const EditRulePage = () => {
...ruleData,
name: trim(ruleData.name),
};
const withNormalizedOps = {
...rest,
operations: filterRedundantPolicyOperations(rest.operations),
};

return condition ? { ...rest, condition } : rest;
return condition
? { ...withNormalizedOps, condition }
: withNormalizedOps;
} else {
return rule;
}
Expand Down Expand Up @@ -174,6 +190,7 @@ const EditRulePage = () => {
</Typography.Paragraph>
<Form
data-testid="rule-form"
form={form}
id="rule-form"
initialValues={{
ruleEffect: ruleData.effect,
Expand All @@ -182,10 +199,12 @@ const EditRulePage = () => {
operations: ruleData.operations,
condition: ruleData.condition,
}}
key={`${fqn}-${ruleName}`}
layout="vertical"
onFinish={handleSubmit}>
<RuleForm
description={selectedRuleRef.current?.description}
form={form}
ruleData={ruleData}
setRuleData={setRuleData}
/>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
*/

import { AutoComplete, Form, Input, Select, TreeSelect } from 'antd';
import type { FormInstance } from 'antd';
import { BaseOptionType } from 'antd/lib/select';
import { AxiosError } from 'axios';
import { capitalize, startCase, uniq, uniqBy } from 'lodash';
Expand All @@ -35,6 +36,7 @@ import {
} from '../../../rest/rolesAPIV1';
import { getField } from '../../../utils/formUtils';
import { ALL_TYPE_RESOURCE_LIST } from '../../../utils/PermissionsUtils';
import { filterRedundantPolicyOperations } from '../../../utils/PolicyRuleUtils';
import { getErrorText } from '../../../utils/StringsUtils';
import { showErrorToast } from '../../../utils/ToastUtils';

Expand All @@ -44,12 +46,14 @@ export interface RuleFormProps {
ruleData: Rule;
setRuleData: (value: React.SetStateAction<Rule>) => void;
description?: string;
form?: FormInstance;
}

const RuleForm: FC<RuleFormProps> = ({
ruleData,
setRuleData,
description,
form: formFromParent,
}) => {
const { t } = useTranslation();
const [policyResources, setPolicyResources] = useState<ResourceDescriptor[]>(
Expand Down Expand Up @@ -298,10 +302,12 @@ const RuleForm: FC<RuleFormProps> = ({
showCheckedStrategy={TreeSelect.SHOW_PARENT}
treeData={operationOptions}
onChange={(values: Operation[]) => {
const next = filterRedundantPolicyOperations(values);
setRuleData((prev: Rule) => ({
...prev,
operations: values,
operations: next,
}));
formFromParent?.setFieldValue('operations', next);
}}
/>
</Form.Item>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
/*
* Copyright 2026 Collate.
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
* http://www.apache.org/licenses/LICENSE-2.0
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

import { Operation } from '../generated/api/policies/createPolicy';
import { filterRedundantPolicyOperations } from './PolicyRuleUtils';

describe('PolicyRuleUtils', () => {
it('keeps ViewBasic when it is the only view operation', () => {
const result = filterRedundantPolicyOperations([Operation.ViewBasic]);
expect(result).toEqual([Operation.ViewBasic]);
});

it('removes other view operations when ViewAll is present', () => {
const result = filterRedundantPolicyOperations([
Operation.Create,
Operation.ViewAll,
Operation.ViewBasic,
Operation.ViewUsage,
]);
expect(result).toEqual([Operation.Create, Operation.ViewAll]);
});
Comment on lines +17 to +31
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Quality: Unit tests missing coverage for EditAll filtering path

The PolicyRuleUtils.test.ts file only tests ViewAll/ViewBasic scenarios but has no test for the EditAll branch of filterRedundantPolicyOperations. Since the function has two independent filter paths (ViewAll and EditAll), both should be exercised to prevent regressions.

Suggested fix:

Add tests for EditAll filtering, e.g.:

it('removes other edit operations when EditAll is present', () => {
  const result = filterRedundantPolicyOperations([
    Operation.ViewBasic,
    Operation.EditAll,
    Operation.EditDescription,
    Operation.EditTags,
  ]);
  expect(result).toEqual([Operation.ViewBasic, Operation.EditAll]);
});

it('handles both ViewAll and EditAll together', () => {
  const result = filterRedundantPolicyOperations([
    Operation.ViewAll,
    Operation.ViewBasic,
    Operation.EditAll,
    Operation.EditTags,
  ]);
  expect(result).toEqual([Operation.ViewAll, Operation.EditAll]);
});

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


it('does not remove view operations when ViewAll is absent', () => {
const result = filterRedundantPolicyOperations([
Operation.Create,
Operation.ViewBasic,
Operation.ViewUsage,
]);
expect(result).toEqual([
Operation.Create,
Operation.ViewBasic,
Operation.ViewUsage,
]);
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
/*
* Copyright 2026 Collate.
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
* http://www.apache.org/licenses/LICENSE-2.0
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

import { Operation } from '../generated/api/policies/createPolicy';

const isViewOperation = (operation: Operation) => operation.startsWith('View');

const isEditOperation = (operation: Operation) => operation.startsWith('Edit');

/**
* Matches server-side PolicyRepository.filterRedundantOperations so the policy
* rule editor stays consistent with stored policy rules.
*/
export const filterRedundantPolicyOperations = (
operations: Operation[]
): Operation[] => {
let result = [...operations];
if (result.includes(Operation.ViewAll)) {
result = result.filter(
(o) => o === Operation.ViewAll || !isViewOperation(o)
);
}
if (result.includes(Operation.EditAll)) {
result = result.filter(
(o) => o === Operation.EditAll || !isEditOperation(o)
);
}
return result;
};
Loading