----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47612/#review134104 -----------------------------------------------------------
authorizer/src/main/java/org/apache/atlas/authorize/AtlasAccessRequest.java (line 21) <https://reviews.apache.org/r/47612/#comment198753> Instead of importing static methods, consider using the ClassName.methodName() while calling the methods. authorizer/src/main/java/org/apache/atlas/authorize/AtlasAuthorizerFactory.java (line 64) <https://reviews.apache.org/r/47612/#comment198751> Review and update the intendation in line #64 and #65. authorizer/src/main/java/org/apache/atlas/authorize/simple/AtlasAuthorizationUtils.java (line 41) <https://reviews.apache.org/r/47612/#comment198748> Second paramter ", contextPath.length()" is not necessary. Consider removing it. authorizer/src/main/java/org/apache/atlas/authorize/simple/AtlasAuthorizationUtils.java (line 44) <https://reviews.apache.org/r/47612/#comment198754> ensure that contextPath starts with '/'; only in such case remove leading '/'. authorizer/src/main/java/org/apache/atlas/authorize/simple/AtlasAuthorizationUtils.java (line 46) <https://reviews.apache.org/r/47612/#comment198756> Here only first or first 2 tokes are of interest; hence add limit to split call: contextPath.split("/", 3). For example: /entities/1212-qwf33-d4425/xyz/123/rty 0: entities 1: 1212-qwf33-d4425 2: xyz/123/rty /v1/taxonomy/myTaxonomy/abc/1/xyz/1/pqrst 0: v1 1: taxonomy 2: myTaxonomy/abc/1/xyz/1/pqrst authorizer/src/main/java/org/apache/atlas/authorize/simple/AtlasAuthorizationUtils.java (line 48) <https://reviews.apache.org/r/47612/#comment198757> This assumes that if first token from split() is "v1", a second token exists i.e. split.length >= 2. This may not be a safe assumption. Please validate length before accessing split[1] authorizer/src/main/java/org/apache/atlas/authorize/simple/AtlasAuthorizationUtils.java (line 111) <https://reviews.apache.org/r/47612/#comment198758> Looking at getApi() return value, api.startsWith() can be replaced with api.equals() - please review and update all such uses. authorizer/src/main/java/org/apache/atlas/authorize/simple/AtlasAuthorizationUtils.java (line 113) <https://reviews.apache.org/r/47612/#comment198759> In calls to contextPath.contains() here, consider using leading "/" - for example contextPath.contains("/gremlin"). This can help eliminate unexpected matches if the given string is present elsewhere in the URL. common/src/main/java/org/apache/atlas/utils/PropertiesUtil.java (line 58) <https://reviews.apache.org/r/47612/#comment198762> Why not use props.stringPropertyNames() - similar to line #49? webapp/src/main/java/org/apache/atlas/web/filters/AtlasAuthorizationFilter.java (line 158) <https://reviews.apache.org/r/47612/#comment198763> atlasResourceTypes and atlasRequest.getResourceTypes() are same - look at line #128. Please review and remove the duplicate from the message string. Review and update the message at line #156 for the same issue. - Madhan Neethiraj On May 19, 2016, 6:49 p.m., Nixon Rodrigues wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/47612/ > ----------------------------------------------------------- > > (Updated May 19, 2016, 6:49 p.m.) > > > Review request for atlas, Madhan Neethiraj, Shwetha GS, Selvamohan Neethiraj, > and Hemanth Yamijala. > > > Bugs: ATLAS-495 > https://issues.apache.org/jira/browse/ATLAS-495 > > > Repository: atlas > > > Description > ------- > > Patch contains following changes : > > 1) Refactoring of authorization code to authorizer module to be reused by > webapp, ranger plugin modules. > 1) Default policy file path to atlas-home/conf/policy-store.txt if property > is not set in atlas-application.properties file. > 2) Renamed create action instead of write. > 4) Added authorizer impl class in atlas properties and fall back to simple > authorizer if authorizer is not set in property. > 5) AtlasAuthorizerFactory for authorizer. > 6) Handled changes in Test cases and in filter as per new implementation of > AtlasAuthorizerFactory. > > > Diffs > ----- > > authorizer/pom.xml PRE-CREATION > authorizer/src/main/java/org/apache/atlas/authorize/AtlasAccessRequest.java > PRE-CREATION > authorizer/src/main/java/org/apache/atlas/authorize/AtlasActionTypes.java > PRE-CREATION > > authorizer/src/main/java/org/apache/atlas/authorize/AtlasAuthorizationException.java > PRE-CREATION > authorizer/src/main/java/org/apache/atlas/authorize/AtlasAuthorizer.java > PRE-CREATION > > authorizer/src/main/java/org/apache/atlas/authorize/AtlasAuthorizerFactory.java > PRE-CREATION > authorizer/src/main/java/org/apache/atlas/authorize/AtlasResourceTypes.java > PRE-CREATION > > authorizer/src/main/java/org/apache/atlas/authorize/simple/AtlasAuthorizationUtils.java > PRE-CREATION > > authorizer/src/main/java/org/apache/atlas/authorize/simple/FileReaderUtil.java > PRE-CREATION > authorizer/src/main/java/org/apache/atlas/authorize/simple/PolicyDef.java > PRE-CREATION > > authorizer/src/main/java/org/apache/atlas/authorize/simple/PolicyParser.java > PRE-CREATION > authorizer/src/main/java/org/apache/atlas/authorize/simple/PolicyUtil.java > PRE-CREATION > > authorizer/src/main/java/org/apache/atlas/authorize/simple/SimpleAtlasAuthorizer.java > PRE-CREATION > > authorizer/src/test/java/org/apache/atlas/authorize/simple/AtlasAuthorizationUtilsTest.java > PRE-CREATION > > authorizer/src/test/java/org/apache/atlas/authorize/simple/PolicyParserTest.java > PRE-CREATION > > authorizer/src/test/java/org/apache/atlas/authorize/simple/PolicyUtilTest.java > PRE-CREATION > > authorizer/src/test/java/org/apache/atlas/authorize/simple/SimpleAtlasAuthorizerTest.java > PRE-CREATION > common/pom.xml 614b3f6 > common/src/main/java/org/apache/atlas/utils/PropertiesUtil.java > PRE-CREATION > common/src/main/java/org/apache/atlas/utils/XMLPropertiesUtil.java > PRE-CREATION > distro/src/conf/atlas-application.properties d4722fb > pom.xml 30fb95a > webapp/pom.xml 4b67ffa > webapp/src/main/java/org/apache/atlas/authorize/AtlasAccessRequest.java > 5db9646 > webapp/src/main/java/org/apache/atlas/authorize/AtlasAccessorTypes.java > 5f3827a > webapp/src/main/java/org/apache/atlas/authorize/AtlasActionTypes.java > 13c8b53 > > webapp/src/main/java/org/apache/atlas/authorize/AtlasAuthorizationException.java > 676c9f9 > > webapp/src/main/java/org/apache/atlas/authorize/AtlasAuthorizationUtils.java > 14a2aac > webapp/src/main/java/org/apache/atlas/authorize/AtlasAuthorizer.java > 7c93c7a > webapp/src/main/java/org/apache/atlas/authorize/AtlasResourceTypes.java > 14a72f1 > webapp/src/main/java/org/apache/atlas/authorize/PolicyDef.java 0ee39df > webapp/src/main/java/org/apache/atlas/authorize/PolicyParser.java 51a6dc2 > webapp/src/main/java/org/apache/atlas/authorize/PolicyUtil.java a565f96 > webapp/src/main/java/org/apache/atlas/authorize/SimpleAtlasAuthorizer.java > 2a32e4e > webapp/src/main/java/org/apache/atlas/util/FileReaderUtil.java 22eaff9 > webapp/src/main/java/org/apache/atlas/util/PropertiesUtil.java fef8efb > webapp/src/main/java/org/apache/atlas/util/XMLPropertiesUtil.java 9c4f1c7 > > webapp/src/main/java/org/apache/atlas/web/filters/AtlasAuthorizationFilter.java > 13fc7da > > webapp/src/main/java/org/apache/atlas/web/security/AtlasADAuthenticationProvider.java > 9e5df45 > > webapp/src/main/java/org/apache/atlas/web/security/AtlasLdapAuthenticationProvider.java > e66b930 > webapp/src/main/webapp/WEB-INF/applicationContext.xml b58952c > > webapp/src/test/java/org/apache/atlas/authorize/AtlasAuthorizationUtilsTest.java > 5fc4420 > webapp/src/test/java/org/apache/atlas/authorize/PolicyParserTest.java > 507d4c6 > webapp/src/test/java/org/apache/atlas/authorize/PolicyUtilTest.java 59e88c9 > > webapp/src/test/java/org/apache/atlas/authorize/SimpleAtlasAuthorizerTest.java > 5041e6f > > Diff: https://reviews.apache.org/r/47612/diff/ > > > Testing > ------- > > Verified :: Rat check and mvn clean install > > Manually verified policy enforcement for 3 different groups. > > > Thanks, > > Nixon Rodrigues > >
