From bb734ec0cfefe6f4c89d1eb1b75b40359c664831 Mon Sep 17 00:00:00 2001 From: John Lacey Date: Wed, 13 Dec 2017 00:23:26 -0800 Subject: [PATCH] A draft refactoring for ACLs based on #110. * 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. --- .../adaptor/database/DatabaseAdaptor.java | 91 ++++++++++++++----- .../adaptor/database/UniqueKey.java | 2 +- 2 files changed, 70 insertions(+), 23 deletions(-) diff --git a/src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java b/src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java index a39cede..af9f71d 100644 --- a/src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java +++ b/src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java @@ -125,6 +125,7 @@ private static boolean isNullOrEmptyString(String str) { private String user; private String password; private UniqueKey uniqueKey; + private AclHandler aclHandler; private String everyDocIdSql; private String singleDocContentSql; private MetadataColumns metadataColumns; @@ -311,6 +312,14 @@ public void init(AdaptorContext context) throws Exception { .setContentSqlColumns(cfg.getValue("db.singleDocContentSqlParameters")) .setAclSqlColumns(cfg.getValue("db.aclSqlParameters")); + if (aclSql == null) { + aclHandler = new AclHandler(singleDocContentSql, + ukBuilder.getContentSqlColumns()).withResultSetStrategy(); + } else { + aclHandler = new AclHandler(aclSql, + ukBuilder.getAclSqlColumns()).withQueryStrategy(); + } + // Verify all column names. try (Connection conn = makeNewConnection()) { Map columnTypes = @@ -674,7 +683,7 @@ public void getDocContent(Request req, Response resp) throws IOException { // Generate response metadata first. addMetadataToResponse(resp, rs); // Generate Acl. - resp.setAcl(getAcl(conn, rs, id.getUniqueId())); + resp.setAcl(aclHandler.forContent().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. @@ -689,28 +698,72 @@ public static void main(String[] args) { AbstractAdaptor.main(new DatabaseAdaptor(), args); } - private Acl getAcl(Connection conn, ResultSet contentRs, String uniqueId) - throws SQLException { - if (aclSql != null || contentRs == null) { - try (PreparedStatement stmt = getAclFromDb(conn, uniqueId); + private class AclHandler { + String sql; + List sqlColumns; + ResultSetNext rsNext; + Acl defaultAcl; + Strategy contentStrategy; + Strategy authzStrategy; + + AclHandler(String sql, List sqlColumns) { + this.sql = sql; + this.sqlColumns = sqlColumns; + this.authzStrategy = new QueryStrategy(); + } + + public AclHandler withQueryStrategy() { + contentStrategy = new QueryStrategy(); + rsNext = ResultSetNext.PROCESS_ALL_ROWS; + defaultAcl = Acl.EMPTY; + return this; + } + + public AclHandler withResultSetStrategy() { + contentStrategy = new ResultSetStrategy(); + rsNext = ResultSetNext.PROCESS_ONE_ROW; + defaultAcl = null; + return this; + } + + public Strategy forContent() { + return contentStrategy; + } + + public Strategy forAuthz() { + return authzStrategy; + } + + public abstract class Strategy { + public abstract Acl getAcl(Connection conn, ResultSet rs, String uniqueId) + throws SQLException; + } + + private class QueryStrategy extends Strategy { + public Acl getAcl(Connection conn, ResultSet ignored, String uniqueId) + throws SQLException { + try (PreparedStatement stmt = getAclFromDb(conn, uniqueId, sql, sqlColumns); ResultSet rs = stmt.executeQuery()) { log.finer("got acl"); 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 { return buildAcl(rs, aclPrincipalDelimiter, aclNamespace, - ResultSetNext.PROCESS_ONE_ROW, null); + rsNext, defaultAcl); } } - } else { + } + } + + private class ResultSetStrategy extends Strategy { + public Acl getAcl(Connection conn, ResultSet contentRs, String uniqueId) + throws SQLException { // Generate ACL if it came as part of result to singleDocContentSql. return buildAcl(contentRs, aclPrincipalDelimiter, aclNamespace, - ResultSetNext.PROCESS_ONE_ROW, null); + rsNext, defaultAcl); + } } } @@ -830,16 +883,10 @@ private PreparedStatement getDocFromDb(Connection conn, } 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); - } + String uniqueId, String sql, List sqlColumns) + throws SQLException { + PreparedStatement st = conn.prepareStatement(sql); + uniqueKey.setSqlValues(st, uniqueId, sqlColumns); log.log(Level.FINER, "about to get acl: {0}", uniqueId); return st; } @@ -1128,7 +1175,7 @@ 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, null, id.getUniqueId()); + Acl acl = aclHandler.forAuthz().getAcl(conn, null, id.getUniqueId()); if (acl == null) { result.put(id, AuthzStatus.PERMIT); } else if (userIdentity == null || userIdentity.getUser() == null) { diff --git a/src/com/google/enterprise/adaptor/database/UniqueKey.java b/src/com/google/enterprise/adaptor/database/UniqueKey.java index 8488f86..29275eb 100644 --- a/src/com/google/enterprise/adaptor/database/UniqueKey.java +++ b/src/com/google/enterprise/adaptor/database/UniqueKey.java @@ -112,7 +112,7 @@ String makeUniqueId(ResultSet rs) throws SQLException, URISyntaxException { return sb.toString().substring(1); } - private void setSqlValues(PreparedStatement st, String uniqueId, + void setSqlValues(PreparedStatement st, String uniqueId, List sqlCols) throws SQLException { // parse on / that isn't preceded by escape char _ // (a / that is preceded by _ is part of column value)