Reject party/role combinations the party does not hold for TimesheetRole and RequirementRole

applications/accounting/servicedef/secas.xml

@@ -47,6 +47,19 @@ under the License.
         <action service="updatePaymentMethodAddress" mode="sync"/>
     </eca>
 
+    <!-- reject an improbable party/role combination (party does not already hold roleTypeId) before it
+         reaches ensurePartyRole below, which would otherwise silently create a spurious PartyRole - OFBIZ-12370/12372/12373 -->
+    <eca service="createInvoiceRole" event="in-validate">
+        <condition field-name="roleTypeId" operator="is-not-empty"/>
+        <condition field-name="partyId" operator="is-not-empty"/>
+        <action service="checkPartyRoleExists" mode="sync"/>
+    </eca>
+
+    <eca service="createBillingAccountRole" event="in-validate">
+        <condition field-name="roleTypeId" operator="is-not-empty"/>
+        <condition field-name="partyId" operator="is-not-empty"/>
+        <action service="checkPartyRoleExists" mode="sync"/>
+    </eca>
     <eca service="createBillingAccountRole" event="invoke">
         <condition field-name="roleTypeId" operator="is-not-empty"/>
         <condition field-name="partyId" operator="is-not-empty"/>
@@ -105,11 +118,25 @@ under the License.
     </eca>
 
     <!-- budget role ecas -->
+    <!-- reject an improbable party/role combination before it reaches ensurePartyRole below,
+         which would otherwise silently create a spurious PartyRole - OFBIZ-12371 -->
+    <eca service="createBudgetRole" event="in-validate">
+        <condition field-name="roleTypeId" operator="is-not-empty"/>
+        <condition field-name="partyId" operator="is-not-empty"/>
+        <action service="checkPartyRoleExists" mode="sync"/>
+    </eca>
     <eca service="createBudgetRole" event="invoke">
         <action service="ensurePartyRole" mode="sync" run-as-user="system"/>
     </eca>
 
     <!-- financial account role ecas -->
+    <!-- reject an improbable party/role combination before it reaches ensurePartyRole below,
+         which would otherwise silently create a spurious PartyRole - OFBIZ-12373 -->
+    <eca service="createFinAccountRole" event="in-validate">
+        <condition field-name="roleTypeId" operator="is-not-empty"/>
+        <condition field-name="partyId" operator="is-not-empty"/>
+        <action service="checkPartyRoleExists" mode="sync"/>
+    </eca>
     <eca service="createFinAccountRole" event="invoke">
         <action service="ensurePartyRole" mode="sync" run-as-user="system"/>
     </eca>

applications/accounting/src/test/groovy/org/apache/ofbiz/accounting/accounting/AutoAcctgBillingAccountTests.groovy

@@ -0,0 +1,100 @@
+/*******************************************************************************
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you 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.
+ *******************************************************************************/
+package org.apache.ofbiz.accounting.accounting
+
+import org.apache.ofbiz.entity.GenericValue
+import org.apache.ofbiz.service.ServiceUtil
+import org.apache.ofbiz.testtools.JunitJupiterTest
+import org.apache.ofbiz.testtools.JupiterTestHelper
+import org.junit.jupiter.api.Order
+import org.junit.jupiter.api.Test
+
+@JunitJupiterTest
+class AutoAcctgBillingAccountTests implements JupiterTestHelper {
+
+    private String billingAccountId
+
+    @Test
+    @Order(1)
+    void testCreateBillingAccount() {
+        Map serviceCtx = [
+                accountLimit: 1000,
+                description: 'AutoAcctgBillingAccountTests billing account',
+                userLogin: userLogin
+        ]
+        Map serviceResult = dispatcher.runSync('createBillingAccount', serviceCtx)
+        assert ServiceUtil.isSuccess(serviceResult)
+        billingAccountId = serviceResult.billingAccountId
+        assert billingAccountId
+
+        GenericValue billingAccount = from('BillingAccount').where('billingAccountId', billingAccountId).queryOne()
+        assert billingAccount
+    }
+
+    // DEMO_COMPANY already holds the INTERNAL_ORGANIZATIO role (see AccountingTestsData.xml), so this
+    // combination is legitimate and must still succeed after OFBIZ-12372's validation is in place.
+    @Test
+    @Order(2)
+    void testCreateBillingAccountRole() {
+        String partyId = testParams.partyId ?: 'DEMO_COMPANY'
+        String roleTypeId = 'INTERNAL_ORGANIZATIO'
+        Map serviceCtx = [
+                billingAccountId: billingAccountId,
+                partyId: partyId,
+                roleTypeId: roleTypeId,
+                userLogin: userLogin
+        ]
+        Map serviceResult = dispatcher.runSync('createBillingAccountRole', serviceCtx)
+        assert ServiceUtil.isSuccess(serviceResult)
+
+        GenericValue billingAccountRole = from('BillingAccountRole')
+                .where('billingAccountId', billingAccountId, 'partyId', partyId, 'roleTypeId', roleTypeId)
+                .queryOne()
+        assert billingAccountRole
+    }
+
+    // OFBIZ-12372: DEMO_COMPANY does not hold the CARRIER role (only INTERNAL_ORGANIZATIO/SUPPLIER,
+    // see AccountingTestsData.xml), so this combination must be rejected instead of the
+    // createBillingAccountRole -> ensurePartyRole eca silently fabricating a PartyRole record for a
+    // role the party was never given.
+    @Test
+    @Order(3)
+    void testCreateBillingAccountRoleRejectsPartyWithoutRole() {
+        String partyId = testParams.partyId ?: 'DEMO_COMPANY'
+        Map serviceCtx = [
+                billingAccountId: billingAccountId,
+                partyId: partyId,
+                roleTypeId: 'CARRIER',
+                userLogin: userLogin
+        ]
+        Map serviceResult = dispatcher.runSync('createBillingAccountRole', serviceCtx)
+        assert ServiceUtil.isError(serviceResult)
+
+        GenericValue billingAccountRole = from('BillingAccountRole')
+                .where('billingAccountId', billingAccountId, 'partyId', partyId, 'roleTypeId', 'CARRIER')
+                .queryOne()
+        assert !billingAccountRole
+
+        GenericValue spuriousPartyRole = from('PartyRole')
+                .where('partyId', partyId, 'roleTypeId', 'CARRIER')
+                .queryOne()
+        assert !spuriousPartyRole
+    }
+
+}

applications/accounting/src/test/groovy/org/apache/ofbiz/accounting/accounting/AutoAcctgBudgetTests.groovy

@@ -62,4 +62,56 @@ class AutoAcctgBudgetTests implements JupiterTestHelper {
         assert budgetStatuses[0].statusId == statusId
     }
 
+    // DEMO_COMPANY already holds the INTERNAL_ORGANIZATIO role (see AccountingTestsData.xml), so this
+    // combination is legitimate and must still succeed after OFBIZ-12371's validation is in place.
+    @Test
+    @Order(3)
+    void testCreateBudgetRole() {
+        String budgetId = testParams.budgetId ?: '9999'
+        String partyId = testParams.partyId ?: 'DEMO_COMPANY'
+        String roleTypeId = 'INTERNAL_ORGANIZATIO'
+        Map serviceCtx = [
+                budgetId: budgetId,
+                partyId: partyId,
+                roleTypeId: roleTypeId,
+                userLogin: userLogin
+        ]
+        Map serviceResult = dispatcher.runSync('createBudgetRole', serviceCtx)
+        assert ServiceUtil.isSuccess(serviceResult)
+
+        GenericValue budgetRole = from('BudgetRole')
+                .where('budgetId', budgetId, 'partyId', partyId, 'roleTypeId', roleTypeId)
+                .queryOne()
+        assert budgetRole
+    }
+
+    // OFBIZ-12371: DEMO_COMPANY does not hold the CARRIER role (only INTERNAL_ORGANIZATIO/SUPPLIER,
+    // see AccountingTestsData.xml), so this combination must be rejected instead of the
+    // createBudgetRole -> ensurePartyRole eca silently fabricating a PartyRole record for a role
+    // the party was never given.
+    @Test
+    @Order(4)
+    void testCreateBudgetRoleRejectsPartyWithoutRole() {
+        String budgetId = testParams.budgetId ?: '9999'
+        String partyId = testParams.partyId ?: 'DEMO_COMPANY'
+        Map serviceCtx = [
+                budgetId: budgetId,
+                partyId: partyId,
+                roleTypeId: 'CARRIER',
+                userLogin: userLogin
+        ]
+        Map serviceResult = dispatcher.runSync('createBudgetRole', serviceCtx)
+        assert ServiceUtil.isError(serviceResult)
+
+        GenericValue budgetRole = from('BudgetRole')
+                .where('budgetId', budgetId, 'partyId', partyId, 'roleTypeId', 'CARRIER')
+                .queryOne()
+        assert !budgetRole
+
+        GenericValue spuriousPartyRole = from('PartyRole')
+                .where('partyId', partyId, 'roleTypeId', 'CARRIER')
+                .queryOne()
+        assert !spuriousPartyRole
+    }
+
 }

applications/accounting/src/test/groovy/org/apache/ofbiz/accounting/accounting/AutoAcctgFinAccountTests.groovy

@@ -114,8 +114,38 @@ class AutoAcctgFinAccountTests implements JupiterTestHelper {
         assert finAccountRole
     }
 
+    // OFBIZ-12373: DEMO_COMPANY does not hold the CARRIER role (only INTERNAL_ORGANIZATIO/SUPPLIER,
+    // see AccountingTestsData.xml), so this combination must be rejected instead of the createFinAccountRole
+    // -> ensurePartyRole eca silently fabricating a PartyRole record for a role the party was never given.
     @Test
     @Order(5)
+    void testCreateFinAccountRoleRejectsPartyWithoutRole() {
+        String finAccountId = testParams.finAccountId ?: '1003'
+        String partyId = testParams.partyId ?: 'DEMO_COMPANY'
+        Map serviceCtx = [
+                finAccountId: finAccountId,
+                partyId: partyId,
+                roleTypeId: 'CARRIER',
+                fromDate: UtilDateTime.nowTimestamp(),
+                currencyUomId: testParams.currencyUomId ?: 'USD',
+                userLogin: userLogin
+        ]
+        Map serviceResult = dispatcher.runSync('createFinAccountRole', serviceCtx)
+        assert ServiceUtil.isError(serviceResult)
+
+        GenericValue finAccountRole = from('FinAccountRole')
+                .where('finAccountId', finAccountId, 'partyId', partyId, 'roleTypeId', 'CARRIER')
+                .queryFirst()
+        assert !finAccountRole
+
+        GenericValue spuriousPartyRole = from('PartyRole')
+                .where('partyId', partyId, 'roleTypeId', 'CARRIER')
+                .queryOne()
+        assert !spuriousPartyRole
+    }
+
+    @Test
+    @Order(6)
     void testUpdateFinAccountRole() {
         String finAccountId = testParams.finAccountId ?: '1004'
         String partyId = testParams.partyId ?: 'DEMO_COMPANY'
@@ -139,7 +169,7 @@ class AutoAcctgFinAccountTests implements JupiterTestHelper {
     }
 
     @Test
-    @Order(6)
+    @Order(7)
     void testDeleteFinAccountRole() {
         String finAccountId = testParams.finAccountId ?: '1004'
         String partyId = testParams.partyId ?: 'DEMO_COMPANY'
@@ -161,7 +191,7 @@ class AutoAcctgFinAccountTests implements JupiterTestHelper {
     }
 
     @Test
-    @Order(7)
+    @Order(8)
     void testCreateFinAccountTrans() {
         String finAccountId = testParams.finAccountId ?: '1003'
         String finAccountTransTypeId = testParams.finAccountTransTypeId ?: 'ADJUSTMENT'
@@ -180,7 +210,7 @@ class AutoAcctgFinAccountTests implements JupiterTestHelper {
     }
 
     @Test
-    @Order(8)
+    @Order(9)
     void testCreateFinAccountStatus() {
         String finAccountId = testParams.finAccountId ?: '1003'
         String statusId = testParams.statusId ?: 'FNACT_ACTIVE'
@@ -200,7 +230,7 @@ class AutoAcctgFinAccountTests implements JupiterTestHelper {
     }
 
     @Test
-    @Order(9)
+    @Order(10)
     void testCreateFinAccountAuth() {
         String finAccountId = testParams.finAccountId ?: '1004'
         String currencyUomId = testParams.currencyUomId ?: 'USD'
@@ -218,7 +248,7 @@ class AutoAcctgFinAccountTests implements JupiterTestHelper {
     }
 
     @Test
-    @Order(10)
+    @Order(11)
     void testSetFinAccountTransStatus() {
         String finAccountTransId = testParams.finAccountTransId ?: '1010'
         String statusId = testParams.statusId ?: 'FINACT_TRNS_APPROVED'

applications/accounting/src/test/groovy/org/apache/ofbiz/accounting/accounting/AutoAcctgInvoiceTests.groovy

@@ -217,8 +217,31 @@ class AutoAcctgInvoiceTests implements JupiterTestHelper {
         assert invoiceRole
     }
 
+    // OFBIZ-12370: DEMO_COMPANY does not hold the CARRIER role (only INTERNAL_ORGANIZATIO/SUPPLIER,
+    // see AccountingTestsData.xml), so this combination must be rejected instead of hitting the
+    // InvoiceRole/PartyRole foreign key constraint with a raw SQL error.
     @Test
     @Order(11)
+    void testCreateInvoiceRoleRejectsPartyWithoutRole() {
+        Map serviceCtx = [
+                invoiceId: testParams.invoiceId ?: '1006',
+                partyId: testParams.partyId ?: 'DEMO_COMPANY',
+                roleTypeId: 'CARRIER',
+                userLogin: userLogin
+        ]
+        Map serviceResult = dispatcher.runSync('createInvoiceRole', serviceCtx)
+        assert ServiceUtil.isError(serviceResult)
+
+        GenericValue invoiceRole = from('InvoiceRole')
+                .where('invoiceId', serviceCtx.invoiceId,
+                        'partyId', serviceCtx.partyId,
+                        'roleTypeId', 'CARRIER')
+                .queryOne()
+        assert !invoiceRole
+    }
+
+    @Test
+    @Order(12)
     void testCreateInvoiceTerm() {
         Map serviceCtx = [
                 invoiceId: testParams.invoiceId ?: '1006',
@@ -239,7 +262,7 @@ class AutoAcctgInvoiceTests implements JupiterTestHelper {
     }
 
     @Test
-    @Order(12)
+    @Order(13)
     void testCancelInvoice() {
         Map serviceCtx = [
                 invoiceId: testParams.invoiceId ?: '1007',

applications/accounting/testdef/accountingtests.xml

@@ -42,6 +42,9 @@
     <test-case case-name="auto-accounting-agreement-tests">
         <jupiter-test-suite class-name="org.apache.ofbiz.accounting.accounting.AutoAcctgAgreementTests"/>
     </test-case>
+    <test-case case-name="auto-accounting-billingaccount-tests">
+        <jupiter-test-suite class-name="org.apache.ofbiz.accounting.accounting.AutoAcctgBillingAccountTests"/>
+    </test-case>
     <test-case case-name="auto-accounting-budget-tests">
         <jupiter-test-suite class-name="org.apache.ofbiz.accounting.accounting.AutoAcctgBudgetTests"/>
     </test-case>

applications/order/servicedef/secas.xml

@@ -515,6 +515,13 @@ under the License.
         <action service="updateCustRequestLastModifiedDate" mode="sync"/>
     </eca>
     <!-- RequirementRole eca -->
+    <!-- reject an improbable party/role combination (party does not already hold roleTypeId) before it
+         reaches ensurePartyRole below, which would otherwise silently create a spurious PartyRole -->
+    <eca service="createRequirementRole" event="in-validate">
+        <condition field-name="roleTypeId" operator="is-not-empty"/>
+        <condition field-name="partyId" operator="is-not-empty"/>
+        <action service="checkPartyRoleExists" mode="sync"/>
+    </eca>
     <eca service="createRequirementRole" event="invoke">
         <action service="ensurePartyRole" mode="sync"/>
     </eca>

applications/order/src/test/groovy/org/apache/ofbiz/order/order/test/OrderRequirementTests.groovy

@@ -132,4 +132,37 @@ class OrderRequirementTests implements JupiterTestHelper {
         assert ServiceUtil.isSuccess(serviceResult)
     }
 
+    // createRequirementRole's in-validate eca rejects a party/role combination the party does not
+    // already hold, before it reaches ensurePartyRole (which would otherwise silently create a
+    // spurious PartyRole). DemoCustomer's seeded PartyRole set (OrderDemoData.xml) is exactly
+    // {BILL_TO_CUSTOMER, CONTACT, CUSTOMER, END_USER_CUSTOMER, PLACING_CUSTOMER, SHIP_TO_CUSTOMER} --
+    // CARRIER is provably not among them.
+    @Test
+    @Order(8)
+    void testCreateRequirementRole_rejectsRoleThePartyDoesNotHold() {
+        String requirementId = testParams.requirementId ?: '1000'
+        Map serviceCtx = [
+            requirementId: requirementId,
+            partyId: 'DemoCustomer',
+            roleTypeId: 'CARRIER',
+            userLogin: userLogin
+        ]
+        Map serviceResult = dispatcher.runSync('createRequirementRole', serviceCtx)
+        assert ServiceUtil.isError(serviceResult)
+    }
+
+    @Test
+    @Order(9)
+    void testCreateRequirementRole_allowsRoleThePartyAlreadyHolds() {
+        String requirementId = testParams.requirementId ?: '1000'
+        Map serviceCtx = [
+            requirementId: requirementId,
+            partyId: 'DemoCustomer',
+            roleTypeId: 'CUSTOMER',
+            userLogin: userLogin
+        ]
+        Map serviceResult = dispatcher.runSync('createRequirementRole', serviceCtx)
+        assert ServiceUtil.isSuccess(serviceResult)
+    }
+
 }