Skip to content

Feat# Implement self-approval prevention for workflow change requests#26289

Merged
karanh37 merged 2 commits intomainfrom
ram/self-approval-restriction
Mar 13, 2026
Merged

Feat# Implement self-approval prevention for workflow change requests#26289
karanh37 merged 2 commits intomainfrom
ram/self-approval-restriction

Conversation

@yan-3005
Copy link
Copy Markdown
Contributor

@yan-3005 yan-3005 commented Mar 6, 2026

Describe your changes:

Fixes https://github.com/open-metadata/openmetadata-collate/issues/2867

Type of change:

  • Bug fix
  • Improvement
  • New feature
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation

Checklist:

  • I have read the CONTRIBUTING document.
  • My PR title is Fixes <issue-number>: <short explanation>
  • I have commented on my code, particularly in hard-to-understand areas.
  • For JSON Schema changes: I updated the migration scripts or explained why it is not needed.

Summary by Gitar

  • Self-approval prevention logic:
    • Added check in SetApprovalAssigneesImpl to remove the updatedBy user from approval assignees list
    • Retrieves updatedBy from global workflow namespace and excludes from task assignees
  • Comprehensive integration test:
    • Added test_SelfApprovalPrevention test case with three users, classification entity, and approval workflow
    • Verifies that user making the change is excluded from approval task assignees while other reviewers remain

This will update automatically on new commits.

# Conflicts:
#	openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/WorkflowDefinitionResourceIT.java
@yan-3005 yan-3005 requested a review from a team as a code owner March 12, 2026 19:00
@gitar-bot
Copy link
Copy Markdown

gitar-bot bot commented Mar 12, 2026

🔍 CI failure analysis for ca1c007: 1 test failure in McpToolsValidationIT caused by assertion expecting indexed data that wasn't found, likely due to ElasticSearch version conflicts during concurrent bulk operations; 1 infrastructure timeout in PostgreSQL+OpenSearch integration tests from partial shard unavailability.

Overview

Analysis of 6 error templates across 8 CI logs reveals 2 distinct failure patterns: 1 PR-related test assertion failure in the integration test suite, and 1 infrastructure-level timeout from an external search service. The new workflow self-approval feature implementation appears to have introduced a test data indexing issue in ElasticSearch concurrent operations.

Failures

McpToolsValidationIT Test Assertion Failure (confidence: high)

  • Type: test
  • Affected jobs: integration-tests-mysql-elasticsearch
  • Related to PR: yes
  • Root cause: Test assertion at line 384 expects data count > 0 but receives 0. This coincides with ElasticSearch bulk operation version conflicts (seqNo/primaryTerm mismatches) during concurrent domain FQN updates. The new WorkflowDefinitionResourceIT.java and SetApprovalAssigneesImpl.java code likely triggers concurrent test data operations that race against test assertions, preventing proper indexing or retrieval.
  • Suggested fix: (1) Add explicit ElasticSearch refresh/flush calls before assertions in the new test to ensure indices are current; (2) Implement retry logic with exponential backoff for test data validation; (3) Review WorkflowDefinitionResourceIT test setup for proper cleanup and isolation between test cases; (4) Consider adding seqNo versioning checks or conflict handling in bulk operations during tests.

OpenSearch Partial Shard Unavailability Timeout (confidence: medium)

  • Type: infrastructure, flaky_test
  • Affected jobs: integration-tests-postgres-opensearch
  • Related to PR: unclear
  • Root cause: HTTP 503 Service Unavailable from OpenSearch with "Partial shards failure (2 shards unavailable)" and "search_phase_execution_exception". This occurs during ResponseException handling in the Jetty server request processing chain, indicating temporary unavailability of OpenSearch cluster shards.
  • Suggested fix: (1) Verify OpenSearch cluster health and shard allocation in test environment; (2) Increase shard recovery timeout if shards are temporarily unavailable; (3) Implement client-side retry logic with backoff for search operations; (4) Monitor OpenSearch resource constraints (memory, CPU) during test execution.

Summary

  • PR-related failures: 1 test assertion failure in McpToolsValidationIT (integration-tests-mysql-elasticsearch). Root cause is likely test data not being indexed to ElasticSearch due to concurrent bulk operation version conflicts introduced by the new workflow feature tests.
  • Infrastructure/flaky failures: 1 OpenSearch shard unavailability timeout in integration-tests-postgres-opensearch, which appears transient.
  • Recommended action: Priority fix the McpToolsValidationIT assertion by adding ElasticSearch index refresh/retry logic to the new WorkflowDefinitionResourceIT test setup. Verify test isolation and concurrent operation handling. Monitor OpenSearch infrastructure for shard allocation issues as secondary concern.
Code Review ✅ Approved 1 resolved / 1 findings

Implements self-approval prevention for workflow change requests to block users from approving their own changes. No issues found.

✅ 1 resolved
Edge Case: Self-approval prevention auto-approves when user is sole assignee

📄 openmetadata-service/src/main/java/org/openmetadata/service/governance/workflows/elements/nodes/userTask/impl/SetApprovalAssigneesImpl.java:120 📄 openmetadata-service/src/main/java/org/openmetadata/service/governance/workflows/elements/nodes/userTask/impl/SetApprovalAssigneesImpl.java:146 📄 openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/WorkflowDefinitionResourceIT.java:7516
When the updatedBy user is the only assignee (e.g., they are the sole reviewer or sole owner of the entity), removing them from the assignee list results in an empty list. This sets hasAssignees = false, which causes the hasAssigneesGateway to route to the auto-approve path — meaning the change is approved with zero human review.

This is the exact opposite of the intended security control: instead of preventing self-approval, it grants automatic approval when there's no other reviewer available. A malicious or careless sole-owner could modify their own entity and have it auto-approved with no oversight.

The fix should detect this case and either:

  1. Keep the original assignee (allow self-approval as a fallback when no alternative exists), or
  2. Block the workflow / raise an error requiring additional reviewers to be configured, or
  3. Escalate to a configurable fallback approver (e.g., admins).

Tip

Comment Gitar fix CI or enable auto-apply: gitar auto-apply:on

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change:

Auto-apply Compact
gitar auto-apply:on         
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

@github-actions
Copy link
Copy Markdown
Contributor

Jest test Coverage

UI tests summary

Lines Statements Branches Functions
Coverage: 65%
65.97% (57411/87022) 45.49% (30266/66523) 48.49% (9087/18738)

@sonarqubecloud
Copy link
Copy Markdown

@sonarqubecloud
Copy link
Copy Markdown

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 12, 2026

🔴 Playwright Results — 1 failure(s), 30 flaky

✅ 3306 passed · ❌ 1 failed · 🟡 30 flaky · ⏭️ 183 skipped

Shard Passed Failed Flaky Skipped
🟡 Shard 1 451 0 4 2
🟡 Shard 2 301 0 4 1
🟡 Shard 3 649 0 8 33
🔴 Shard 4 720 1 5 47
🟡 Shard 5 589 0 2 67
🟡 Shard 6 596 0 7 33

Genuine Failures (failed on all attempts)

Pages/Entity.spec.ts › Tag Add, Update and Remove for child entities (shard 4)
Error: �[2mexpect(�[22m�[31mlocator�[39m�[2m).�[22mtoBeHidden�[2m(�[22m�[2m)�[22m failed

Locator:  locator('.column-detail-panel').locator('.tags-list').getByTestId('tag-PersonalData.SpecialCategory')
Expected: hidden
Received: visible
Timeout:  5000ms

Call log:
�[2m  - Expect "toBeHidden" with timeout 5000ms�[22m
�[2m  - waiting for locator('.column-detail-panel').locator('.tags-list').getByTestId('tag-PersonalData.SpecialCategory')�[22m
�[2m    9 × locator resolved to <div class="tag-item" data-testid="tag-PersonalData.SpecialCategory">…</div>�[22m
�[2m      - unexpected value "visible"�[22m

🟡 30 flaky test(s) (passed on retry)
  • Features/CustomizeDetailPage.spec.ts › API Collection - customization should work (shard 1, 1 retry)
  • Features/TeamsDragAndDrop.spec.ts › Should drag and drop on Department team type (shard 1, 1 retry)
  • Pages/Customproperties-part1.spec.ts › sqlQuery shows scrollable CodeMirror container and no expand toggle (shard 1, 1 retry)
  • Pages/CustomThemeConfig.spec.ts › Update Hover and selected Color (shard 1, 1 retry)
  • Features/ColumnBulkOperations.spec.ts › should not reset stats to zero while search request is loading (shard 2, 1 retry)
  • Flow/ServiceForm.spec.ts › Verify SSL cert upload with long filename and UI overflow handling (shard 2, 1 retry)
  • Pages/ExploreTree.spec.ts › Verify Database and Database Schema available in explore tree (shard 2, 1 retry)
  • Pages/ExploreTree.spec.ts › Verify Database and Database schema after rename (shard 2, 1 retry)
  • Features/BulkImport.spec.ts › Database (shard 3, 1 retry)
  • Features/DataProductRename.spec.ts › should handle multiple consecutive renames and preserve assets (shard 3, 1 retry)
  • Features/DomainTierCertificationVoting.spec.ts › DataProduct - Certification assign, update, and remove (shard 3, 1 retry)
  • Features/Glossary/GlossaryAssets.spec.ts › should remove glossary term tag from entity page (shard 3, 1 retry)
  • Features/Glossary/GlossaryHierarchy.spec.ts › should move nested term to root level of same glossary (shard 3, 1 retry)
  • Features/Glossary/GlossaryHierarchy.spec.ts › should move term with children to different glossary (shard 3, 1 retry)
  • Features/Permissions/GlossaryPermissions.spec.ts › Team-based permissions work correctly (shard 3, 1 retry)
  • Features/Permissions/ServiceEntityPermissions.spec.ts › SearchIndex Service allow common operations permissions (shard 3, 1 retry)
  • Flow/NotificationAlerts.spec.ts › Task source alert (shard 4, 1 retry)
  • Pages/Customproperties-part2.spec.ts › entityReferenceList shows item count, scrollable list, no expand toggle (shard 4, 1 retry)
  • Pages/Domains.spec.ts › Rename domain with tags and glossary terms preserves associations (shard 4, 1 retry)
  • Pages/Domains.spec.ts › Multiple consecutive domain renames preserve all associations (shard 4, 1 retry)
  • Pages/Entity.spec.ts › Glossary Term Add, Update and Remove (shard 4, 1 retry)
  • Pages/EntityDataSteward.spec.ts › User as Owner Add, Update and Remove (shard 5, 1 retry)
  • Pages/ExplorePageRightPanel.spec.ts › Should verify deleted user not visible in owner selection for pipeline (shard 5, 1 retry)
  • Pages/Glossary.spec.ts › Async Delete - multiple deletes all succeed (shard 6, 1 retry)
  • Pages/Glossary.spec.ts › Create glossary with all optional fields (tags, owners, reviewers, domain) (shard 6, 1 retry)
  • Pages/HyperlinkCustomProperty.spec.ts › should show No Data placeholder when hyperlink has no value (shard 6, 1 retry)
  • Pages/Tag.spec.ts › Add and Remove Assets and Check Restricted Entity (shard 6, 1 retry)
  • Pages/Teams.spec.ts › Add and Remove User for Team (shard 6, 1 retry)
  • Pages/Teams.spec.ts › Should not have edit access on team page with no data available (shard 6, 1 retry)
  • Pages/UserDetails.spec.ts › Admin user can get all the roles hierarchy and edit roles (shard 6, 1 retry)

📦 Download artifacts

How to debug locally
# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip    # view trace

@karanh37 karanh37 merged commit e4aa012 into main Mar 13, 2026
49 of 54 checks passed
@karanh37 karanh37 deleted the ram/self-approval-restriction branch March 13, 2026 14:04
@github-actions
Copy link
Copy Markdown
Contributor

Failed to cherry-pick changes to the 1.12.2 branch.
Please cherry-pick the changes manually.
You can find more details here.

yan-3005 added a commit that referenced this pull request Mar 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend safe to test Add this label to run secure Github workflows on PRs To release Will cherry-pick this PR into the release branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants