[ https://issues.apache.org/jira/browse/ATLAS-497?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15264200#comment-15264200 ]
Saqeeb Shaikh commented on ATLAS-497: ------------------------------------- Thanks [~yhemanth] for reviewing the patch. Please find my comments inline. * Do we have a requirement for separating creates and updates? Can we merge them into one write operation? In fact many operations in Atlas backend are a create or update kind of operation. Merging into one may be better, IMHO. ** *Can we please finalize the requirement for this and track it as part of another JIRA?* * In AtlasAccessRequest and PolicyUtil there are many unused methods. Please remove them. ** *Some of the unused methods from AtlasAccessRequest, will be used when I add support for RangerPlugin. I will remove the setter methods from the PolicyUtils.* * In the authorization code path where AtlasException is thrown due to authorization problems, maybe it is better to throw a custom AtlasAuthorizationException. This could have information about what was attempted to be accessed etc. ** *Will make this change* * For requests that require access to multiple resource types (e.g. paths like entities/traits - which requires access to both entities & traits), access should be granted only if all of them are allowed, no? Currently, even if one matches we are allowing access, as far as I can tell. ** *Yes, you are right, the access to such resources is granted only if the user or group have access to both the resource types. In SimpleAtlasAuthorizer.checkAccess() method, I am iterating through each of the resourceTypes that the user has access to and if he has access to all of them only then he is granted access.* * Currently, since we don't have resource specific match, in SimpleAtlasAuthorizer, can we simplify the resource check logic and just check for access to resourcetypes for now? ** *Yes, I can change it for now, however I had added this keeping in mind that down the line we will support resource filtering as well.* * Without the above, there are some important issues: for e.g. since SimpleAtlasAuthorizer is a singleton object, the value isMatchAny is being accessed in a non-thread safe manner. ** *Yes, I will fix this* * In a later JIRA, we'll need to figure how principal information like user name / groups will be got in Kerberos authentication case. This is because currently we are picking these up from Spring security context. ** *Yes, I will raise another JIRA for this.* * Can we please add a merge test in PolicyUtilTest - one that has > 1 policies with different (possibly conflicting) rules and see how the end result works out? * Please add some tests for AtlasAuthorizationFilter. ** *Yes, I am adding more test cases for this feature.* > Simple Authorization > -------------------- > > Key: ATLAS-497 > URL: https://issues.apache.org/jira/browse/ATLAS-497 > Project: Atlas > Issue Type: New Feature > Affects Versions: 0.7-incubating > Reporter: Erik Bergenholtz > Assignee: Saqeeb Shaikh > Fix For: 0.7-incubating > > Attachments: ATLAS-497.1.patch, ATLAS-497.2.patch, ATLAS-497.patch > > > Atlas needs to support a simple (out of box) authorization mechanism. > Defined Roles: > - Data Scientist: provides a read only view (GET) > - Data Steward: provides a read/edit view (PUT, POST, DELETE) > - Admin (can do anything) > All can comment on entity > Requirements > - Atlas will implement a simple file based store for providing user to role > mapping > - The out of box experience will be this file based mechanism for > authorization -- This message was sent by Atlassian JIRA (v6.3.4#6332)