From: Dominik Orliński <d.orlinski@samsung.com>
Date: Tue, 30 Apr 2019 09:29:06 +0000 (+0200)
Subject: Fix sql injection vulnerability
X-Git-Tag: 3.2.0~298^2
X-Git-Url: https://gerrit.onap.org/r/gitweb?a=commitdiff_plain;ds=sidebyside;h=3264d36e04f57e7f9d407b49c1253f73c4bf5d72;p=portal.git

Fix sql injection vulnerability

Use a variable binding instead of concatenation.
Add new test for function 'createLocalUserIfNecessary'.

Issue-ID: OJSI-174
Change-Id: Iddd65893bb2cb16c90d4f8db59816fdf261874bc
Signed-off-by: Dominik Orliński <d.orlinski@samsung.com>
---

diff --git a/ecomp-portal-BE-common/src/main/java/org/onap/portalapp/portal/service/UserRolesCommonServiceImpl.java b/ecomp-portal-BE-common/src/main/java/org/onap/portalapp/portal/service/UserRolesCommonServiceImpl.java
index 5d9761ce..aaaf91bd 100644
--- a/ecomp-portal-BE-common/src/main/java/org/onap/portalapp/portal/service/UserRolesCommonServiceImpl.java
+++ b/ecomp-portal-BE-common/src/main/java/org/onap/portalapp/portal/service/UserRolesCommonServiceImpl.java
@@ -176,10 +176,10 @@ public class UserRolesCommonServiceImpl  {
 	 * 
 	 * @param userId
 	 */
-	protected void createLocalUserIfNecessary(String userId) {
+	protected boolean createLocalUserIfNecessary(String userId) {
 		if (StringUtils.isEmpty(userId)) {
 			logger.error(EELFLoggerDelegate.errorLogger, "createLocalUserIfNecessary : empty userId!");
-			return;
+			return false;
 		}
 		Session localSession = null;
 		Transaction transaction = null;
@@ -188,7 +188,10 @@ public class UserRolesCommonServiceImpl  {
 			transaction = localSession.beginTransaction();
 			@SuppressWarnings("unchecked")
 			List<EPUser> userList = localSession
-					.createQuery("from " + EPUser.class.getName() + " where orgUserId='" + userId + "'").list();
+					.createQuery("from :name where orgUserId=:userId")
+					.setParameter("name",EPUser.class.getName())
+					.setParameter("userId",userId)
+					.list();
 			if (userList.size() == 0) {
 				EPUser client = searchService.searchUserByUserId(userId);
 				if (client == null) {
@@ -202,9 +205,11 @@ public class UserRolesCommonServiceImpl  {
 				}
 			}
 			transaction.commit();
+			return true;
 		} catch (Exception e) {
 			EPLogUtil.logEcompError(logger, EPAppMessagesEnum.BeDaoSystemError, e);
 			EcompPortalUtils.rollbackTransaction(transaction, "searchOrCreateUser rollback, exception = " + e);
+			return false;
 		} finally {
 			EcompPortalUtils.closeLocalSession(localSession, "searchOrCreateUser");
 		}
diff --git a/ecomp-portal-BE-common/src/test/java/org/onap/portalapp/portal/service/UserRolesCommonServiceImplTest.java b/ecomp-portal-BE-common/src/test/java/org/onap/portalapp/portal/service/UserRolesCommonServiceImplTest.java
index c907a6e5..82b902a1 100644
--- a/ecomp-portal-BE-common/src/test/java/org/onap/portalapp/portal/service/UserRolesCommonServiceImplTest.java
+++ b/ecomp-portal-BE-common/src/test/java/org/onap/portalapp/portal/service/UserRolesCommonServiceImplTest.java
@@ -55,6 +55,7 @@ import java.util.TreeSet;
 import javax.servlet.http.HttpServletResponse;
 
 import org.apache.cxf.transport.http.HTTPException;
+import org.drools.core.command.assertion.AssertEquals;
 import org.hibernate.Query;
 import org.hibernate.SQLQuery;
 import org.hibernate.Session;
@@ -237,6 +238,31 @@ public class UserRolesCommonServiceImplTest {
 		return mockRoleInAppForUserList;
 	}
 
+	@SuppressWarnings("unchecked")
+	@Test
+	public void checkTheProtectionAgainstSQLInjection() throws Exception {
+		EPUser user = mockUser.mockEPUser();
+		user.setId(1l);
+		user.setOrgId(2l);
+		Query epUserQuery = Mockito.mock(Query.class);
+		List<EPUser> mockEPUserList = new ArrayList<>();
+		mockEPUserList.add(user);
+
+		// test with SQL injection, should return false
+		Mockito.when(session.createQuery("from :name where orgUserId=:userId")).thenReturn(epUserQuery);
+		Mockito.when(epUserQuery.setParameter("name",EPUser.class.getName())).thenReturn(epUserQuery);
+		Mockito.when(epUserQuery.setParameter("userId",user.getOrgUserId() + "; select * from " + EPUser.class.getName() +";")).thenReturn(epUserQuery);
+		boolean ret = userRolesCommonServiceImpl.createLocalUserIfNecessary(user.getOrgUserId());
+		assertFalse(ret);
+
+		// test without SQL injection, should return true
+		Mockito.when(session.createQuery("from :name where orgUserId=:userId")).thenReturn(epUserQuery);
+		Mockito.when(epUserQuery.setParameter("name",EPUser.class.getName())).thenReturn(epUserQuery);
+		Mockito.when(epUserQuery.setParameter("userId",user.getOrgUserId())).thenReturn(epUserQuery);
+		ret = userRolesCommonServiceImpl.createLocalUserIfNecessary(user.getOrgUserId());
+		assertTrue(ret);
+	}
+
 	@SuppressWarnings("unchecked")
 	@Test
 	public void getAppRolesForUserNonCentralizedForPortal() throws Exception {