WIP: Fix/search 1892 #440

agazzarini · 2019-12-06T09:01:56Z

Minor formatting, formal refacroting + additional illegal character in group id
Added unit tests for the issue

…RESTAPI project)

michaelsuzukisagi · 2019-12-09T10:37:32Z

src/test/java/org/alfresco/repo/web/scripts/groups/GroupsTest.java

@@ -357,57 +356,6 @@ public void testGetGroup() throws Exception

    }

-	/**


Any reason for removing the test

Hi @michaelsuzukisagi it was a test added by me. After talking with Meenal, we realized the tests have to be implemented in the TAS RESTAPI project.

So I left the test class as it was, (because the services tested by GroupsTest are not what we are fixing, they belong to some old api), and I'm implementing them in the other repository

I prefer having more unit tests over integration tests.

Added unit tests for this issue.

… characters are found in GroupName

CLAassistant · 2020-04-02T09:33:04Z

All committers have signed the CLA.

amohammedalfresco · 2020-04-02T16:09:23Z

I'm having confusion.

I could see there's a place where the new invalid character " would have been just appended to this array: https://github.com/Alfresco/alfresco-remote-api/blob/master/src/main/java/org/alfresco/rest/api/impl/GroupsImpl.java#L90

There's already an existing test to check for this array of invalid characters at: https://github.com/Alfresco/alfresco-remote-api/blob/master/src/test/java/org/alfresco/rest/api/tests/GroupsTest.java#L1457 without the need to write separate unit test.

As this branch is pretty old and the fact it has other changes that am not sure of. I will now fork a new branch from master and append the array with " in source code and it's test to simplify the fix.

cristianturlica · 2020-04-03T06:20:26Z

src/test/java/org/alfresco/AppContext02TestSuite.java

@@ -48,6 +48,7 @@
    org.alfresco.rest.api.tests.AuthenticationsTest.class,
    org.alfresco.rest.api.tests.DiscoveryApiTest.class,
    org.alfresco.rest.api.tests.GroupsTest.class,
+    org.alfresco.rest.api.impl.GroupsImplTest.class,


Please remove this and update the test in GroupsTest.java line 1453.

cristianturlica · 2020-04-03T06:21:11Z

src/test/java/org/alfresco/repo/web/scripts/groups/GroupsTest.java

@@ -52,6 +52,8 @@
 import org.springframework.extensions.webscripts.TestWebScriptServer.PutRequest;
 import org.springframework.extensions.webscripts.TestWebScriptServer.Response;

+import static java.util.Arrays.stream;


Why do we need this?

cristianturlica · 2020-04-03T06:22:50Z

src/test/java/org/alfresco/rest/api/impl/GroupsImplTest.java

+/*
+ * #%L
+ * Alfresco Remote API
+ * %%
+ * Copyright (C) 2005 - 2016 Alfresco Software Limited
+ * %%
+ * This file is part of the Alfresco software. 
+ * If the software was purchased under a paid Alfresco license, the terms of 
+ * the paid license agreement will prevail.  Otherwise, the software is 
+ * provided under the following open source license terms:
+ * 
+ * Alfresco is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ * 
+ * Alfresco is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Lesser General Public License for more details.
+ * 
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with Alfresco. If not, see <http://www.gnu.org/licenses/>.
+ * #L%
+ */
+package org.alfresco.rest.api.impl;
+
+import org.alfresco.repo.domain.permissions.Authority;
+import org.alfresco.repo.security.authority.AuthorityDAO;
+import org.alfresco.rest.api.impl.GroupsImpl;
+import org.alfresco.rest.api.tests.client.data.Group;
+import org.alfresco.rest.framework.resource.parameters.Parameters;
+import org.alfresco.service.cmr.security.AuthorityService;
+import org.alfresco.service.cmr.security.AuthorityType;
+import org.alfresco.service.cmr.security.PermissionService;
+import org.junit.Test;
+import org.mockito.Mockito;
+
+import static org.mockito.Matchers.*;
+import static org.mockito.Mockito.when;
+
+public class GroupsImplTest
+{
+    private GroupsImpl groupsImpl;
+
+    @Test(expected = IllegalArgumentException.class)
+    public void createGroupCheckWithillegalCharacterInGroupId() {
+        groupsImpl  = new GroupsImpl();
+        Group group= new Group();
+        String groupId="GROUP_Identifier\"WithIllegalChar";
+        group.setId(groupId);
+        group.setDisplayName(groupId);
+        AuthorityDAO authorityDAO =  Mockito.mock(AuthorityDAO.class);
+        when(authorityDAO.getAuthorityDisplayName(anyString())).thenReturn(groupId);
+        AuthorityService authorityService = Mockito.mock(AuthorityService.class);
+        groupsImpl.setAuthorityService(authorityService);
+        groupsImpl.setAuthorityDAO(authorityDAO);
+        when(authorityService.authorityExists(anyString())).thenReturn(false);
+        when(authorityService.getName(any(AuthorityType.class), anyString())).thenReturn("test");
+        when(authorityService.createAuthority(any(AuthorityType.class), anyString(), anyString(), anySet())).thenReturn(PermissionService.ALL_AUTHORITIES);
+        Parameters parameters = Mockito.mock(Parameters.class);
+        groupsImpl.create(group, parameters);
+    }
+
+    @Test()
+    public void createGroupCheckWithlegalCharacterInGroupId() {
+        groupsImpl  = new GroupsImpl();
+        Group group= new Group();
+        String groupId="GROUP_IdentifierWithLegalChar";
+        group.setId(groupId);
+        group.setDisplayName(groupId);
+        AuthorityDAO authorityDAO =  Mockito.mock(AuthorityDAO.class);
+        when(authorityDAO.getAuthorityDisplayName(anyString())).thenReturn(groupId);
+        AuthorityService authorityService = Mockito.mock(AuthorityService.class);
+        groupsImpl.setAuthorityService(authorityService);
+        groupsImpl.setAuthorityDAO(authorityDAO);
+        when(authorityService.authorityExists(anyString())).thenReturn(false);
+        when(authorityService.getName(any(AuthorityType.class), anyString())).thenReturn("test");
+        when(authorityService.createAuthority(any(AuthorityType.class), anyString(), anyString(), anySet())).thenReturn(PermissionService.ALL_AUTHORITIES);
+        Parameters parameters = Mockito.mock(Parameters.class);
+        groupsImpl.create(group, parameters);
+    }
+}


Please remove this class and update the test in GroupsTest.java line 1453.

amohammedalfresco · 2020-04-03T08:32:18Z

@agazzarini: I've raised another PR #603 in regards to my previous observations. To avoid confusion, do you consider if we can close this PR? Thanks.

agazzarini added 5 commits December 6, 2019 08:22

[SEARCH-1892] Formatting and formal refactoring

cf615fc

[SEARCH-1892] " added to illegal characters list

b24d152

[SEARCH-1892] Unit test

e91e650

[SEARCH-1892] Unit test

9dbac42

[SEARCH-1892] Restored Group Unit test (which have been moved in TAS …

2ab9940

…RESTAPI project)

michaelsuzukisagi reviewed Dec 9, 2019

View reviewed changes

agazzarini self-assigned this Dec 9, 2019

Added Unit tests for GroupsImpl class to raise exception when illegal…

f8d4f36

… characters are found in GroupName

amohammedalfresco requested a review from killerboot April 2, 2020 09:33

amohammedalfresco unassigned agazzarini Apr 2, 2020

amohammedalfresco added 3 commits April 2, 2020 10:38

Removed commented code

d901f7d

Removed commented code

6c0d558

Rename test names

97ff05c

amohammedalfresco requested a review from cristianturlica April 2, 2020 14:20

cristianturlica suggested changes Apr 3, 2020

View reviewed changes

amohammedalfresco changed the title ~~Fix/search 1892~~ WIP: Fix/search 1892 Apr 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Fix/search 1892 #440

WIP: Fix/search 1892 #440

agazzarini commented Dec 6, 2019 •

edited by amohammedalfresco

Loading

michaelsuzukisagi Dec 9, 2019

agazzarini Dec 9, 2019

michaelsuzukisagi Dec 9, 2019

amohammedalfresco Apr 2, 2020

CLAassistant commented Apr 2, 2020 •

edited

Loading

amohammedalfresco commented Apr 2, 2020

cristianturlica Apr 3, 2020

cristianturlica Apr 3, 2020

cristianturlica Apr 3, 2020

amohammedalfresco commented Apr 3, 2020

		@@ -357,57 +356,6 @@ public void testGetGroup() throws Exception

		}

		/**

WIP: Fix/search 1892 #440

Are you sure you want to change the base?

WIP: Fix/search 1892 #440

Conversation

agazzarini commented Dec 6, 2019 • edited by amohammedalfresco Loading

michaelsuzukisagi Dec 9, 2019

Choose a reason for hiding this comment

agazzarini Dec 9, 2019

Choose a reason for hiding this comment

michaelsuzukisagi Dec 9, 2019

Choose a reason for hiding this comment

amohammedalfresco Apr 2, 2020

Choose a reason for hiding this comment

CLAassistant commented Apr 2, 2020 • edited Loading

amohammedalfresco commented Apr 2, 2020

cristianturlica Apr 3, 2020

Choose a reason for hiding this comment

cristianturlica Apr 3, 2020

Choose a reason for hiding this comment

cristianturlica Apr 3, 2020

Choose a reason for hiding this comment

amohammedalfresco commented Apr 3, 2020

agazzarini commented Dec 6, 2019 •

edited by amohammedalfresco

Loading

CLAassistant commented Apr 2, 2020 •

edited

Loading