DB Fix bug#67471678. Include ACLs in the db.singleDocContentSql by srinicodebytes · Pull Request #110 · googlegsa/database

srinicodebytes · 2017-11-28T22:47:18Z

No description provided.

googlebot · 2017-11-28T22:47:21Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

PapaJoltOmega

Thank you.

PapaJoltOmega · 2017-11-28T23:13:28Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

-      aclPrincipalDelimiter = cfg.getValue("db.aclPrincipalDelimiter");
-      log.config("aclPrincipalDelimiter: '" + aclPrincipalDelimiter + "'");
    }
+    aclPrincipalDelimiter = cfg.getValue("db.aclPrincipalDelimiter");


add comment
// we might have delimiter and no aclSql as ACLs might come from singleDocContentSql rather than aclSql

PapaJoltOmega · 2017-11-28T23:21:09Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

      if (aclSql != null) {
        resp.setAcl(getAcl(conn, id.getUniqueId()));
+      } else {
+        resp.setAcl(buildAcl(rs, aclPrincipalDelimiter, aclNamespace,


add comment
// see if ACL came as part of result to singleDocContentSql query

PapaJoltOmega · 2017-11-28T23:23:31Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

        denyGroups.addAll(getGroupPrincipalsFromResultSet(rs,
            GsaSpecialColumns.GSA_DENY_GROUPS, delim, namespace));
      }
+      if (rsNext == ResultSetNext.STOP_NEXT) {


Please help me understand the constraint that ACL coming from singleDocContentSql imposes that requires this statement.

We shouldn't be calling rs.next() when processing ACLs from singleDocContentSql, or else we would be moving to next or end of result set and wouldn't be able to get content from result set.

PapaJoltOmega · 2017-11-28T23:24:37Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

      String uniqueId) throws SQLException {
    PreparedStatement st = conn.prepareStatement(singleDocContentSql);
-    uniqueKey.setContentSqlValues(st, uniqueId);  
+    uniqueKey.setContentSqlValues(st, uniqueId);


has something changed on this line? github shows it as changed.

Looks like I removed the whitespace at the end. Restored it.

PapaJoltOmega · 2017-11-28T23:25:09Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

+       st = conn.prepareStatement(singleDocContentSql);
+    } else {
+       st = conn.prepareStatement(aclSql);
+    }


indent looks 1off

Thx, fixed it.

PapaJoltOmega · 2017-11-28T23:26:46Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

+    CONTINUE_NEXT,
+    STOP_NEXT,
+  }
+


How about
PROCESS_ALL_RESULTS
PROCESS_ONE_RESULT

Am I understanding the semantics?

Thx, modified these.

wiarlawd · 2017-11-28T23:20:47Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

  }

  @VisibleForTesting
  static Acl buildAcl(ResultSet rs, String delim, String namespace)


Who calls this overload?

Only tests currently. Moved this to tests.

wiarlawd · 2017-11-28T23:22:12Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

      if (aclSql != null) {
        resp.setAcl(getAcl(conn, id.getUniqueId()));
+      } else {
+        resp.setAcl(buildAcl(rs, aclPrincipalDelimiter, aclNamespace,


This seems like it's going to set the empty ACL (an explicitly constructed one, rather than Acl.EMPTY) on a document that should be public.

Thx, fixed it by returning Acl.EMPTY when no acl info in the result

wiarlawd · 2017-11-28T23:23:15Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

      String uniqueId) throws SQLException {
    PreparedStatement st = conn.prepareStatement(singleDocContentSql);
-    uniqueKey.setContentSqlValues(st, uniqueId);  
+    uniqueKey.setContentSqlValues(st, uniqueId);


Restore trailing whitespace on this unmodified line.

wiarlawd · 2017-11-28T23:28:21Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

-    PreparedStatement st = conn.prepareStatement(aclSql);
+    PreparedStatement st;
+    if (aclSql == null) {
+       st = conn.prepareStatement(singleDocContentSql);


It seems worth a comment here that this only happens when called from isUserAuthorized. Also, we will execute db.singleDocContentSql twice in that case. I suppose that's inevitable, but did you think about trying to avoid that?

wiarlawd · 2017-11-28T23:28:38Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

  }

+  enum ResultSetNext {
+    CONTINUE_NEXT,


What do you think about ALL_ROWS, ONE_ROW?

Combining your and PJ's suggestion, modified to PROCESS_ALL_ROWS , PROCESS_ONE_ROW.

wiarlawd · 2017-11-29T00:13:38Z

test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java

+        + GsaSpecialColumns.GSA_PERMIT_USERS + " varchar(20),"
+        + GsaSpecialColumns.GSA_DENY_GROUPS + " varchar(20),"
+        + GsaSpecialColumns.GSA_DENY_USERS + " varchar(20))");
+    executeUpdate("insert into data("


This test should minimally have 2 rows as a test of STOP_NEXT.

Thx, added another row

wiarlawd · 2017-11-29T00:15:43Z

test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java

+        .build();
+    ResultSet rs = executeQuery("select * from data");
+    rs.next();
+    Acl acl = DatabaseAdaptor.buildAcl(rs, ",", DEFAULT_NAMESPACE,


There are only six tests that use buildAcl instead of getDocContent. I think we should convert all of them, perhaps in a separate CL. Perhaps minimally we could use getDocContent for this new test. We have more logic around db.aclSql now, and I don't like bypassing that in the tests.

Agree. Modified this test to call getDocContent instead of buildAcl.

wiarlawd · 2017-11-29T21:04:13Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

+
+    if (!hasPermitUsers && !hasDenyUsers
+        && !hasPermitGroups && !hasDenyGroups) {
+      return Acl.EMPTY;


Whether implicit or explicit, the empty ACL is wrong in the case of getting ACLs from db.singleDocContentSql. I think we could just add another argument for the default ACL to use here, either null or Acl.EMPTY. These values are paired with the ResultSetNext enum, so we could conceivably collapse them into a single argument, but that feels a little obfuscated.

Since this is a bug, albeit newly introduced, you should add a test for it.

Done. Added test for it.

wiarlawd · 2017-11-29T21:07:23Z

test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java

        GsaSpecialColumns.GSA_TIMESTAMP));
  }

+  private Acl buildAcl(ResultSet rs, String delim, String namespace)


I would have left this alone with a TODO, since we're planning to replace it entirely, or you could have used executeQueryAndNext instead of executeQuery. Just mentioning other ideas for posterity, since we're planning to sweep this away anyway.

Agree. Keeping as is for now.

wiarlawd · 2017-11-29T21:11:39Z

test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java

+
+    Map<String, String> configEntries = new HashMap<String, String>();
+    configEntries.put("db.uniqueKey", "id:int");
+    configEntries.put("db.everyDocIdSql", "select id from data");


Follow the boilerplate and put db.everyDocIdSql after the "Required for validation, ..." comment.

wiarlawd · 2017-11-29T21:12:41Z

test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java

+    RecordingResponse response = new RecordingResponse();
+    adaptor.getDocContent(request, response);
+
+    Acl acl = response.getAcl();


It's more consistent now, but if we're planning to use this as an exemplar, I think it's nicer to just inline this value in the assert.

wiarlawd · 2017-11-29T21:15:31Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

-    PreparedStatement st = conn.prepareStatement(aclSql);
+    PreparedStatement st;
+    if (aclSql == null) {
+      st = conn.prepareStatement(singleDocContentSql);


Forgotten comment here?

Added comment. Done.

wiarlawd · 2017-11-29T21:19:18Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

    PreparedStatement st = conn.prepareStatement(singleDocContentSql);
    uniqueKey.setContentSqlValues(st, uniqueId);  
+    if (aclSql == null) {
+      uniqueKey.setAclSqlValues(st, uniqueId);


This seems wrong. The db.aclSqlParameters columns are specific to db.aclSql, and may not be the same in db.singleDocContentSql. Did something fail without this?

Since this is wrong, too, maybe add a test where doing this would fail, too.

Deleted the newly added lines, as these are not required. And no test required.

wiarlawd · 2017-11-29T21:22:28Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

  private PreparedStatement getAclFromDb(Connection conn,
      String uniqueId) throws SQLException {
-    PreparedStatement st = conn.prepareStatement(aclSql);
+    PreparedStatement st;


Maybe simpler:

String sql = (aclSql == null) ? singleDocContentSql : aclSql; PreparedStatement st = conn.prepareStatement(sql);

It seems a little redundant that we run this check before calling this function on line 681. I don't, at this time, have a better suggestion. Let's keep an eye out to see if there is a better way to keep the functions orthogonality composable. The current way is fine, but I will keep an eye out to see if there is something better. It is also possible that you are planning to revisit/refactor in follow up. Thank you

Yes, thx. Done.

wiarlawd

Nits

wiarlawd · 2017-11-30T01:10:33Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

        denyGroups.addAll(getGroupPrincipalsFromResultSet(rs,
            GsaSpecialColumns.GSA_DENY_GROUPS, delim, namespace));
      }
+      if (rsNext == ResultSetNext.PROCESS_ONE_ROW) {


Or maybe just:

} while (rsNext == ResultSetNext.PROCESS_ALL_ROWS && rs.next());

wiarlawd · 2017-11-30T21:22:50Z

test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java

+    RecordingResponse response = new RecordingResponse();
+    adaptor.getDocContent(request, response);
+
+    assertEquals(golden, response.getAcl());


The use of golden here seems silly. assertNull(response.getAcl()) is more clear.

wiarlawd · 2017-11-30T21:29:20Z

test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java

+
+    Map<String, String> configEntries = new HashMap<String, String>();
+    configEntries.put("db.uniqueKey", "id:int");
+    configEntries.put("db.everyDocIdSql", "select id from data");


Missed comment here: put this at the end of the configEntries, after a comment:

// Required for validation, but not specific to this test.

wiarlawd · 2017-11-30T21:30:46Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

  private PreparedStatement getAclFromDb(Connection conn,
      String uniqueId) throws SQLException {
-    PreparedStatement st = conn.prepareStatement(aclSql);
+    // aclSql will be null when called from isUserAuthorized.


s/will/will only/

PapaJoltOmega · 2017-11-29T21:42:12Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

  private PreparedStatement getAclFromDb(Connection conn,
      String uniqueId) throws SQLException {
-    PreparedStatement st = conn.prepareStatement(aclSql);
+    PreparedStatement st;


It seems a little redundant that we run this check before calling this function on line 681. I don't, at this time, have a better suggestion. Let's keep an eye out to see if there is a better way to keep the functions orthogonality composable. The current way is fine, but I will keep an eye out to see if there is something better. It is also possible that you are planning to revisit/refactor in follow up. Thank you

PapaJoltOmega · 2017-11-30T21:39:18Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

+      boolean hasResult = rs.next();
+      if (!hasResult) {
+        // empty Acl ensures adaptor will mark this document as secure
+        return Acl.EMPTY;


Why move this code from buildAcl?

Because the other call to buildAcl from getDocContent using the content SQL doesn't ever want to call rs.next(). We could check rsNext at the top and bottom of the loop in buildAcl, which is messier and another double-check, but it's in one method.

PapaJoltOmega · 2017-11-30T21:43:25Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

@@ -697,18 +703,20 @@ private Acl getAcl(Connection conn, String uniqueId) throws SQLException {
    try (PreparedStatement stmt = getAclFromDb(conn, uniqueId);


Perhaps rename getAclFromDb to runAclSqlStatement (to disambiguate that it is not running content sql)?

As I mentioned, I didn't entirely like the structure of getAclFromDb, but it has parallel methods for getDocFromDb and getStreamFromDb. All three methods just create a PreparedStatement, so a basic rename to prepare{Acl,Doc,Stream}Statement seems reasonable, but feels like a separate CL. Maybe we can finish this one and start a new pull request to massage the structure?

Also, these methods have functional utility for try-with-resources: we want to call prepareStatement and executeQuery but do something in between. So we either need these helper methods, or we need two nested try-with-resources.

Ah, yes, that changed here afe59e4#diff-dd03438283161f00e652d4769de767bf

I agree that some structure/renaming changes are fitting.

I have made a client and am reading DatabaseAdaptor.java file to re-acquaint myself with it. I plan to read it on Monday and make some suggestions then.

Thank you

PapaJoltOmega · 2017-11-30T21:45:05Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

@@ -678,6 +680,10 @@ public void getDocContent(Request req, Response resp) throws IOException {
      // Generate Acl if aclSql is provided.
      if (aclSql != null) {
        resp.setAcl(getAcl(conn, id.getUniqueId()));


Perhaps rename getAcl to getAclFromAclSql (to disambiguate that it is not running the content sql)?

This call site for getAcl isn't running the content SQL, but when called from isUserAuthorized it is. I don't think that exposing the conditional from getAclFromDb in isUserAuthorized feels right. Perhaps better would be to bury this check for aclSql != null in getAcl as well, so that both call sites just always call getAcl, which figures out how to do the work.

PapaJoltOmega · 2017-11-30T21:47:55Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

+
+    if (!hasPermitUsers && !hasDenyUsers
+        && !hasPermitGroups && !hasDenyGroups) {
+      return defaultAcl;


why do we need default acl parameter?

If we have aclSql but no ACL entries, we want an empty ACL to mark the document secure. But if we have no aclSql and no ACL entries, then we want a null ACL to mark the document public.

wiarlawd

Replies to PJ.

wiarlawd · 2017-11-30T23:21:22Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

@@ -678,6 +680,10 @@ public void getDocContent(Request req, Response resp) throws IOException {
      // Generate Acl if aclSql is provided.
      if (aclSql != null) {
        resp.setAcl(getAcl(conn, id.getUniqueId()));


This call site for getAcl isn't running the content SQL, but when called from isUserAuthorized it is. I don't think that exposing the conditional from getAclFromDb in isUserAuthorized feels right. Perhaps better would be to bury this check for aclSql != null in getAcl as well, so that both call sites just always call getAcl, which figures out how to do the work.

wiarlawd · 2017-11-30T23:28:24Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

@@ -697,18 +703,20 @@ private Acl getAcl(Connection conn, String uniqueId) throws SQLException {
    try (PreparedStatement stmt = getAclFromDb(conn, uniqueId);


As I mentioned, I didn't entirely like the structure of getAclFromDb, but it has parallel methods for getDocFromDb and getStreamFromDb. All three methods just create a PreparedStatement, so a basic rename to prepare{Acl,Doc,Stream}Statement seems reasonable, but feels like a separate CL. Maybe we can finish this one and start a new pull request to massage the structure?

Also, these methods have functional utility for try-with-resources: we want to call prepareStatement and executeQuery but do something in between. So we either need these helper methods, or we need two nested try-with-resources.

wiarlawd · 2017-11-30T23:30:24Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

+      boolean hasResult = rs.next();
+      if (!hasResult) {
+        // empty Acl ensures adaptor will mark this document as secure
+        return Acl.EMPTY;


Because the other call to buildAcl from getDocContent using the content SQL doesn't ever want to call rs.next(). We could check rsNext at the top and bottom of the loop in buildAcl, which is messier and another double-check, but it's in one method.

wiarlawd · 2017-11-30T23:31:23Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

+
+    if (!hasPermitUsers && !hasDenyUsers
+        && !hasPermitGroups && !hasDenyGroups) {
+      return defaultAcl;


If we have aclSql but no ACL entries, we want an empty ACL to mark the document secure. But if we have no aclSql and no ACL entries, then we want a null ACL to mark the document public.

PapaJoltOmega

Thanks for your thoughtful review John.

PapaJoltOmega · 2017-12-01T03:22:25Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

@@ -697,18 +703,20 @@ private Acl getAcl(Connection conn, String uniqueId) throws SQLException {
    try (PreparedStatement stmt = getAclFromDb(conn, uniqueId);


Ah, yes, that changed here afe59e4#diff-dd03438283161f00e652d4769de767bf

I agree that some structure/renaming changes are fitting.

I have made a client and am reading DatabaseAdaptor.java file to re-acquaint myself with it. I plan to read it on Monday and make some suggestions then.

Thank you

wiarlawd · 2017-12-01T09:10:58Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

      String uniqueId) throws SQLException {
-    PreparedStatement st = conn.prepareStatement(aclSql);
+    // aclSql will only be null when called from isUserAuthorized.
+    String sql = (aclSql == null) ? singleDocContentSql : aclSql;


There are two bugs here. We'll have to go back to the if statement, because as I suggested elsewhere, the UniqueKey method depends on the query being executed and not the data being read from the result. So if we use singleDocContentSql here, we need to call setContentSqlValues rather than setAclSqlValues. We could just call getDocFromDb in that case, but the logging would be odd, and I think it's worth being clear that we're executing this statement to fetch the ACL in this case. We need some tests for mismatches in singleDocContentSqlParameters and aclSqlParameters (note that everyDocIdSql must return all the columns needed for both, but it could, for example, return 2 columns, and have each of the other queries use a different column; to check for errors I think something like a varchar and an integer column might do the trick, or one query could use both columns and the other just one).

Second, we're actually using AllPublic rather than AccessChecker when aclSql is null (this is set in init). At the moment, aclSql really can't be null here, but that's a bug. We need to always check for ACLs in the AuthzAuthority, but AccessChecker needs some work. We need to deal with the two TODOs, and the fact that getAcl can return null (I think the latter is the key to the former: a null ACL makes a null identity acceptable, otherwise we need the identity to compare to the ACL). Obviously, we need some tests here as well.

As discussed, changed the code and added tests.

- added processAclQuery() - removed getAclFromDb() Added tests.

wiarlawd · 2017-12-07T23:56:08Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

+    }
+  }
+
+  private Acl processAclQuery(PreparedStatement st, Acl defaultAcl)


This was a surprising choice. Why did you extract this method instead of leaving getAclFromDb? The latter is a parallel method with two siblings, so instead of that consistency, you added this one-off method, and I don't think it isolates concerns very well, or reads any cleaner (it checks aclSql for null 3 times in getAcl, for example).

Was trying to make it cleaner. Can't avoid aclSql check multiple times though.
Using getAclFromDb(), the code would be

private Acl getAcl(Connection conn, ResultSet rs, String uniqueId) throws SQLException { if (aclSql != null || rs == 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 { return buildAcl(resSet, aclPrincipalDelimiter, aclNamespace, ResultSetNext.PROCESS_ALL_ROWS, Acl.EMPTY); } } } else { // Generate ACL if it came as part of result to singleDocContentSql. return buildAcl(rs, aclPrincipalDelimiter, aclNamespace, ResultSetNext.PROCESS_ONE_ROW, null); } }

Restored it back.

wiarlawd · 2017-12-07T23:57:24Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

    }

-    if (aclSql == null) {
-      context.setAuthzAuthority(new AllPublic());


AllPublic is unused now.

Yes missed it, removed it.

wiarlawd · 2017-12-08T00:02:24Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

        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());


You missed some changes here. Try running this code live by fetching public docs directly from the adaptor.

wiarlawd · 2017-12-08T00:04:13Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

+    try (ResultSet resSet = st.executeQuery()) {
      log.finer("got acl");
-      boolean hasResult = rs.next();
+      boolean hasResult = resSet.next();


Why s/rs/resSet/? It's a non-standard abbreviation, and rs in a database connector seems perfectly clear.

Unintended change in this new method, and carried over from trying to make it two nested try-with-resources blocks.

wiarlawd · 2017-12-08T00:04:52Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

      if (!hasResult) {
-        // empty Acl ensures adaptor will mark this document as secure
-        return Acl.EMPTY;
+        return defaultAcl;


This is a behavioral change, albeit a minor one. Why make this change?

When calling from isUserAuthorized and aclSql is null and has no results this needs to return null. Hence passing the defaultAcl and returning it.

In fact, it needs to not return null. I thought it was just a nuisance to return null (it will execute the content SQL rather than just skipping this search result), but it's actually a security hole. If the row doesn't exist, then it makes no sense to return a PERMIT for it.

wiarlawd

Partial review. I didn't finish looking at the tests.

wiarlawd · 2017-12-12T08:41:07Z

test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java

    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)");


This syntax is H2-specific and we eliminated it when porting the unit tests to other databases. What's the point of having two rows here, instead of just one? Also, why separate this from the CREATE TABLE? Next, note that this "public" test specifies a non-null identity. Finally, both the executeUpdate calls and the db.uniqueKey and db.singleDocContentSql properties are now specific to this test. In that case, we put the executeUpdate calls first, then create the moreEntries hash map and put db.singleDocContentSql into it.

Only inserted data to existing test. Made changes as per comments.

wiarlawd · 2017-12-12T08:42:47Z

test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java

  @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), "


What's the s/loginid/author/g for?

Instead of id and loginid as unique keys, id and author sounds better. Hence changed it.

wiarlawd · 2017-12-12T09:01:00Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

+                ResultSetNext.PROCESS_ALL_ROWS, Acl.EMPTY);
+          } else {
+            return buildAcl(resSet, aclPrincipalDelimiter, aclNamespace,
+                ResultSetNext.PROCESS_ALL_ROWS, null);


This should be PROCESS_ONE_ROW. We're back to 3 checks for aclSql == null, which I think suggests we need some refactoring. Maybe some kind of ACL strategy pattern, where we construct an appropriate object once for each scenario?

Yes, it should be PROCESS_ONE_ROW, thx. Done.
Yes, agree, refactoring to reduce the null check for aclSql will look cleaner.

wiarlawd · 2017-12-12T09:02:17Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

+      throws SQLException {
+    if (aclSql != null || rs == null) {
+      try (PreparedStatement stmt = getAclFromDb(conn, uniqueId);
+          ResultSet resSet = stmt.executeQuery()) {


resSet is a terrible name. It gives no indication of how it's different from rs, is uses a short name for a parameter and a longer name for a local variable, and it uses a non-standard abbreviation. Better would be something like s/rs/contentRs/g and then s/resSet/rs/g.

wiarlawd · 2017-12-12T09:02:42Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

+        if (!hasResult) {
+          // empty Acl ensures adaptor will mark this document as secure
+          return Acl.EMPTY;
+        } else {


wiarlawd · 2017-12-12T09:04:38Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

-              "authorization decision {0} for user {1} and doc {2}",
-              new Object[]{decision, user, id});
-          result.put(id, decision);
+          if (userIdentity == null) {


What did you hope to gain by just moving this check inside the loop? Did you try this live?

As discussed, changed to get and check Acl first.

wiarlawd · 2017-12-12T09:05:04Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

+            if (acl == null) {
+              result.put(id, AuthzStatus.PERMIT);
+            } else {
+              log.log(Level.INFO, "about to authorize {0} {1}",


What this meant to stay here?

Existing log statement moved.

wiarlawd

This looks right to me. Since we had to work so hard to get the parameters right, let's be sure to capture that in the tests. Some missing tests that I found:

PROCESS_ONE_ROW for both calls (testSingleDocSqlResultSetHasTwoRecords is not doing the job for the contentRs case because the query only returns one row)

Previously untested behaviors that we changed the code paths for, including:

PROCESS_ALL_ROWS (there are apparently no tests with multiple rows returned by aclSql, despite the existing code to process such)
Acl.EMPTY (ditto: the implicit empty ACL the code used to return for what's now the Acl.EMPTY defaultAcl parameter is untested: an aclSql with no ACL columns)
Combining aclSql with setContentSqlColumns (this was never happening, but I think it's reasonable to augment testAuthzAuthorityAcl_permit with your author field and swizzle things so that this combo would fail there)

I don't think this needs to be 5 new tests, maybe just some config or test data variations in existing tests could cover these.

wiarlawd · 2017-12-13T01:57:10Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

+          } else if (userIdentity == null || userIdentity.getUser() == null) {
+            result.put(id, AuthzStatus.DENY);
+          } else {
+            log.log(Level.INFO, "about to authorize {0} {1}",


s/INFO/FINE/, since it's executed now for every document in the authZ request.

wiarlawd · 2017-12-13T02:05:08Z

test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java

-        getAuthnIdentity(new UserPrincipal("alice")),
-        Arrays.asList(new DocId("1"), new DocId("2")));
+        null,
+        Arrays.asList(new DocId("1")));


Oh, I see now, there were two docs in this test. I somehow missed that. I'm OK with restoring that. It seems reasonable to maintain that and test multiple IDs.

Restored to maintain testing multiple IDs.

* Remove persistent if statements, especially checking whether aclSql is null. * Make it clear that there are two choices, whether aclSql is null, and whether we are coming from getDocContent or isUserAuthorized. * Make most of the config decisions at init-time. Notes: * Indentation is incorrect to reduce diffs. * Additional refactoring can eliminate the additional arguments for buildAcl and getAclFromDb by moving them inside of AclHandler.

- Added test testAuthzAuthoritySingleDocContentSql_muiltiRowAcl to test PROCESS_ONE_ROW - Modified test testAuthzAuthorityAcl_permit to test aclSql with setContentSqlColumns - Other changes as per review

wiarlawd

Just some organizational nits in the tests.

wiarlawd · 2017-12-14T08:42:48Z

test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java


    Map<String, String> moreEntries = new HashMap<String, String>();
    moreEntries.put("db.aclSql", "select * from acl where id = ?");
-    // Required for validation, but not specific to this test.


Why did you remove this, or treat the data table, modeOfOperation, or everyDocIdSql as specific to this test? I can see moving db.uniqueKey and db.everyDocIdSql up (though I probably wouldn't have move the latter, since it's never executed).

It looked odd to have "create table data.." after "moreEntries.put("db.aclSql"..", hence moved it to top. However, I see that the other tests have this pattern. Restored. it. Thx.

wiarlawd · 2017-12-14T08:43:28Z

test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java

+    executeUpdate("create table data(id integer)");
+
+    Map<String, String> moreEntries = new HashMap<String, String>();
+    moreEntries.put("db.modeOfOperation", "rowToText");


Move to "Required for validation"

wiarlawd · 2017-12-14T08:44:14Z

test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java

+    Map<String, String> moreEntries = new HashMap<String, String>();
+    moreEntries.put("db.aclSqlParameters", "author");
+    moreEntries.put("db.singleDocContentSqlParameters", "id, author");
+    moreEntries.put("db.modeOfOperation", "rowToText");


Move to "Required for validation"

wiarlawd · 2017-12-14T08:45:53Z

test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java

  @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))");


This shouldn't be here at all, but elsewhere the pattern is "create data, insert data, create acl, insert acl".

wiarlawd · 2017-12-14T08:46:18Z

test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java

+    executeUpdate("insert into data(id, gsa_permit_users) values (1, 'alice')");
+
+    Map<String, String> moreEntries = new HashMap<String, String>();
+    moreEntries.put("db.modeOfOperation", "rowToText");


You see the pattern.

Yes, thx. Moved it in all the new tests.

wiarlawd

Looks good. We're waiting a bit for PJ, and in the meantime, please do a "git merge master" locally and resolve the conflicts and push the commit here.

* Remove persistent if statements, especially checking whether aclSql is null. * Make it clear that there are two choices, whether aclSql is null, and whether we are coming from getDocContent or isUserAuthorized. * Make most of the config decisions at init-time. Notes: * Indentation is incorrect to reduce diffs. * Additional refactoring can eliminate the additional arguments for buildAcl and getAclFromDb by moving them inside of AclHandler.

DB Fix bug#67471678. Include ACLs in the db.singleDocContentSql

f28dade

srinicodebytes requested a review from wiarlawd November 28, 2017 22:48

PapaJoltOmega reviewed Nov 28, 2017

View reviewed changes

wiarlawd reviewed Nov 29, 2017

View reviewed changes

Changes as per review.

a8df1f1

wiarlawd suggested changes Nov 29, 2017

View reviewed changes

Changes as suggested.

16f8673

wiarlawd reviewed Nov 30, 2017

View reviewed changes

PapaJoltOmega reviewed Nov 30, 2017

View reviewed changes

wiarlawd reviewed Nov 30, 2017

View reviewed changes

Changes as per review.

ec20849

PapaJoltOmega reviewed Dec 1, 2017

View reviewed changes

wiarlawd suggested changes Dec 1, 2017

View reviewed changes

Restructured code around getAcl()

bdbe6c5

- added processAclQuery() - removed getAclFromDb() Added tests.

wiarlawd reviewed Dec 8, 2017

View reviewed changes

srinicodebytes added 2 commits December 7, 2017 17:09

Restored getAclFromDb(). Removed AllPublic.

9f2edd6

Modified getAcl(), isUserAuthorized() and added tests.

ad41538

wiarlawd suggested changes Dec 12, 2017

View reviewed changes

Changes as per review.

e07e129

wiarlawd reviewed Dec 13, 2017

View reviewed changes

Changes as per review:

aed6dfa

- Added test testAuthzAuthoritySingleDocContentSql_muiltiRowAcl to test PROCESS_ONE_ROW - Modified test testAuthzAuthorityAcl_permit to test aclSql with setContentSqlColumns - Other changes as per review

wiarlawd reviewed Dec 14, 2017

View reviewed changes

Changes as per review.

61b6b9f

wiarlawd reviewed Dec 15, 2017

View reviewed changes

Merge branch 'master' into sri-acls

bcf643c

		@@ -697,18 +703,20 @@ private Acl getAcl(Connection conn, String uniqueId) throws SQLException {
		try (PreparedStatement stmt = getAclFromDb(conn, uniqueId);

Conversation

srinicodebytes commented Nov 28, 2017

Uh oh!

googlebot commented Nov 28, 2017

Uh oh!

PapaJoltOmega left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srinicodebytes Nov 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srinicodebytes Nov 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srinicodebytes Nov 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srinicodebytes Nov 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srinicodebytes Nov 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srinicodebytes Nov 29, 2017 •

edited

Loading

srinicodebytes Nov 29, 2017 •

edited

Loading

srinicodebytes Nov 29, 2017 •

edited

Loading

srinicodebytes Nov 29, 2017 •

edited

Loading

srinicodebytes Nov 30, 2017 •

edited

Loading

srinicodebytes Nov 30, 2017 •

edited

Loading

srinicodebytes Nov 30, 2017 •

edited

Loading