From f28dadeea31c343d66a49547309225ee24eb99a6 Mon Sep 17 00:00:00 2001 From: Srinivas Veldurthi Date: Tue, 28 Nov 2017 14:41:43 -0800 Subject: [PATCH 01/10] DB Fix bug#67471678. Include ACLs in the db.singleDocContentSql --- .../adaptor/database/DatabaseAdaptor.java | 48 +++++++++++++++---- .../adaptor/database/DatabaseAdaptorTest.java | 39 +++++++++++++++ 2 files changed, 77 insertions(+), 10 deletions(-) diff --git a/src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java b/src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java index c0539cf..21c0dce 100644 --- a/src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java +++ b/src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java @@ -274,9 +274,9 @@ public void init(AdaptorContext context) throws Exception { if (!isNullOrEmptyString(cfg.getValue("db.aclSql"))) { aclSql = cfg.getValue("db.aclSql"); log.config("acl sql: " + aclSql); - aclPrincipalDelimiter = cfg.getValue("db.aclPrincipalDelimiter"); - log.config("aclPrincipalDelimiter: '" + aclPrincipalDelimiter + "'"); } + aclPrincipalDelimiter = cfg.getValue("db.aclPrincipalDelimiter"); + log.config("aclPrincipalDelimiter: '" + aclPrincipalDelimiter + "'"); disableStreaming = Boolean.parseBoolean(cfg.getValue("db.disableStreaming")); @@ -678,6 +678,9 @@ public void getDocContent(Request req, Response resp) throws IOException { // Generate Acl if aclSql is provided. if (aclSql != null) { resp.setAcl(getAcl(conn, id.getUniqueId())); + } else { + resp.setAcl(buildAcl(rs, aclPrincipalDelimiter, aclNamespace, + ResultSetNext.STOP_NEXT)); } // Generate response body. // In database adaptor's case, we almost never want to follow the URLs. @@ -697,18 +700,27 @@ private Acl getAcl(Connection conn, String uniqueId) throws SQLException { try (PreparedStatement stmt = getAclFromDb(conn, uniqueId); ResultSet rs = stmt.executeQuery()) { log.finer("got acl"); - return buildAcl(rs, aclPrincipalDelimiter, aclNamespace); + boolean hasResult = rs.next(); + if (!hasResult) { + // empty Acl ensures adaptor will mark this document as secure + return Acl.EMPTY; + } else { + return buildAcl(rs, aclPrincipalDelimiter, aclNamespace, + ResultSetNext.CONTINUE_NEXT); + } } } @VisibleForTesting static Acl buildAcl(ResultSet rs, String delim, String namespace) throws SQLException { - boolean hasResult = rs.next(); - if (!hasResult) { - // empty Acl ensures adaptor will mark this document as secure - return Acl.EMPTY; - } + rs.next(); + return buildAcl(rs, delim, namespace, ResultSetNext.CONTINUE_NEXT); + } + + @VisibleForTesting + static Acl buildAcl(ResultSet rs, String delim, String namespace, + ResultSetNext rsNext) throws SQLException { ResultSetMetaData metadata = rs.getMetaData(); Acl.Builder builder = new Acl.Builder(); ArrayList permitUsers = new ArrayList(); @@ -740,6 +752,9 @@ static Acl buildAcl(ResultSet rs, String delim, String namespace) denyGroups.addAll(getGroupPrincipalsFromResultSet(rs, GsaSpecialColumns.GSA_DENY_GROUPS, delim, namespace)); } + if (rsNext == ResultSetNext.STOP_NEXT) { + break; + } } while (rs.next()); return builder .setPermitUsers(permitUsers) @@ -810,14 +825,22 @@ private Connection makeNewConnection() throws SQLException { private PreparedStatement getDocFromDb(Connection conn, String uniqueId) throws SQLException { PreparedStatement st = conn.prepareStatement(singleDocContentSql); - uniqueKey.setContentSqlValues(st, uniqueId); + uniqueKey.setContentSqlValues(st, uniqueId); + if (aclSql == null) { + uniqueKey.setAclSqlValues(st, uniqueId); + } log.log(Level.FINER, "about to get doc: {0}", uniqueId); return st; } private PreparedStatement getAclFromDb(Connection conn, String uniqueId) throws SQLException { - PreparedStatement st = conn.prepareStatement(aclSql); + PreparedStatement st; + if (aclSql == null) { + st = conn.prepareStatement(singleDocContentSql); + } else { + st = conn.prepareStatement(aclSql); + } uniqueKey.setAclSqlValues(st, uniqueId); log.log(Level.FINER, "about to get acl: {0}", uniqueId); return st; @@ -1085,6 +1108,11 @@ enum GsaSpecialColumns { GSA_TIMESTAMP; } + enum ResultSetNext { + CONTINUE_NEXT, + STOP_NEXT, + } + private static class AllPublic implements AuthzAuthority { public Map isUserAuthorized(AuthnIdentity userIdentity, Collection ids) throws IOException { diff --git a/test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java b/test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java index ff557e1..52407dd 100644 --- a/test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java +++ b/test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java @@ -61,6 +61,7 @@ import com.google.enterprise.adaptor.TestHelper.RecordingContext; import com.google.enterprise.adaptor.UserPrincipal; import com.google.enterprise.adaptor.database.DatabaseAdaptor.GsaSpecialColumns; +import com.google.enterprise.adaptor.database.DatabaseAdaptor.ResultSetNext; import java.io.ByteArrayOutputStream; import java.io.File; import java.io.FileOutputStream; @@ -417,6 +418,44 @@ public void testAclSqlResultSetHasOneRecord() throws SQLException { assertEquals(golden, acl); } + @Test + public void testSingleDocSqlResultSetHasOneRecord() throws SQLException { + executeUpdate("create table data(" + + "id int, col1 varchar(20)," + + GsaSpecialColumns.GSA_PERMIT_GROUPS + " varchar(20)," + + GsaSpecialColumns.GSA_PERMIT_USERS + " varchar(20)," + + GsaSpecialColumns.GSA_DENY_GROUPS + " varchar(20)," + + GsaSpecialColumns.GSA_DENY_USERS + " varchar(20))"); + executeUpdate("insert into data(" + + "id, col1," + + GsaSpecialColumns.GSA_PERMIT_GROUPS + "," + + GsaSpecialColumns.GSA_PERMIT_USERS + "," + + GsaSpecialColumns.GSA_DENY_GROUPS + "," + + GsaSpecialColumns.GSA_DENY_USERS + ") values (" + + "1, 'test', " + + "'pgroup1, pgroup2', 'puser1, puser2', " + + "'dgroup1, dgroup2', 'duser1, duser2')"); + Acl golden = new Acl.Builder() + .setPermitUsers(Arrays.asList( + new UserPrincipal("puser2"), + new UserPrincipal("puser1"))) + .setDenyUsers(Arrays.asList( + new UserPrincipal("duser1"), + new UserPrincipal("duser2"))) + .setPermitGroups(Arrays.asList( + new GroupPrincipal("pgroup1"), + new GroupPrincipal("pgroup2"))) + .setDenyGroups(Arrays.asList( + new GroupPrincipal("dgroup2"), + new GroupPrincipal("dgroup1"))) + .build(); + ResultSet rs = executeQuery("select * from data"); + rs.next(); + Acl acl = DatabaseAdaptor.buildAcl(rs, ",", DEFAULT_NAMESPACE, + ResultSetNext.STOP_NEXT); + assertEquals(golden, acl); + } + @Test public void testAclSqlResultSetHasNonOverlappingTwoRecord() throws SQLException { From a8df1f1c9b74030b6c0b4b2edd48e3a700edcb60 Mon Sep 17 00:00:00 2001 From: Srinivas Veldurthi Date: Tue, 28 Nov 2017 18:17:58 -0800 Subject: [PATCH 02/10] Changes as per review. --- .../adaptor/database/DatabaseAdaptor.java | 32 +++++++------ .../adaptor/database/DatabaseAdaptorTest.java | 48 ++++++++++++++----- 2 files changed, 54 insertions(+), 26 deletions(-) diff --git a/src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java b/src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java index 21c0dce..6593c40 100644 --- a/src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java +++ b/src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java @@ -275,6 +275,8 @@ public void init(AdaptorContext context) throws Exception { aclSql = cfg.getValue("db.aclSql"); log.config("acl sql: " + aclSql); } + // May have delimiter and no aclSql as ACLs might come from + // singleDocContentSql rather than aclSql. aclPrincipalDelimiter = cfg.getValue("db.aclPrincipalDelimiter"); log.config("aclPrincipalDelimiter: '" + aclPrincipalDelimiter + "'"); @@ -679,8 +681,9 @@ public void getDocContent(Request req, Response resp) throws IOException { if (aclSql != null) { resp.setAcl(getAcl(conn, id.getUniqueId())); } else { + // Generate ACL if it came as part of result to singleDocContentSql. resp.setAcl(buildAcl(rs, aclPrincipalDelimiter, aclNamespace, - ResultSetNext.STOP_NEXT)); + ResultSetNext.PROCESS_ONE_ROW)); } // Generate response body. // In database adaptor's case, we almost never want to follow the URLs. @@ -706,18 +709,11 @@ private Acl getAcl(Connection conn, String uniqueId) throws SQLException { return Acl.EMPTY; } else { return buildAcl(rs, aclPrincipalDelimiter, aclNamespace, - ResultSetNext.CONTINUE_NEXT); + ResultSetNext.PROCESS_ALL_ROWS); } } } - @VisibleForTesting - static Acl buildAcl(ResultSet rs, String delim, String namespace) - throws SQLException { - rs.next(); - return buildAcl(rs, delim, namespace, ResultSetNext.CONTINUE_NEXT); - } - @VisibleForTesting static Acl buildAcl(ResultSet rs, String delim, String namespace, ResultSetNext rsNext) throws SQLException { @@ -735,6 +731,12 @@ static Acl buildAcl(ResultSet rs, String delim, String namespace, hasColumn(metadata, GsaSpecialColumns.GSA_PERMIT_GROUPS); boolean hasDenyGroups = hasColumn(metadata, GsaSpecialColumns.GSA_DENY_GROUPS); + + if (!hasPermitUsers && !hasDenyUsers + && !hasPermitGroups && !hasDenyGroups) { + return Acl.EMPTY; + } + do { if (hasPermitUsers) { permitUsers.addAll(getUserPrincipalsFromResultSet(rs, @@ -752,7 +754,7 @@ static Acl buildAcl(ResultSet rs, String delim, String namespace, denyGroups.addAll(getGroupPrincipalsFromResultSet(rs, GsaSpecialColumns.GSA_DENY_GROUPS, delim, namespace)); } - if (rsNext == ResultSetNext.STOP_NEXT) { + if (rsNext == ResultSetNext.PROCESS_ONE_ROW) { break; } } while (rs.next()); @@ -825,7 +827,7 @@ private Connection makeNewConnection() throws SQLException { private PreparedStatement getDocFromDb(Connection conn, String uniqueId) throws SQLException { PreparedStatement st = conn.prepareStatement(singleDocContentSql); - uniqueKey.setContentSqlValues(st, uniqueId); + uniqueKey.setContentSqlValues(st, uniqueId); if (aclSql == null) { uniqueKey.setAclSqlValues(st, uniqueId); } @@ -837,9 +839,9 @@ private PreparedStatement getAclFromDb(Connection conn, String uniqueId) throws SQLException { PreparedStatement st; if (aclSql == null) { - st = conn.prepareStatement(singleDocContentSql); + st = conn.prepareStatement(singleDocContentSql); } else { - st = conn.prepareStatement(aclSql); + st = conn.prepareStatement(aclSql); } uniqueKey.setAclSqlValues(st, uniqueId); log.log(Level.FINER, "about to get acl: {0}", uniqueId); @@ -1109,8 +1111,8 @@ enum GsaSpecialColumns { } enum ResultSetNext { - CONTINUE_NEXT, - STOP_NEXT, + PROCESS_ALL_ROWS, + PROCESS_ONE_ROW, } private static class AllPublic implements AuthzAuthority { diff --git a/test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java b/test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java index 52407dd..b5bbe60 100644 --- a/test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java +++ b/test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java @@ -364,6 +364,13 @@ public void testHasColumn() throws Exception { GsaSpecialColumns.GSA_TIMESTAMP)); } + private Acl buildAcl(ResultSet rs, String delim, String namespace) + throws SQLException { + rs.next(); + return DatabaseAdaptor.buildAcl(rs, delim, namespace, + ResultSetNext.PROCESS_ALL_ROWS); + } + @Test public void testAclSqlResultSetHasNoAclColumns() throws SQLException { Acl golden = Acl.EMPTY; @@ -371,7 +378,7 @@ public void testAclSqlResultSetHasNoAclColumns() throws SQLException { executeUpdate("create table acl(id integer)"); executeUpdate("insert into acl(id) values(1)"); ResultSet rs = executeQuery("select * from acl"); - Acl acl = DatabaseAdaptor.buildAcl(rs, ",", DEFAULT_NAMESPACE); + Acl acl = buildAcl(rs, ",", DEFAULT_NAMESPACE); assertEquals(golden, acl); } @@ -381,7 +388,7 @@ public void testAclSqlResultSetHasNoRecord() throws SQLException { executeUpdate("create table acl(id integer)"); ResultSet rs = executeQuery("select * from acl"); - Acl acl = DatabaseAdaptor.buildAcl(rs, ",", DEFAULT_NAMESPACE); + Acl acl = buildAcl(rs, ",", DEFAULT_NAMESPACE); assertEquals(golden, acl); } @@ -414,12 +421,12 @@ public void testAclSqlResultSetHasOneRecord() throws SQLException { new GroupPrincipal("dgroup1"))) .build(); ResultSet rs = executeQuery("select * from acl"); - Acl acl = DatabaseAdaptor.buildAcl(rs, ",", DEFAULT_NAMESPACE); + Acl acl = buildAcl(rs, ",", DEFAULT_NAMESPACE); assertEquals(golden, acl); } @Test - public void testSingleDocSqlResultSetHasOneRecord() throws SQLException { + public void testSingleDocSqlResultSetHasTwoRecords() throws Exception { executeUpdate("create table data(" + "id int, col1 varchar(20)," + GsaSpecialColumns.GSA_PERMIT_GROUPS + " varchar(20)," @@ -435,6 +442,15 @@ public void testSingleDocSqlResultSetHasOneRecord() throws SQLException { + "1, 'test', " + "'pgroup1, pgroup2', 'puser1, puser2', " + "'dgroup1, dgroup2', 'duser1, duser2')"); + executeUpdate("insert into data(" + + "id, col1," + + GsaSpecialColumns.GSA_PERMIT_GROUPS + "," + + GsaSpecialColumns.GSA_PERMIT_USERS + "," + + GsaSpecialColumns.GSA_DENY_GROUPS + "," + + GsaSpecialColumns.GSA_DENY_USERS + ") values (" + + "2, 'test2', " + + "'pgroup3, pgroup4', 'puser3, puser4', " + + "'dgroup3, dgroup4', 'duser3, duser4')"); Acl golden = new Acl.Builder() .setPermitUsers(Arrays.asList( new UserPrincipal("puser2"), @@ -449,10 +465,20 @@ public void testSingleDocSqlResultSetHasOneRecord() throws SQLException { new GroupPrincipal("dgroup2"), new GroupPrincipal("dgroup1"))) .build(); - ResultSet rs = executeQuery("select * from data"); - rs.next(); - Acl acl = DatabaseAdaptor.buildAcl(rs, ",", DEFAULT_NAMESPACE, - ResultSetNext.STOP_NEXT); + + Map configEntries = new HashMap(); + configEntries.put("db.uniqueKey", "id:int"); + configEntries.put("db.everyDocIdSql", "select id from data"); + configEntries.put("db.singleDocContentSql", + "select * from data where id = ?"); + configEntries.put("db.modeOfOperation", "rowToText"); + + DatabaseAdaptor adaptor = getObjectUnderTest(configEntries); + MockRequest request = new MockRequest(new DocId("1")); + RecordingResponse response = new RecordingResponse(); + adaptor.getDocContent(request, response); + + Acl acl = response.getAcl(); assertEquals(golden, acl); } @@ -501,7 +527,7 @@ public void testAclSqlResultSetHasNonOverlappingTwoRecord() new GroupPrincipal("dgroup2"))) .build(); ResultSet rs = executeQuery("select * from acl"); - Acl acl = DatabaseAdaptor.buildAcl(rs, ",", DEFAULT_NAMESPACE); + Acl acl = buildAcl(rs, ",", DEFAULT_NAMESPACE); assertEquals(golden, acl); } @@ -542,7 +568,7 @@ public void testAclSqlResultSetHasOverlappingTwoRecord() new GroupPrincipal("dgroup2"))) .build(); ResultSet rs = executeQuery("select * from acl"); - Acl acl = DatabaseAdaptor.buildAcl(rs, ",", DEFAULT_NAMESPACE); + Acl acl = buildAcl(rs, ",", DEFAULT_NAMESPACE); assertEquals(golden, acl); } @@ -576,7 +602,7 @@ public void testAclSqlResultSetOneColumnPerRow() throws SQLException { new UserPrincipal("duser2"))) .build(); ResultSet rs = executeQuery("select * from acl"); - Acl acl = DatabaseAdaptor.buildAcl(rs, ",", DEFAULT_NAMESPACE); + Acl acl = buildAcl(rs, ",", DEFAULT_NAMESPACE); assertEquals(golden, acl); } From 16f86738a4267a08b95971ba10ccb86ca14873eb Mon Sep 17 00:00:00 2001 From: Srinivas Veldurthi Date: Wed, 29 Nov 2017 18:39:53 -0800 Subject: [PATCH 03/10] Changes as suggested. --- .../adaptor/database/DatabaseAdaptor.java | 20 +++++--------- .../adaptor/database/DatabaseAdaptorTest.java | 26 ++++++++++++++++--- 2 files changed, 30 insertions(+), 16 deletions(-) diff --git a/src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java b/src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java index 6593c40..35b13a5 100644 --- a/src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java +++ b/src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java @@ -683,7 +683,7 @@ public void getDocContent(Request req, Response resp) throws IOException { } else { // Generate ACL if it came as part of result to singleDocContentSql. resp.setAcl(buildAcl(rs, aclPrincipalDelimiter, aclNamespace, - ResultSetNext.PROCESS_ONE_ROW)); + ResultSetNext.PROCESS_ONE_ROW, null)); } // Generate response body. // In database adaptor's case, we almost never want to follow the URLs. @@ -709,14 +709,14 @@ private Acl getAcl(Connection conn, String uniqueId) throws SQLException { return Acl.EMPTY; } else { return buildAcl(rs, aclPrincipalDelimiter, aclNamespace, - ResultSetNext.PROCESS_ALL_ROWS); + ResultSetNext.PROCESS_ALL_ROWS, Acl.EMPTY); } } } @VisibleForTesting static Acl buildAcl(ResultSet rs, String delim, String namespace, - ResultSetNext rsNext) throws SQLException { + ResultSetNext rsNext, Acl defaultAcl) throws SQLException { ResultSetMetaData metadata = rs.getMetaData(); Acl.Builder builder = new Acl.Builder(); ArrayList permitUsers = new ArrayList(); @@ -734,7 +734,7 @@ static Acl buildAcl(ResultSet rs, String delim, String namespace, if (!hasPermitUsers && !hasDenyUsers && !hasPermitGroups && !hasDenyGroups) { - return Acl.EMPTY; + return defaultAcl; } do { @@ -828,21 +828,15 @@ private PreparedStatement getDocFromDb(Connection conn, String uniqueId) throws SQLException { PreparedStatement st = conn.prepareStatement(singleDocContentSql); uniqueKey.setContentSqlValues(st, uniqueId); - if (aclSql == null) { - uniqueKey.setAclSqlValues(st, uniqueId); - } log.log(Level.FINER, "about to get doc: {0}", uniqueId); return st; } private PreparedStatement getAclFromDb(Connection conn, String uniqueId) throws SQLException { - PreparedStatement st; - if (aclSql == null) { - st = conn.prepareStatement(singleDocContentSql); - } else { - st = conn.prepareStatement(aclSql); - } + // aclSql will be null when called from isUserAuthorized. + String sql = (aclSql == null) ? singleDocContentSql : aclSql; + PreparedStatement st = conn.prepareStatement(sql); uniqueKey.setAclSqlValues(st, uniqueId); log.log(Level.FINER, "about to get acl: {0}", uniqueId); return st; diff --git a/test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java b/test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java index b5bbe60..a944142 100644 --- a/test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java +++ b/test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java @@ -368,7 +368,7 @@ private Acl buildAcl(ResultSet rs, String delim, String namespace) throws SQLException { rs.next(); return DatabaseAdaptor.buildAcl(rs, delim, namespace, - ResultSetNext.PROCESS_ALL_ROWS); + ResultSetNext.PROCESS_ALL_ROWS, Acl.EMPTY); } @Test @@ -478,8 +478,28 @@ public void testSingleDocSqlResultSetHasTwoRecords() throws Exception { RecordingResponse response = new RecordingResponse(); adaptor.getDocContent(request, response); - Acl acl = response.getAcl(); - assertEquals(golden, acl); + assertEquals(golden, response.getAcl()); + } + + @Test + public void testSingleDocSqlHasNoAcl() throws Exception { + executeUpdate("create table data(id int, col1 varchar(20))"); + executeUpdate("insert into data(id, col1) values (1, 'test')"); + Acl golden = null; + + Map configEntries = new HashMap(); + configEntries.put("db.uniqueKey", "id:int"); + configEntries.put("db.everyDocIdSql", "select id from data"); + configEntries.put("db.singleDocContentSql", + "select * from data where id = ?"); + configEntries.put("db.modeOfOperation", "rowToText"); + + DatabaseAdaptor adaptor = getObjectUnderTest(configEntries); + MockRequest request = new MockRequest(new DocId("1")); + RecordingResponse response = new RecordingResponse(); + adaptor.getDocContent(request, response); + + assertEquals(golden, response.getAcl()); } @Test From ec208496dfc2634252bb24c8d978eee0565ff889 Mon Sep 17 00:00:00 2001 From: Srinivas Veldurthi Date: Thu, 30 Nov 2017 17:27:17 -0800 Subject: [PATCH 04/10] Changes as per review. --- .../enterprise/adaptor/database/DatabaseAdaptor.java | 7 ++----- .../enterprise/adaptor/database/DatabaseAdaptorTest.java | 9 +++++---- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java b/src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java index 35b13a5..c46eebe 100644 --- a/src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java +++ b/src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java @@ -754,10 +754,7 @@ static Acl buildAcl(ResultSet rs, String delim, String namespace, denyGroups.addAll(getGroupPrincipalsFromResultSet(rs, GsaSpecialColumns.GSA_DENY_GROUPS, delim, namespace)); } - if (rsNext == ResultSetNext.PROCESS_ONE_ROW) { - break; - } - } while (rs.next()); + } while (rsNext == ResultSetNext.PROCESS_ALL_ROWS && rs.next()); return builder .setPermitUsers(permitUsers) .setDenyUsers(denyUsers) @@ -834,7 +831,7 @@ private PreparedStatement getDocFromDb(Connection conn, private PreparedStatement getAclFromDb(Connection conn, String uniqueId) throws SQLException { - // aclSql will be null when called from isUserAuthorized. + // aclSql will only be null when called from isUserAuthorized. String sql = (aclSql == null) ? singleDocContentSql : aclSql; PreparedStatement st = conn.prepareStatement(sql); uniqueKey.setAclSqlValues(st, uniqueId); diff --git a/test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java b/test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java index a944142..f597291 100644 --- a/test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java +++ b/test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java @@ -468,10 +468,11 @@ public void testSingleDocSqlResultSetHasTwoRecords() throws Exception { Map configEntries = new HashMap(); configEntries.put("db.uniqueKey", "id:int"); - configEntries.put("db.everyDocIdSql", "select id from data"); configEntries.put("db.singleDocContentSql", "select * from data where id = ?"); configEntries.put("db.modeOfOperation", "rowToText"); + // Required for validation, but not specific to this test. + configEntries.put("db.everyDocIdSql", "select id from data"); DatabaseAdaptor adaptor = getObjectUnderTest(configEntries); MockRequest request = new MockRequest(new DocId("1")); @@ -485,21 +486,21 @@ public void testSingleDocSqlResultSetHasTwoRecords() throws Exception { public void testSingleDocSqlHasNoAcl() throws Exception { executeUpdate("create table data(id int, col1 varchar(20))"); executeUpdate("insert into data(id, col1) values (1, 'test')"); - Acl golden = null; Map configEntries = new HashMap(); configEntries.put("db.uniqueKey", "id:int"); - configEntries.put("db.everyDocIdSql", "select id from data"); configEntries.put("db.singleDocContentSql", "select * from data where id = ?"); configEntries.put("db.modeOfOperation", "rowToText"); + // Required for validation, but not specific to this test. + configEntries.put("db.everyDocIdSql", "select id from data"); DatabaseAdaptor adaptor = getObjectUnderTest(configEntries); MockRequest request = new MockRequest(new DocId("1")); RecordingResponse response = new RecordingResponse(); adaptor.getDocContent(request, response); - assertEquals(golden, response.getAcl()); + assertNull(response.getAcl()); } @Test From bdbe6c5f241da5246b7b133289ab95bf5c1cab0d Mon Sep 17 00:00:00 2001 From: Srinivas Veldurthi Date: Tue, 5 Dec 2017 13:25:29 -0800 Subject: [PATCH 05/10] Restructured code around getAcl() - added processAclQuery() - removed getAclFromDb() Added tests. --- .../adaptor/database/DatabaseAdaptor.java | 69 ++++++++++--------- .../adaptor/database/DatabaseAdaptorTest.java | 54 +++++++++++++++ 2 files changed, 91 insertions(+), 32 deletions(-) diff --git a/src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java b/src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java index c46eebe..5028405 100644 --- a/src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java +++ b/src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java @@ -293,11 +293,7 @@ public void init(AdaptorContext context) throws Exception { ignored.add("db.updateTimestampTimezone"); } - if (aclSql == null) { - context.setAuthzAuthority(new AllPublic()); - } else { - context.setAuthzAuthority(new AccessChecker()); - } + context.setAuthzAuthority(new AccessChecker()); aclNamespace = cfg.getValue("adaptor.namespace"); log.config("namespace: " + aclNamespace); @@ -677,14 +673,8 @@ public void getDocContent(Request req, Response resp) throws IOException { } // Generate response metadata first. addMetadataToResponse(resp, rs); - // Generate Acl if aclSql is provided. - if (aclSql != null) { - resp.setAcl(getAcl(conn, id.getUniqueId())); - } else { - // Generate ACL if it came as part of result to singleDocContentSql. - resp.setAcl(buildAcl(rs, aclPrincipalDelimiter, aclNamespace, - ResultSetNext.PROCESS_ONE_ROW, null)); - } + // Generate Acl. + resp.setAcl(getAcl(conn, rs, id.getUniqueId())); // Generate response body. // In database adaptor's case, we almost never want to follow the URLs. // One record means one document. @@ -699,17 +689,38 @@ public static void main(String[] args) { AbstractAdaptor.main(new DatabaseAdaptor(), args); } - private Acl getAcl(Connection conn, String uniqueId) throws SQLException { - try (PreparedStatement stmt = getAclFromDb(conn, uniqueId); - ResultSet rs = stmt.executeQuery()) { + private Acl getAcl(Connection conn, ResultSet rs, String uniqueId) + throws SQLException { + log.log(Level.FINER, "about to get acl: {0}", uniqueId); + if (aclSql != null || rs == null) { + String sql = (aclSql == null) ? singleDocContentSql : aclSql; + try (PreparedStatement st = conn.prepareStatement(sql)) { + // aclSql will only be null when called from isUserAuthorized. + if (aclSql == null) { + uniqueKey.setContentSqlValues(st, uniqueId); + return processAclQuery(st, null); + } else { + uniqueKey.setAclSqlValues(st, uniqueId); + return processAclQuery(st, Acl.EMPTY); + } + } + } else { + // Generate ACL if it came as part of result to singleDocContentSql. + return buildAcl(rs, aclPrincipalDelimiter, aclNamespace, + ResultSetNext.PROCESS_ONE_ROW, null); + } + } + + private Acl processAclQuery(PreparedStatement st, Acl defaultAcl) + throws SQLException { + try (ResultSet resSet = st.executeQuery()) { log.finer("got acl"); - boolean hasResult = rs.next(); + boolean hasResult = resSet.next(); if (!hasResult) { - // empty Acl ensures adaptor will mark this document as secure - return Acl.EMPTY; + return defaultAcl; } else { - return buildAcl(rs, aclPrincipalDelimiter, aclNamespace, - ResultSetNext.PROCESS_ALL_ROWS, Acl.EMPTY); + return buildAcl(resSet, aclPrincipalDelimiter, aclNamespace, + ResultSetNext.PROCESS_ALL_ROWS, defaultAcl); } } } @@ -829,16 +840,6 @@ private PreparedStatement getDocFromDb(Connection conn, return st; } - private PreparedStatement getAclFromDb(Connection conn, - String uniqueId) throws SQLException { - // aclSql will only be null when called from isUserAuthorized. - String sql = (aclSql == null) ? singleDocContentSql : aclSql; - PreparedStatement st = conn.prepareStatement(sql); - uniqueKey.setAclSqlValues(st, uniqueId); - log.log(Level.FINER, "about to get acl: {0}", uniqueId); - return st; - } - private PreparedStatement getStreamFromDb(Connection conn, String query) throws SQLException { PreparedStatement st; @@ -1146,7 +1147,11 @@ public Map isUserAuthorized(AuthnIdentity userIdentity, try (Connection conn = makeNewConnection()) { for (DocId id : ids) { log.log(Level.FINE, "about to get acl of doc {0}", id); - Acl acl = getAcl(conn, id.getUniqueId()); + Acl acl = getAcl(conn, null, id.getUniqueId()); + if (acl == null) { + result.put(id, AuthzStatus.PERMIT); + continue; + } List aclChain = Arrays.asList(acl); log.log(Level.FINE, "about to authorize user {0} for doc {1} and acl {2}", diff --git a/test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java b/test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java index f597291..cbc6413 100644 --- a/test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java +++ b/test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java @@ -2946,4 +2946,58 @@ public void testAuthzAuthorityAcl_sqlException() throws Exception { getAuthnIdentity(new UserPrincipal("alice")), Arrays.asList(new DocId("1"), new DocId("2"))); } + + @Test + public void testAuthzAuthoritySingleDocContentSql_noResults() + throws Exception { + executeUpdate("create table data(id integer)"); + + Map moreEntries = new HashMap(); + moreEntries.put("db.modeOfOperation", "rowToText"); + moreEntries.put("db.uniqueKey", "id:int"); + moreEntries.put("db.singleDocContentSql", + "select * from data where id = ?"); + // Required for validation, but not specific to this test. + moreEntries.put("db.everyDocIdSql", "select id from data"); + + AuthzAuthority authority = getAuthzAuthority(moreEntries); + Map answers = authority.isUserAuthorized( + getAuthnIdentity(new UserPrincipal("alice")), + Arrays.asList(new DocId("1"), new DocId("2"))); + + HashMap golden = new HashMap<>(); + golden.put(new DocId("1"), AuthzStatus.PERMIT); + golden.put(new DocId("2"), AuthzStatus.PERMIT); + assertEquals(golden, answers); + } + + @Test + public void testAuthzAuthoritySingleDocContentSql_permit() throws Exception { + executeUpdate("create table data(id integer, loginid varchar(20), " + + "gsa_permit_users varchar(20), gsa_deny_users varchar(20))"); + executeUpdate("insert into data(id, loginid, gsa_deny_users) values " + + "(1, 'tom', 'tom')"); + executeUpdate("insert into data(id, loginid, gsa_permit_users) values " + + "(2, 'alice', 'alice')"); + + Map moreEntries = new HashMap(); + moreEntries.put("db.aclSqlParameters", "loginid"); + moreEntries.put("db.singleDocContentSqlParameters", "id, loginid"); + moreEntries.put("db.modeOfOperation", "rowToText"); + moreEntries.put("db.uniqueKey", "id:int,loginid:string"); + moreEntries.put("db.singleDocContentSql", + "select * from data where id = ? and loginid = ?"); + // Required for validation, but not specific to this test. + moreEntries.put("db.everyDocIdSql", "select id, loginid from data"); + + AuthzAuthority authority = getAuthzAuthority(moreEntries); + Map answers = authority.isUserAuthorized( + getAuthnIdentity(new UserPrincipal("alice")), + Arrays.asList(new DocId("1/tom"), new DocId("2/alice"))); + + HashMap golden = new HashMap<>(); + golden.put(new DocId("1/tom"), AuthzStatus.DENY); + golden.put(new DocId("2/alice"), AuthzStatus.PERMIT); + assertEquals(golden, answers); + } } From 9f2edd6aee94b2de271409c2d10608e31032e69e Mon Sep 17 00:00:00 2001 From: Srinivas Veldurthi Date: Thu, 7 Dec 2017 17:09:44 -0800 Subject: [PATCH 06/10] Restored getAclFromDb(). Removed AllPublic. --- .../adaptor/database/DatabaseAdaptor.java | 63 +++++++++---------- 1 file changed, 28 insertions(+), 35 deletions(-) diff --git a/src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java b/src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java index 5028405..8bb8f9f 100644 --- a/src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java +++ b/src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java @@ -691,17 +691,21 @@ public static void main(String[] args) { private Acl getAcl(Connection conn, ResultSet rs, String uniqueId) throws SQLException { - log.log(Level.FINER, "about to get acl: {0}", uniqueId); if (aclSql != null || rs == null) { - String sql = (aclSql == null) ? singleDocContentSql : aclSql; - try (PreparedStatement st = conn.prepareStatement(sql)) { - // aclSql will only be null when called from isUserAuthorized. - if (aclSql == null) { - uniqueKey.setContentSqlValues(st, uniqueId); - return processAclQuery(st, null); + try (PreparedStatement stmt = getAclFromDb(conn, uniqueId); + ResultSet resSet = stmt.executeQuery()) { + log.finer("got acl"); + boolean hasResult = resSet.next(); + if (!hasResult) { + if (aclSql == null) { + return null; + } else { + // empty Acl ensures adaptor will mark this document as secure + return Acl.EMPTY; + } } else { - uniqueKey.setAclSqlValues(st, uniqueId); - return processAclQuery(st, Acl.EMPTY); + return buildAcl(resSet, aclPrincipalDelimiter, aclNamespace, + ResultSetNext.PROCESS_ALL_ROWS, Acl.EMPTY); } } } else { @@ -710,20 +714,6 @@ private Acl getAcl(Connection conn, ResultSet rs, String uniqueId) ResultSetNext.PROCESS_ONE_ROW, null); } } - - private Acl processAclQuery(PreparedStatement st, Acl defaultAcl) - throws SQLException { - try (ResultSet resSet = st.executeQuery()) { - log.finer("got acl"); - boolean hasResult = resSet.next(); - if (!hasResult) { - return defaultAcl; - } else { - return buildAcl(resSet, aclPrincipalDelimiter, aclNamespace, - ResultSetNext.PROCESS_ALL_ROWS, defaultAcl); - } - } - } @VisibleForTesting static Acl buildAcl(ResultSet rs, String delim, String namespace, @@ -840,6 +830,21 @@ private PreparedStatement getDocFromDb(Connection conn, return st; } + private PreparedStatement getAclFromDb(Connection conn, + String uniqueId) throws SQLException { + PreparedStatement st; + // aclSql will only be null when called from isUserAuthorized. + if (aclSql == null) { + st = conn.prepareStatement(singleDocContentSql); + uniqueKey.setContentSqlValues(st, uniqueId); + } else { + st = conn.prepareStatement(aclSql); + uniqueKey.setAclSqlValues(st, uniqueId); + } + log.log(Level.FINER, "about to get acl: {0}", uniqueId); + return st; + } + private PreparedStatement getStreamFromDb(Connection conn, String query) throws SQLException { PreparedStatement st; @@ -1107,18 +1112,6 @@ enum ResultSetNext { PROCESS_ONE_ROW, } - private static class AllPublic implements AuthzAuthority { - public Map isUserAuthorized(AuthnIdentity userIdentity, - Collection ids) throws IOException { - Map result = - new HashMap(ids.size() * 2); - for (DocId docId : ids) { - result.put(docId, AuthzStatus.PERMIT); - } - return Collections.unmodifiableMap(result); - } - } - private static Map allDeny(Collection ids) { Map result = new HashMap(ids.size() * 2); From ad415381472c6972dccaf83f697e109a1f8b444a Mon Sep 17 00:00:00 2001 From: Srinivas Veldurthi Date: Mon, 11 Dec 2017 18:17:10 -0800 Subject: [PATCH 07/10] Modified getAcl(), isUserAuthorized() and added tests. --- .../adaptor/database/DatabaseAdaptor.java | 60 ++++++++------- .../adaptor/database/DatabaseAdaptorTest.java | 74 ++++++++++++++++--- 2 files changed, 92 insertions(+), 42 deletions(-) diff --git a/src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java b/src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java index 8bb8f9f..5ba9f37 100644 --- a/src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java +++ b/src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java @@ -697,15 +697,16 @@ private Acl getAcl(Connection conn, ResultSet rs, String uniqueId) log.finer("got acl"); boolean hasResult = resSet.next(); if (!hasResult) { - if (aclSql == null) { - return null; + // empty Acl ensures adaptor will mark this document as secure + return Acl.EMPTY; + } else { + if (aclSql != null) { + return buildAcl(resSet, aclPrincipalDelimiter, aclNamespace, + ResultSetNext.PROCESS_ALL_ROWS, Acl.EMPTY); } else { - // empty Acl ensures adaptor will mark this document as secure - return Acl.EMPTY; + return buildAcl(resSet, aclPrincipalDelimiter, aclNamespace, + ResultSetNext.PROCESS_ALL_ROWS, null); } - } else { - return buildAcl(resSet, aclPrincipalDelimiter, aclNamespace, - ResultSetNext.PROCESS_ALL_ROWS, Acl.EMPTY); } } } else { @@ -1124,36 +1125,33 @@ private static Map allDeny(Collection ids) { private class AccessChecker implements AuthzAuthority { public Map isUserAuthorized(AuthnIdentity userIdentity, Collection ids) throws IOException { - if (null == userIdentity) { - log.info("null identity to authorize"); - return allDeny(ids); // TODO: consider way to permit public - } - UserPrincipal user = userIdentity.getUser(); - if (null == user) { - log.info("null user to authorize"); - return allDeny(ids); // TODO: consider way to permit public - } - log.log(Level.INFO, "about to authorize {0} {1}", - new Object[]{user, userIdentity.getGroups()}); Map result = new HashMap(ids.size() * 2); try (Connection conn = makeNewConnection()) { for (DocId id : ids) { log.log(Level.FINE, "about to get acl of doc {0}", id); - Acl acl = getAcl(conn, null, id.getUniqueId()); - if (acl == null) { - result.put(id, AuthzStatus.PERMIT); - continue; + if (userIdentity == null) { + result.put(id, AuthzStatus.DENY); + } else if (userIdentity.getUser() == null) { + result.put(id, AuthzStatus.DENY); + } else { + Acl acl = getAcl(conn, null, id.getUniqueId()); + if (acl == null) { + result.put(id, AuthzStatus.PERMIT); + } else { + log.log(Level.INFO, "about to authorize {0} {1}", + new Object[]{user, userIdentity.getGroups()}); + List aclChain = Arrays.asList(acl); + log.log(Level.FINE, + "about to authorize user {0} for doc {1} and acl {2}", + new Object[]{user, id, acl}); + AuthzStatus decision = Acl.isAuthorized(userIdentity, aclChain); + log.log(Level.FINE, + "authorization decision {0} for user {1} and doc {2}", + new Object[]{decision, user, id}); + result.put(id, decision); + } } - List aclChain = Arrays.asList(acl); - log.log(Level.FINE, - "about to authorize user {0} for doc {1} and acl {2}", - new Object[]{user, id, acl}); - AuthzStatus decision = Acl.isAuthorized(userIdentity, aclChain); - log.log(Level.FINE, - "authorization decision {0} for user {1} and doc {2}", - new Object[]{decision, user, id}); - result.put(id, decision); } } catch (SQLException ex) { throw new IOException("authz retrieval error", ex); diff --git a/test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java b/test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java index cbc6413..0d433a9 100644 --- a/test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java +++ b/test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java @@ -2805,6 +2805,7 @@ public void testAuthzAuthority_public() throws Exception { moreEntries.put("db.everyDocIdSql", "select id from data"); moreEntries.put("db.singleDocContentSql", "select * from data where id = ?"); + executeUpdate("insert into data(id) values (1),(2)"); AuthzAuthority authority = getAuthzAuthority(moreEntries); Map answers = authority.isUserAuthorized( @@ -2966,29 +2967,29 @@ public void testAuthzAuthoritySingleDocContentSql_noResults() Arrays.asList(new DocId("1"), new DocId("2"))); HashMap golden = new HashMap<>(); - golden.put(new DocId("1"), AuthzStatus.PERMIT); - golden.put(new DocId("2"), AuthzStatus.PERMIT); + golden.put(new DocId("1"), AuthzStatus.INDETERMINATE); + golden.put(new DocId("2"), AuthzStatus.INDETERMINATE); assertEquals(golden, answers); } @Test public void testAuthzAuthoritySingleDocContentSql_permit() throws Exception { - executeUpdate("create table data(id integer, loginid varchar(20), " + executeUpdate("create table data(id integer, author varchar(20), " + "gsa_permit_users varchar(20), gsa_deny_users varchar(20))"); - executeUpdate("insert into data(id, loginid, gsa_deny_users) values " - + "(1, 'tom', 'tom')"); - executeUpdate("insert into data(id, loginid, gsa_permit_users) values " + executeUpdate("insert into data(id, author, gsa_deny_users) values " + + "(1, 'tom', 'alice')"); + executeUpdate("insert into data(id, author, gsa_permit_users) values " + "(2, 'alice', 'alice')"); Map moreEntries = new HashMap(); - moreEntries.put("db.aclSqlParameters", "loginid"); - moreEntries.put("db.singleDocContentSqlParameters", "id, loginid"); + moreEntries.put("db.aclSqlParameters", "author"); + moreEntries.put("db.singleDocContentSqlParameters", "id, author"); moreEntries.put("db.modeOfOperation", "rowToText"); - moreEntries.put("db.uniqueKey", "id:int,loginid:string"); + moreEntries.put("db.uniqueKey", "id:int,author:string"); moreEntries.put("db.singleDocContentSql", - "select * from data where id = ? and loginid = ?"); + "select * from data where id = ? and author = ?"); // Required for validation, but not specific to this test. - moreEntries.put("db.everyDocIdSql", "select id, loginid from data"); + moreEntries.put("db.everyDocIdSql", "select id, author from data"); AuthzAuthority authority = getAuthzAuthority(moreEntries); Map answers = authority.isUserAuthorized( @@ -3000,4 +3001,55 @@ public void testAuthzAuthoritySingleDocContentSql_permit() throws Exception { golden.put(new DocId("2/alice"), AuthzStatus.PERMIT); assertEquals(golden, answers); } + + @Test + public void testAuthzAuthoritySingleDocContentSql_noAclColumns() + throws Exception { + executeUpdate("create table data(id integer)"); + executeUpdate("insert into data(id) values (2)"); + + Map moreEntries = new HashMap(); + moreEntries.put("db.modeOfOperation", "rowToText"); + moreEntries.put("db.uniqueKey", "id:int"); + moreEntries.put("db.singleDocContentSql", + "select * from data where id = ?"); + // Required for validation, but not specific to this test. + moreEntries.put("db.everyDocIdSql", "select id from data"); + + AuthzAuthority authority = getAuthzAuthority(moreEntries); + Map answers = authority.isUserAuthorized( + getAuthnIdentity(new UserPrincipal("alice")), + Arrays.asList(new DocId("1"), new DocId("2"))); + + HashMap golden = new HashMap<>(); + golden.put(new DocId("1"), AuthzStatus.INDETERMINATE); + golden.put(new DocId("2"), AuthzStatus.PERMIT); + assertEquals(golden, answers); + } + + @Test + public void testAuthzAuthoritySingleDocContentSql_noAclEntries() + throws Exception { + executeUpdate("create table data(id integer, " + + "gsa_permit_users varchar(20))"); + executeUpdate("insert into data(id) values (2)"); + + Map moreEntries = new HashMap(); + moreEntries.put("db.modeOfOperation", "rowToText"); + moreEntries.put("db.uniqueKey", "id:int"); + moreEntries.put("db.singleDocContentSql", + "select * from data where id = ?"); + // Required for validation, but not specific to this test. + moreEntries.put("db.everyDocIdSql", "select id from data"); + + AuthzAuthority authority = getAuthzAuthority(moreEntries); + Map answers = authority.isUserAuthorized( + getAuthnIdentity(new UserPrincipal("alice")), + Arrays.asList(new DocId("1"), new DocId("2"))); + + HashMap golden = new HashMap<>(); + golden.put(new DocId("1"), AuthzStatus.INDETERMINATE); + golden.put(new DocId("2"), AuthzStatus.INDETERMINATE); + assertEquals(golden, answers); + } } From e07e129e3cbd92309187cb52b46fdf5b8c89b1c4 Mon Sep 17 00:00:00 2001 From: Srinivas Veldurthi Date: Tue, 12 Dec 2017 16:37:20 -0800 Subject: [PATCH 08/10] Changes as per review. --- .../adaptor/database/DatabaseAdaptor.java | 56 +++++++++---------- .../adaptor/database/DatabaseAdaptorTest.java | 36 ++++++++++-- 2 files changed, 55 insertions(+), 37 deletions(-) diff --git a/src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java b/src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java index 5ba9f37..82e9bee 100644 --- a/src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java +++ b/src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java @@ -689,29 +689,27 @@ public static void main(String[] args) { AbstractAdaptor.main(new DatabaseAdaptor(), args); } - private Acl getAcl(Connection conn, ResultSet rs, String uniqueId) + private Acl getAcl(Connection conn, ResultSet contentRs, String uniqueId) throws SQLException { - if (aclSql != null || rs == null) { + if (aclSql != null || contentRs == null) { try (PreparedStatement stmt = getAclFromDb(conn, uniqueId); - ResultSet resSet = stmt.executeQuery()) { + ResultSet rs = stmt.executeQuery()) { log.finer("got acl"); - boolean hasResult = resSet.next(); + boolean hasResult = rs.next(); if (!hasResult) { // empty Acl ensures adaptor will mark this document as secure return Acl.EMPTY; + } else if (aclSql != null) { + return buildAcl(rs, aclPrincipalDelimiter, aclNamespace, + ResultSetNext.PROCESS_ALL_ROWS, Acl.EMPTY); } else { - if (aclSql != null) { - return buildAcl(resSet, aclPrincipalDelimiter, aclNamespace, - ResultSetNext.PROCESS_ALL_ROWS, Acl.EMPTY); - } else { - return buildAcl(resSet, aclPrincipalDelimiter, aclNamespace, - ResultSetNext.PROCESS_ALL_ROWS, null); - } + return buildAcl(rs, aclPrincipalDelimiter, aclNamespace, + ResultSetNext.PROCESS_ONE_ROW, null); } } } else { // Generate ACL if it came as part of result to singleDocContentSql. - return buildAcl(rs, aclPrincipalDelimiter, aclNamespace, + return buildAcl(contentRs, aclPrincipalDelimiter, aclNamespace, ResultSetNext.PROCESS_ONE_ROW, null); } } @@ -1130,27 +1128,23 @@ public Map isUserAuthorized(AuthnIdentity userIdentity, try (Connection conn = makeNewConnection()) { for (DocId id : ids) { log.log(Level.FINE, "about to get acl of doc {0}", id); - if (userIdentity == null) { - result.put(id, AuthzStatus.DENY); - } else if (userIdentity.getUser() == null) { + Acl acl = getAcl(conn, null, id.getUniqueId()); + if (acl == null) { + result.put(id, AuthzStatus.PERMIT); + } else if (userIdentity == null || userIdentity.getUser() == null) { result.put(id, AuthzStatus.DENY); } else { - Acl acl = getAcl(conn, null, id.getUniqueId()); - if (acl == null) { - result.put(id, AuthzStatus.PERMIT); - } else { - log.log(Level.INFO, "about to authorize {0} {1}", - new Object[]{user, userIdentity.getGroups()}); - List aclChain = Arrays.asList(acl); - log.log(Level.FINE, - "about to authorize user {0} for doc {1} and acl {2}", - new Object[]{user, id, acl}); - AuthzStatus decision = Acl.isAuthorized(userIdentity, aclChain); - log.log(Level.FINE, - "authorization decision {0} for user {1} and doc {2}", - new Object[]{decision, user, id}); - result.put(id, decision); - } + log.log(Level.INFO, "about to authorize {0} {1}", + new Object[] {user, userIdentity.getGroups()}); + List aclChain = Arrays.asList(acl); + log.log(Level.FINE, + "about to authorize user {0} for doc {1} and acl {2}", + new Object[] {user, id, acl}); + AuthzStatus decision = Acl.isAuthorized(userIdentity, aclChain); + log.log(Level.FINE, + "authorization decision {0} for user {1} and doc {2}", + new Object[] {decision, user, id}); + result.put(id, decision); } } } catch (SQLException ex) { diff --git a/test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java b/test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java index 0d433a9..0856919 100644 --- a/test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java +++ b/test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java @@ -2797,24 +2797,48 @@ private AuthnIdentity getAuthnIdentity(final UserPrincipal user) { @Test public void testAuthzAuthority_public() throws Exception { + executeUpdate("create table data(id int)"); + executeUpdate("insert into data(id) values (1)"); + Map moreEntries = new HashMap(); + moreEntries.put("db.uniqueKey", "id:int"); + moreEntries.put("db.singleDocContentSql", + "select * from data where id = ?"); // Required for validation, but not specific to this test. - executeUpdate("create table data(id int)"); moreEntries.put("db.modeOfOperation", "rowToText"); - moreEntries.put("db.uniqueKey", "id:int"); moreEntries.put("db.everyDocIdSql", "select id from data"); + + AuthzAuthority authority = getAuthzAuthority(moreEntries); + Map answers = authority.isUserAuthorized( + getAuthnIdentity(new UserPrincipal("alice")), + Arrays.asList(new DocId("1"))); + + HashMap golden = new HashMap<>(); + golden.put(new DocId("1"), AuthzStatus.PERMIT); + assertEquals(golden, answers); + } + + // add null identity public test + @Test + public void testAuthzAuthority_publicNullIdentity() throws Exception { + executeUpdate("create table data(id int)"); + executeUpdate("insert into data(id) values (1)"); + + Map moreEntries = new HashMap(); + moreEntries.put("db.uniqueKey", "id:int"); moreEntries.put("db.singleDocContentSql", "select * from data where id = ?"); - executeUpdate("insert into data(id) values (1),(2)"); + // Required for validation, but not specific to this test. + moreEntries.put("db.modeOfOperation", "rowToText"); + moreEntries.put("db.everyDocIdSql", "select id from data"); AuthzAuthority authority = getAuthzAuthority(moreEntries); Map answers = authority.isUserAuthorized( - getAuthnIdentity(new UserPrincipal("alice")), - Arrays.asList(new DocId("1"), new DocId("2"))); + null, + Arrays.asList(new DocId("1"))); HashMap golden = new HashMap<>(); golden.put(new DocId("1"), AuthzStatus.PERMIT); - golden.put(new DocId("2"), AuthzStatus.PERMIT); assertEquals(golden, answers); } From aed6dfaf03a2fa3d1d9c3c1883acab228e501639 Mon Sep 17 00:00:00 2001 From: Srinivas Veldurthi Date: Wed, 13 Dec 2017 18:57:15 -0800 Subject: [PATCH 09/10] Changes as per review: - Added test testAuthzAuthoritySingleDocContentSql_muiltiRowAcl to test PROCESS_ONE_ROW - Modified test testAuthzAuthorityAcl_permit to test aclSql with setContentSqlColumns - Other changes as per review --- .../adaptor/database/DatabaseAdaptor.java | 2 +- .../adaptor/database/DatabaseAdaptorTest.java | 54 ++++++++++++++----- 2 files changed, 43 insertions(+), 13 deletions(-) diff --git a/src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java b/src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java index 82e9bee..a39cede 100644 --- a/src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java +++ b/src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java @@ -1134,7 +1134,7 @@ public Map isUserAuthorized(AuthnIdentity userIdentity, } else if (userIdentity == null || userIdentity.getUser() == null) { result.put(id, AuthzStatus.DENY); } else { - log.log(Level.INFO, "about to authorize {0} {1}", + log.log(Level.FINE, "about to authorize {0} {1}", new Object[] {user, userIdentity.getGroups()}); List aclChain = Arrays.asList(acl); log.log(Level.FINE, diff --git a/test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java b/test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java index 0856919..9072679 100644 --- a/test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java +++ b/test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java @@ -448,7 +448,7 @@ public void testSingleDocSqlResultSetHasTwoRecords() throws Exception { + GsaSpecialColumns.GSA_PERMIT_USERS + "," + GsaSpecialColumns.GSA_DENY_GROUPS + "," + GsaSpecialColumns.GSA_DENY_USERS + ") values (" - + "2, 'test2', " + + "1, 'test2', " + "'pgroup3, pgroup4', 'puser3, puser4', " + "'dgroup3, dgroup4', 'duser3, duser4')"); Acl golden = new Acl.Builder() @@ -2799,6 +2799,7 @@ private AuthnIdentity getAuthnIdentity(final UserPrincipal user) { public void testAuthzAuthority_public() throws Exception { executeUpdate("create table data(id int)"); executeUpdate("insert into data(id) values (1)"); + executeUpdate("insert into data(id) values (2)"); Map moreEntries = new HashMap(); moreEntries.put("db.uniqueKey", "id:int"); @@ -2811,18 +2812,19 @@ public void testAuthzAuthority_public() throws Exception { AuthzAuthority authority = getAuthzAuthority(moreEntries); Map answers = authority.isUserAuthorized( getAuthnIdentity(new UserPrincipal("alice")), - Arrays.asList(new DocId("1"))); + Arrays.asList(new DocId("1"), new DocId("2"))); HashMap golden = new HashMap<>(); golden.put(new DocId("1"), AuthzStatus.PERMIT); + golden.put(new DocId("2"), AuthzStatus.PERMIT); assertEquals(golden, answers); } - // add null identity public test @Test public void testAuthzAuthority_publicNullIdentity() throws Exception { executeUpdate("create table data(id int)"); executeUpdate("insert into data(id) values (1)"); + executeUpdate("insert into data(id) values (2)"); Map moreEntries = new HashMap(); moreEntries.put("db.uniqueKey", "id:int"); @@ -2835,10 +2837,11 @@ public void testAuthzAuthority_publicNullIdentity() throws Exception { AuthzAuthority authority = getAuthzAuthority(moreEntries); Map answers = authority.isUserAuthorized( null, - Arrays.asList(new DocId("1"))); + Arrays.asList(new DocId("1"), new DocId("2"))); HashMap golden = new HashMap<>(); golden.put(new DocId("1"), AuthzStatus.PERMIT); + golden.put(new DocId("2"), AuthzStatus.PERMIT); assertEquals(golden, answers); } @@ -2920,27 +2923,28 @@ public void testAuthzAuthorityAcl_noResults() throws Exception { @Test public void testAuthzAuthorityAcl_permit() throws Exception { executeUpdate("create table acl(id integer, gsa_permit_users varchar(20))"); + executeUpdate("create table data(id integer, author varchar(20))"); executeUpdate("insert into acl(id, gsa_permit_users) values " + "(2, 'alice')"); Map moreEntries = new HashMap(); moreEntries.put("db.aclSql", "select * from acl where id = ?"); - // Required for validation, but not specific to this test. - executeUpdate("create table data(id int)"); + moreEntries.put("db.aclSqlParameters", "id"); + moreEntries.put("db.singleDocContentSqlParameters", "author"); moreEntries.put("db.modeOfOperation", "rowToText"); - moreEntries.put("db.uniqueKey", "id:int"); - moreEntries.put("db.everyDocIdSql", "select id from data"); + moreEntries.put("db.uniqueKey", "id:int,author:string"); + moreEntries.put("db.everyDocIdSql", "select id, author from data"); moreEntries.put("db.singleDocContentSql", - "select * from data where id = ?"); + "select * from data where author = ?"); AuthzAuthority authority = getAuthzAuthority(moreEntries); Map answers = authority.isUserAuthorized( getAuthnIdentity(new UserPrincipal("alice")), - Arrays.asList(new DocId("1"), new DocId("2"))); + Arrays.asList(new DocId("1/tom"), new DocId("2/alice"))); HashMap golden = new HashMap<>(); - golden.put(new DocId("1"), AuthzStatus.INDETERMINATE); - golden.put(new DocId("2"), AuthzStatus.PERMIT); + golden.put(new DocId("1/tom"), AuthzStatus.INDETERMINATE); + golden.put(new DocId("2/alice"), AuthzStatus.PERMIT); assertEquals(golden, answers); } @@ -3026,6 +3030,32 @@ public void testAuthzAuthoritySingleDocContentSql_permit() throws Exception { assertEquals(golden, answers); } + @Test + public void testAuthzAuthoritySingleDocContentSql_muiltiRowAcl() + throws Exception { + executeUpdate("create table data(id integer, " + + "gsa_permit_users varchar(20), gsa_deny_users varchar(20))"); + executeUpdate("insert into data(id, gsa_deny_users) values (1, 'tom')"); + executeUpdate("insert into data(id, gsa_permit_users) values (1, 'alice')"); + + Map moreEntries = new HashMap(); + moreEntries.put("db.modeOfOperation", "rowToText"); + moreEntries.put("db.uniqueKey", "id:int"); + moreEntries.put("db.singleDocContentSql", + "select * from data where id = ?"); + // Required for validation, but not specific to this test. + moreEntries.put("db.everyDocIdSql", "select id from data"); + + AuthzAuthority authority = getAuthzAuthority(moreEntries); + Map answers = authority.isUserAuthorized( + getAuthnIdentity(new UserPrincipal("alice")), + Arrays.asList(new DocId("1"))); + + HashMap golden = new HashMap<>(); + golden.put(new DocId("1"), AuthzStatus.DENY); + assertEquals(golden, answers); + } + @Test public void testAuthzAuthoritySingleDocContentSql_noAclColumns() throws Exception { From 61b6b9f1c467eedd533df69d6498043d52aea2e6 Mon Sep 17 00:00:00 2001 From: Srinivas Veldurthi Date: Thu, 14 Dec 2017 12:06:28 -0800 Subject: [PATCH 10/10] Changes as per review. --- .../adaptor/database/DatabaseAdaptorTest.java | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java b/test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java index 9072679..3b881bc 100644 --- a/test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java +++ b/test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java @@ -2923,7 +2923,6 @@ public void testAuthzAuthorityAcl_noResults() throws Exception { @Test public void testAuthzAuthorityAcl_permit() throws Exception { executeUpdate("create table acl(id integer, gsa_permit_users varchar(20))"); - executeUpdate("create table data(id integer, author varchar(20))"); executeUpdate("insert into acl(id, gsa_permit_users) values " + "(2, 'alice')"); @@ -2931,6 +2930,8 @@ public void testAuthzAuthorityAcl_permit() throws Exception { moreEntries.put("db.aclSql", "select * from acl where id = ?"); moreEntries.put("db.aclSqlParameters", "id"); moreEntries.put("db.singleDocContentSqlParameters", "author"); + // Required for validation, but not specific to this test. + executeUpdate("create table data(id integer, author varchar(20))"); moreEntries.put("db.modeOfOperation", "rowToText"); moreEntries.put("db.uniqueKey", "id:int,author:string"); moreEntries.put("db.everyDocIdSql", "select id, author from data"); @@ -2982,11 +2983,11 @@ public void testAuthzAuthoritySingleDocContentSql_noResults() executeUpdate("create table data(id integer)"); Map moreEntries = new HashMap(); - moreEntries.put("db.modeOfOperation", "rowToText"); moreEntries.put("db.uniqueKey", "id:int"); moreEntries.put("db.singleDocContentSql", "select * from data where id = ?"); // Required for validation, but not specific to this test. + moreEntries.put("db.modeOfOperation", "rowToText"); moreEntries.put("db.everyDocIdSql", "select id from data"); AuthzAuthority authority = getAuthzAuthority(moreEntries); @@ -3012,11 +3013,11 @@ public void testAuthzAuthoritySingleDocContentSql_permit() throws Exception { Map moreEntries = new HashMap(); moreEntries.put("db.aclSqlParameters", "author"); moreEntries.put("db.singleDocContentSqlParameters", "id, author"); - moreEntries.put("db.modeOfOperation", "rowToText"); moreEntries.put("db.uniqueKey", "id:int,author:string"); moreEntries.put("db.singleDocContentSql", "select * from data where id = ? and author = ?"); // Required for validation, but not specific to this test. + moreEntries.put("db.modeOfOperation", "rowToText"); moreEntries.put("db.everyDocIdSql", "select id, author from data"); AuthzAuthority authority = getAuthzAuthority(moreEntries); @@ -3039,11 +3040,11 @@ public void testAuthzAuthoritySingleDocContentSql_muiltiRowAcl() executeUpdate("insert into data(id, gsa_permit_users) values (1, 'alice')"); Map moreEntries = new HashMap(); - moreEntries.put("db.modeOfOperation", "rowToText"); moreEntries.put("db.uniqueKey", "id:int"); moreEntries.put("db.singleDocContentSql", "select * from data where id = ?"); // Required for validation, but not specific to this test. + moreEntries.put("db.modeOfOperation", "rowToText"); moreEntries.put("db.everyDocIdSql", "select id from data"); AuthzAuthority authority = getAuthzAuthority(moreEntries); @@ -3063,11 +3064,11 @@ public void testAuthzAuthoritySingleDocContentSql_noAclColumns() executeUpdate("insert into data(id) values (2)"); Map moreEntries = new HashMap(); - moreEntries.put("db.modeOfOperation", "rowToText"); moreEntries.put("db.uniqueKey", "id:int"); moreEntries.put("db.singleDocContentSql", "select * from data where id = ?"); // Required for validation, but not specific to this test. + moreEntries.put("db.modeOfOperation", "rowToText"); moreEntries.put("db.everyDocIdSql", "select id from data"); AuthzAuthority authority = getAuthzAuthority(moreEntries); @@ -3089,11 +3090,11 @@ public void testAuthzAuthoritySingleDocContentSql_noAclEntries() executeUpdate("insert into data(id) values (2)"); Map moreEntries = new HashMap(); - moreEntries.put("db.modeOfOperation", "rowToText"); moreEntries.put("db.uniqueKey", "id:int"); moreEntries.put("db.singleDocContentSql", "select * from data where id = ?"); // Required for validation, but not specific to this test. + moreEntries.put("db.modeOfOperation", "rowToText"); moreEntries.put("db.everyDocIdSql", "select id from data"); AuthzAuthority authority = getAuthzAuthority(moreEntries);