Reject party/role combinations the party does not hold for TimesheetRole and RequirementRole - #2006
Open
karthikchundi-commits wants to merge 3 commits into
Conversation
…OFBIZ-12372, OFBIZ-12373) InvoiceRole, BillingAccountRole, and FinAccountRole all let a user submit any partyId + roleTypeId combination with no validation that the party actually holds that role. For InvoiceRole this hits the InvoiceRole/ PartyRole foreign key constraint and surfaces a raw SQL error. For BillingAccountRole and FinAccountRole, an unconditional ensurePartyRole eca instead silently fabricates a PartyRole record for the submitted roleTypeId, regardless of whether it makes sense for that party. Adds a small, reusable checkPartyRoleExists service (party component, mirroring the existing ensureNaPartyRole helper) that looks up PartyRole by (partyId, roleTypeId) and, if missing, fails with the same PartyRoleAssociationRequired message already used for this exact class of bug in createWorkEffortAndPartyAssign. Wires it as an in-validate eca on createInvoiceRole, createBillingAccountRole, and createFinAccountRole, so invalid combinations are rejected before the existing invoke-event ensurePartyRole ecas ever run. Adds regression tests: a negative case per ticket proving the party/role combination is now rejected (and, for BillingAccountRole/ FinAccountRole, that no PartyRole is fabricated), plus a new AutoAcctgBillingAccountTests suite since BillingAccountRole had no existing test coverage. Signed-off-by: Karth <karthik.chundi@gmail.com>
… (OFBIZ-12371) Same bug shape already fixed for InvoiceRole/BillingAccountRole/ FinAccountRole: createBudgetRole's unconditional ensurePartyRole invoke eca silently fabricates a PartyRole record for whatever roleTypeId is submitted, with no validation that the party actually holds (or should hold) that role. Wires the existing checkPartyRoleExists service (added for the other three role types) as an in-validate eca on createBudgetRole, so invalid combinations are rejected before the invoke-event ensurePartyRole eca ever runs. Adds a positive-case and negative-case regression test to AutoAcctgBudgetTests, following the same pattern used for the other three role types. Depends on the checkPartyRoleExists service already added for OFBIZ-12370/12372/12373 (open as apache#1727) - this change is a no-op without it. Signed-off-by: Karth <karthik.chundi@gmail.com>
…ole and RequirementRole createTimesheetRole and createRequirementRole have the identical unvalidated-PartyRole gap already fixed for TaxAuthority (apache#1692), Invoice/BillingAccount/FinAccount roles (apache#1727), and BudgetRole (apache#1736): a free-choice RoleType dropdown feeding straight into an unconditional ensurePartyRole invoke-eca, so any party/role combination is silently accepted and a spurious PartyRole is fabricated even when the party doesn't legitimately hold that role. Both AddTimesheetRole (TimesheetForms.xml) and AddRequirementRole (RequirementForms.xml) submit directly to their respective create service with no other caller anywhere in the codebase (verified via a search across XML/Groovy/Java, not just the immediate eca/form pair), so the fix mirrors the existing pattern exactly: an in-validate eca calling the already-built checkPartyRoleExists service before the invoke-eca that runs ensurePartyRole. Two other services flagged during this pass - createContentRole and createQuoteRole - have the same free-choice dropdown on their "assign role" forms, but are NOT included here: both are also the mechanism their own components use internally to legitimately grant a brand-new role a party doesn't hold yet (createContentRole via ContentManagementServices.java's updateSiteRoles/addRoleToUser bootstrapping site access; createQuoteRole via QuoteServicesScript.groovy assigning REQ_TAKER/INTERNAL_ORGANIZATIO when a quote is created, and copying CustRequestParty roles onto a converted quote). A blanket in-validate eca on either service would reject those legitimate internal grants, not just the vulnerable direct-form path. Fixing those two needs a different approach (validating at the form-submission event level, not the service level) and is left for separate follow-up rather than risking a regression here. Verified: applications/workeffort/servicedef/secas.xml, applications/order/servicedef/secas.xml, and applications/workeffort/testdef/workefforttests.xml all validate against their real XSD schemas (xmllint --schema). Added TimesheetRoleTests (new) and two tests to the existing OrderRequirementTests, both following the established pattern: a negative case using DemoCustomer + CARRIER (a role its seeded PartyRole set - BILL_TO_CUSTOMER/CONTACT/CUSTOMER/ END_USER_CUSTOMER/PLACING_CUSTOMER/SHIP_TO_CUSTOMER - provably does not include) and a positive case using a role it already holds. `./gradlew compileTestGroovy checkstyleMain checkstyleTest codenarcMain codenarcTest` all BUILD SUCCESSFUL. No running OFBiz+DB instance available in this environment to execute the Groovy tests end-to-end - disclosed honestly, same limitation as prior PRs in this series. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Karth <karthik.chundi@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
createTimesheetRoleandcreateRequirementRolehave the identical unvalidated-PartyRole gap already fixed for TaxAuthority (#1692), Invoice/BillingAccount/FinAccount roles (#1727), and BudgetRole (#1736): a free-choice RoleType dropdown (AddTimesheetRoleinTimesheetForms.xml,AddRequirementRoleinRequirementForms.xml) feeding straight into an unconditionalensurePartyRoleinvoke-eca, so any party/role combination is silently accepted and a spuriousPartyRolegets fabricated even when the party doesn't already hold that role.Both forms submit directly to their respective create service, and neither service has any other caller anywhere in the codebase (verified with a search across XML, Groovy, and Java — not just the immediate eca/form pair). That makes the fix a direct mirror of the already-merged/reviewed pattern: an
in-validateeca calling the existingcheckPartyRoleExistsservice before theinvokeeca that runsensurePartyRole.No JIRA ticket filed for this one specifically — found via a source-reading sweep for the same bug shape after the sibling PRs, rather than a separate bug report. Happy to file one if that's preferred before review.
Scope note — two related findings deliberately NOT included here
While sweeping for this pattern, two more services turned up with the same free-choice "assign role" form:
createContentRoleandcreateQuoteRole. Neither is included in this PR, because both are also the actual mechanism their own components use internally to legitimately grant a brand-new role to a party that doesn't hold it yet:createContentRoleis called byContentManagementServices.java'supdateSiteRoles/addRoleToUserto bootstrap a user's first-time site access.createQuoteRoleis called byQuoteServicesScript.groovyto assignREQ_TAKER/INTERNAL_ORGANIZATIOwhen a quote is created, and to copy roles when a customer request converts to a quote.A blanket
in-validateeca on either service would reject those legitimate internal grants along with the real vulnerability in their respective "assign role" forms. Fixing those two properly needs validation scoped to the form-submission path rather than the service itself, which is a different (and more careful) change — flagging here in case it's worth a separate ticket, rather than silently dropping the finding or risking a regression by rushing a blanket fix into this PR.Testing
applications/workeffort/servicedef/secas.xml,applications/order/servicedef/secas.xml, andapplications/workeffort/testdef/workefforttests.xmlall validate against their real XSD schemas (xmllint --schema).TimesheetRoleTests(new file) and two tests to the existingOrderRequirementTests, both following the established pattern from the BillingAccount/FinAccount fixes: a negative case usingDemoCustomer+CARRIER(a role its seededPartyRoleset —BILL_TO_CUSTOMER/CONTACT/CUSTOMER/END_USER_CUSTOMER/PLACING_CUSTOMER/SHIP_TO_CUSTOMER— provably does not include) and a positive case using a role it already holds../gradlew compileTestGroovy checkstyleMain checkstyleTest codenarcMain codenarcTest— allBUILD SUCCESSFUL.