> On Jan. 30, 2015, 6:36 a.m., Abraham Elmahrek wrote: > > security/src/main/java/org/apache/sqoop/security/Authorization/AuthorizationEngine.java, > > line 32 > > <https://reviews.apache.org/r/30266/diff/2/?file=836112#file836112line32> > > > > Potentially make methods non static so that they are mockable. I don't > > think it matters so much in the current implementation.
There is no need to mock AuthorizationEngine. The classes need to be implemeneted is AuthorizationHandler, AuthorizationAccessController and AuthorizationValidator. Thank you. > On Jan. 30, 2015, 6:36 a.m., Abraham Elmahrek wrote: > > security/src/main/java/org/apache/sqoop/security/Authorization/AuthorizationEngine.java, > > lines 63-70 > > <https://reviews.apache.org/r/30266/diff/2/?file=836112#file836112line63> > > > > Avoid exception handling for control flow? > > > > Note on style: potentially use java list filters instead. Big NIT > > though. That's a good suggestion. Replaced by java list filters. > On Jan. 30, 2015, 6:36 a.m., Abraham Elmahrek wrote: > > security/src/main/java/org/apache/sqoop/security/Authorization/AuthorizationEngine.java, > > line 81 > > <https://reviews.apache.org/r/30266/diff/2/?file=836112#file836112line81> > > > > Create privileges seem to use empty strings as resource IDs. Is the > > resource ID arbitrary and can be any thing? If so, it might make sense to > > make this a configurable string. The privilege type of create is global, which means resource ID will be ignored. Replaced by StringUtils.Empty, align with Sqoop code style. > On Jan. 30, 2015, 6:36 a.m., Abraham Elmahrek wrote: > > security/src/main/java/org/apache/sqoop/security/Authorization/AuthorizationEngine.java, > > line 155 > > <https://reviews.apache.org/r/30266/diff/2/?file=836112#file836112line155> > > > > return boolean? If failed, exception will be thrown, which aligns with Hive implementation (https://github.com/apache/hive/blob/trunk/ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/HiveAuthorizationValidator.java#L42). - richard ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30266/#review70321 ----------------------------------------------------------- On Jan. 30, 2015, 7:46 a.m., richard zhou wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30266/ > ----------------------------------------------------------- > > (Updated Jan. 30, 2015, 7:46 a.m.) > > > Review request for Sqoop. > > > Repository: sqoop-sqoop2 > > > Description > ------- > > RBAC engine, which handles all rules. Such as, create a job needs global > create job privilege and use related link privilege. > > > Diffs > ----- > > core/src/main/java/org/apache/sqoop/security/SecurityError.java > c68b666c83212d91e183d8041e694629a62b7224 > > security/src/main/java/org/apache/sqoop/security/Authorization/AuthorizationEngine.java > PRE-CREATION > server/src/main/java/org/apache/sqoop/handler/ConnectorRequestHandler.java > ccf928eb0efbb2137b9269db03ee444f51c034e6 > server/src/main/java/org/apache/sqoop/handler/JobRequestHandler.java > f4482cee7459febb37bb747a011f611655412783 > server/src/main/java/org/apache/sqoop/handler/LinkRequestHandler.java > 74fa321a57468e9b75b75213f0ee20ed708c8ead > server/src/main/java/org/apache/sqoop/handler/SubmissionRequestHandler.java > a0b29c86b7b7d7fe8d5fc185164c7899050083ca > > Diff: https://reviews.apache.org/r/30266/diff/ > > > Testing > ------- > > local + integration test > > > Thanks, > > richard zhou > >
