details: https://code.openbravo.com/erp/devel/pi/rev/bb3ab3249a33 changeset: 34200:bb3ab3249a33 user: Asier Lostalé <asier.lostale <at> openbravo.com> date: Thu Jun 14 16:44:11 2018 +0200 summary: related to bug 38761: create TestConstants to share some common constants
details: https://code.openbravo.com/erp/devel/pi/rev/283d1bb725e7 changeset: 34201:283d1bb725e7 user: Asier Lostalé <asier.lostale <at> openbravo.com> date: Thu Jun 14 16:49:46 2018 +0200 summary: related to bug 38761: rename inner class to prevent collisions details: https://code.openbravo.com/erp/devel/pi/rev/6588bbc83db4 changeset: 34202:6588bbc83db4 user: Asier Lostalé <asier.lostale <at> openbravo.com> date: Thu Jun 14 16:55:26 2018 +0200 summary: related to bug 38761: fixed OBBaseTest details: https://code.openbravo.com/erp/devel/pi/rev/5d06f2cf366f changeset: 34203:5d06f2cf366f user: Asier Lostalé <asier.lostale <at> openbravo.com> date: Thu Jun 14 16:56:21 2018 +0200 summary: related to bug 38761: added test case details: https://code.openbravo.com/erp/devel/pi/rev/e90a66f0b903 changeset: 34204:e90a66f0b903 user: Asier Lostalé <asier.lostale <at> openbravo.com> date: Thu Jun 14 17:08:07 2018 +0200 summary: fixed bug 38761: with write access to an org and one child, can't read siblings Having an organization A, with at least 2 child nodes B and C and a role with write access to A and only one of its children B, randomly cannot read info in C. Readable organizations should be all the ones that are part of the natural tree of each of the writable organizations. When calculating them there was a wrong optimization that assumed if an organization was already calculated to be part of the readable orgs, its natural tree would also be so there was no need to recalculate it. This is not true if it was included becuase it is part of natural tree of one of its writable descendants, in this case if this org is also writable its natural tree needs to be calculated. As calculating natural tree on memory is not that expensive it is not worth to include more complex logic to decide whether it requires to be calculated or not: now it will be calculated for all writable organizations. diffstat: src-test/src/org/openbravo/test/base/OBBaseTest.java | 5 +- src-test/src/org/openbravo/test/base/TestConstants.java | 44 ++++++++ src-test/src/org/openbravo/test/security/AllowedOrganizationsTest.java | 17 +- src-test/src/org/openbravo/test/security/WritableReadableOrganizationClientTest.java | 53 +++++++++- src/org/openbravo/dal/core/OBContext.java | 4 +- 5 files changed, 108 insertions(+), 15 deletions(-) diffs (213 lines): diff -r 9ef9e20d439e -r e90a66f0b903 src-test/src/org/openbravo/test/base/OBBaseTest.java --- a/src-test/src/org/openbravo/test/base/OBBaseTest.java Mon Jun 11 14:26:36 2018 +0200 +++ b/src-test/src/org/openbravo/test/base/OBBaseTest.java Thu Jun 14 17:08:07 2018 +0200 @@ -63,6 +63,7 @@ import org.openbravo.database.ExternalConnectionPool; import org.openbravo.model.ad.access.User; import org.openbravo.service.db.DalConnectionProvider; +import org.openbravo.test.base.TestConstants.Orgs; /** * OBBaseTest class which can/should be extended by most other test classes which want to make use @@ -172,12 +173,12 @@ /** * Record ID of Organization "F&B España - Región Norte" */ - protected static final String TEST_ORG_ID = "E443A31992CB4635AFCAEABE7183CE85"; + protected static final String TEST_ORG_ID = Orgs.ESP_NORTE; /** * Record ID of Organization "F&B US West Coast" */ - protected static final String TEST_US_ORG_ID = "BAE22373FEBE4CCCA24517E23F0C8A48"; + protected static final String TEST_US_ORG_ID = Orgs.US_WEST; /** * Record ID of Warehouse "España Región Norte" diff -r 9ef9e20d439e -r e90a66f0b903 src-test/src/org/openbravo/test/base/TestConstants.java --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/src-test/src/org/openbravo/test/base/TestConstants.java Thu Jun 14 17:08:07 2018 +0200 @@ -0,0 +1,44 @@ +/* + ************************************************************************* + * The contents of this file are subject to the Openbravo Public License + * Version 1.1 (the "License"), being the Mozilla Public License + * Version 1.1 with a permitted attribution clause; you may not use this + * file except in compliance with the License. You may obtain a copy of + * the License at http://www.openbravo.com/legal/license.html + * Software distributed under the License is distributed on an "AS IS" + * basis, WITHOUT WARRANTY OF ANY KIND, either express or implied. See the + * License for the specific language governing rights and limitations + * under the License. + * The Original Code is Openbravo ERP. + * The Initial Developer of the Original Code is Openbravo SLU + * All portions are Copyright (C) 2018 Openbravo SLU + * All Rights Reserved. + * Contributor(s): ______________________________________. + ************************************************************************ + */ + +package org.openbravo.test.base; + +/** Some constants to be used in tests */ +public class TestConstants { + + public static class Orgs { + public static final String MAIN = "0"; + public static final String FB_GROUP = "19404EAD144C49A0AF37D54377CF452D"; + + public static final String US = "2E60544D37534C0B89E765FE29BC0B43"; + public static final String US_EST = "7BABA5FF80494CAFA54DEBD22EC46F01"; + public static final String US_WEST = "BAE22373FEBE4CCCA24517E23F0C8A48"; + + public static final String ESP = "B843C30461EA4501935CB1D125C9C25A"; + public static final String ESP_SUR = "DC206C91AA6A4897B44DA897936E0EC3"; + public static final String ESP_NORTE = "E443A31992CB4635AFCAEABE7183CE85"; + } + + public static class Roles { + public static final String ESP_ADMIN = "F3196A30B53A42778727B2852FF90C24"; + } + + private TestConstants() { + } +} diff -r 9ef9e20d439e -r e90a66f0b903 src-test/src/org/openbravo/test/security/AllowedOrganizationsTest.java --- a/src-test/src/org/openbravo/test/security/AllowedOrganizationsTest.java Mon Jun 11 14:26:36 2018 +0200 +++ b/src-test/src/org/openbravo/test/security/AllowedOrganizationsTest.java Thu Jun 14 17:08:07 2018 +0200 @@ -26,6 +26,14 @@ import static org.hamcrest.Matchers.not; import static org.junit.Assert.assertThat; import static org.junit.Assume.assumeThat; +import static org.openbravo.test.base.TestConstants.Orgs.ESP; +import static org.openbravo.test.base.TestConstants.Orgs.ESP_NORTE; +import static org.openbravo.test.base.TestConstants.Orgs.ESP_SUR; +import static org.openbravo.test.base.TestConstants.Orgs.FB_GROUP; +import static org.openbravo.test.base.TestConstants.Orgs.MAIN; +import static org.openbravo.test.base.TestConstants.Orgs.US; +import static org.openbravo.test.base.TestConstants.Orgs.US_EST; +import static org.openbravo.test.base.TestConstants.Orgs.US_WEST; import java.io.IOException; import java.util.ArrayList; @@ -62,15 +70,6 @@ @RunWith(Parameterized.class) public class AllowedOrganizationsTest extends OBBaseTest { - private static final String MAIN = "0"; - private static final String FB_GROUP = "19404EAD144C49A0AF37D54377CF452D"; - private static final String US = "2E60544D37534C0B89E765FE29BC0B43"; - private static final String US_EST = "7BABA5FF80494CAFA54DEBD22EC46F01"; - private static final String US_WEST = "BAE22373FEBE4CCCA24517E23F0C8A48"; - private static final String ESP = "B843C30461EA4501935CB1D125C9C25A"; - private static final String ESP_SUR = "DC206C91AA6A4897B44DA897936E0EC3"; - private static final String ESP_NORTE = "E443A31992CB4635AFCAEABE7183CE85"; - private static final Map<String, String> ORG_NAMES = ImmutableMap.<String, String> builder() .put(MAIN, "Main") // .put(FB_GROUP, "F&B International Group") // diff -r 9ef9e20d439e -r e90a66f0b903 src-test/src/org/openbravo/test/security/WritableReadableOrganizationClientTest.java --- a/src-test/src/org/openbravo/test/security/WritableReadableOrganizationClientTest.java Mon Jun 11 14:26:36 2018 +0200 +++ b/src-test/src/org/openbravo/test/security/WritableReadableOrganizationClientTest.java Thu Jun 14 17:08:07 2018 +0200 @@ -11,7 +11,7 @@ * under the License. * The Original Code is Openbravo ERP. * The Initial Developer of the Original Code is Openbravo SLU - * All portions are Copyright (C) 2009-2014 Openbravo SLU + * All portions are Copyright (C) 2009-2018 Openbravo SLU * All Rights Reserved. * Contributor(s): ______________________________________. ************************************************************************ @@ -19,11 +19,16 @@ package org.openbravo.test.security; +import static org.hamcrest.CoreMatchers.anyOf; +import static org.hamcrest.CoreMatchers.hasItem; +import static org.hamcrest.Matchers.not; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertThat; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import java.math.BigDecimal; +import java.util.Arrays; import java.util.List; import java.util.Set; @@ -33,9 +38,13 @@ import org.openbravo.dal.core.OBContext; import org.openbravo.dal.service.OBCriteria; import org.openbravo.dal.service.OBDal; +import org.openbravo.model.ad.access.Role; +import org.openbravo.model.ad.access.RoleOrganization; import org.openbravo.model.common.businesspartner.Category; import org.openbravo.model.common.plm.Product; import org.openbravo.test.base.OBBaseTest; +import org.openbravo.test.base.TestConstants.Orgs; +import org.openbravo.test.base.TestConstants.Roles; /** * Tests check of writable organization and allowed client. @@ -157,4 +166,46 @@ e.getMessage().indexOf("is not present in ClientList") != -1); } } + + /* see issue #38761 */ + @Test + public void readSibilingOrganizationsShouldBeAllowed() { + RoleOrganization disabledAccess = null; + try { + // given a role with access to España and España Sur Organizations + OBContext.setAdminMode(false); + Role espAdminRole = OBDal.getInstance().get(Role.class, Roles.ESP_ADMIN); + for (RoleOrganization orgAccess : espAdminRole.getADRoleOrganizationList()) { + if (orgAccess.getOrganization().getId().equals(Orgs.ESP_SUR)) { + orgAccess.setActive(false); + disabledAccess = orgAccess; + } + } + + OBDal.getInstance().flush(); + + // when it is used + OBContext.setOBContext(TEST_USER_ID, Roles.ESP_ADMIN, TEST_CLIENT_ID, Orgs.ESP_NORTE); + + // then it shouldn't be able to write in sibling organizations + Set<String> writableOrganizations = OBContext.getOBContext().getWritableOrganizations(); + assertThat("Role shouldn't be able to write on España Sur", writableOrganizations, + not(hasItem(Orgs.ESP_SUR))); + + // and it should be able to read them if it has access to their ancestor + List<String> readableOrgs = Arrays + .asList(OBContext.getOBContext().getReadableOrganizations()); + assertThat("Role should be able to read España Sur", readableOrgs, hasItem(Orgs.ESP_SUR)); + + // and it should not be able to read them if it has no access to their ancestor + assertThat("Role should not be able to read any US organization", readableOrgs, + not(anyOf(hasItem(Orgs.US_EST), hasItem(Orgs.US_WEST), hasItem(Orgs.US)))); + } finally { + if (disabledAccess != null) { + disabledAccess.setActive(true); + } + OBDal.getInstance().flush(); + OBContext.restorePreviousMode(); + } + } } \ No newline at end of file diff -r 9ef9e20d439e -r e90a66f0b903 src/org/openbravo/dal/core/OBContext.java --- a/src/org/openbravo/dal/core/OBContext.java Mon Jun 11 14:26:36 2018 +0200 +++ b/src/org/openbravo/dal/core/OBContext.java Thu Jun 14 17:08:07 2018 +0200 @@ -697,9 +697,7 @@ readableOrgs.addAll(getOrganizations(getCurrentClient())); } else { for (final String o : os) { - if (!readableOrgs.contains(o)) { - readableOrgs.addAll(getOrganizationStructureProvider().getNaturalTree(o)); - } + readableOrgs.addAll(getOrganizationStructureProvider().getNaturalTree(o)); } } readableOrgs.add("0"); ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot _______________________________________________ Openbravo-commits mailing list Openbravo-commits@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openbravo-commits
