----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38359/#review98978 -----------------------------------------------------------
Initial thoughts. distribution/src/resources/drill-override-example.conf (line 86) <https://reviews.apache.org/r/38359/#comment155767> maybe rename session_max_idle_secs exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/StorageResources.java (line 73) <https://reviews.apache.org/r/38359/#comment155762> You can inject security context at the class level so you don't have to pass around so much. exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/StorageResources.java (line 91) <https://reviews.apache.org/r/38359/#comment155763> Can you update all of these. Let's just create a ViewableWithPermissions or similar. Then we can create a Map<String, Object> as the object with "model" as the value we pass in. And the appropriate other values as booleans. This should work and allow everything else to be the same. See here for why I think it will work: https://github.com/jersey/jersey/blob/master/ext/mvc-freemarker/src/main/java/org/glassfish/jersey/server/mvc/freemarker/FreemarkerViewProcessor.java#L120 exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/ThreadsResources.java (line 31) <https://reviews.apache.org/r/38359/#comment155764> This seems like it should be ADMIN exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/WebUserSession.java (line 31) <https://reviews.apache.org/r/38359/#comment155761> Why do we have a WebUserSession and a principal? Shouldn't they be interconnected? exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/WebUserSession.java (line 42) <https://reviews.apache.org/r/38359/#comment155754> What happens if a user happens to have the account named anonymous? exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/auth/DrillUserPrincipal.java (line 47) <https://reviews.apache.org/r/38359/#comment155760> It seems wrong that we are holding onto this. Do we need to? exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/ProfileResources.java (line 166) <https://reviews.apache.org/r/38359/#comment155756> encapsulate separately for each use exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/ProfileResources.java (line 191) <https://reviews.apache.org/r/38359/#comment155757> I don't think the ModelWrapper makes sense. We should be specific about what we are passing and make it the model type. To me the generic approach leaks to much to the template when it isn't needed. exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/ProfileResources.java (line 294) <https://reviews.apache.org/r/38359/#comment155758> Please encapsulate this as above. E.g. canRead and canManage (which probably delegate to the same underlying method). Right now you have this in numerous places. I actually would suggest that you add this to the principal: for example: boolean isReadAllowed(Profile) boolean isManageAllowed(Profile) exec/java-exec/src/main/resources/rest/generic.ftl (line 59) <https://reviews.apache.org/r/38359/#comment155765> You should leak authorization logic into the template. This should be constrained to the model. Along the following: boolean showStorage() boolean showOptions() boolean showLoginLogout() pom.xml (line 167) <https://reviews.apache.org/r/38359/#comment155766> We should include apache headers in html. - Jacques Nadeau On Sept. 14, 2015, 5:43 p.m., Venki Korukanti wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38359/ > ----------------------------------------------------------- > > (Updated Sept. 14, 2015, 5:43 p.m.) > > > Review request for drill, Jacques Nadeau and Jason Altekruse. > > > Repository: drill-git > > > Description > ------- > > Use jetty's SecurityHandler (with FormAuthenticator and LoginService) to > enforce authentication. Use jersey's annotations to enforece authorizations. > > > Diffs > ----- > > distribution/src/resources/drill-override-example.conf 805d6e9 > exec/java-exec/src/main/java/org/apache/drill/exec/ExecConstants.java > 0f6a5bb > > exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/DrillRestServer.java > 8c14587 > > exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/DrillRoot.java > 3e972b4 > > exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/LogOutServlet.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/MetricsResources.java > 28a292b > > exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/ModelWrapper.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/QueryResources.java > 145a476 > > exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/QueryWrapper.java > ee31929 > > exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/RestServerHelper.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/StatusResources.java > c99c49b > > exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/StorageResources.java > 49f387c > > exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/ThreadsResources.java > def5acb > > exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/WebUserSession.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/auth/AnonymousAuthenticator.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/auth/AnonymousLoginService.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/auth/DrillRestLoginService.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/auth/DrillUserPrincipal.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/ProfileResources.java > 6656bf6 > exec/java-exec/src/main/resources/drill-module.conf dbe449a > exec/java-exec/src/main/resources/rest/generic.ftl 9df2424 > exec/java-exec/src/main/resources/rest/index.ftl 99e9d8c > exec/java-exec/src/main/resources/rest/options.ftl 7ba1250 > exec/java-exec/src/main/resources/rest/profile/list.ftl cf92ede > exec/java-exec/src/main/resources/rest/profile/profile.ftl 47c7e06 > exec/java-exec/src/main/resources/rest/query/errorMessage.ftl dbdcc9e > exec/java-exec/src/main/resources/rest/query/result.ftl 7fe52a4 > exec/java-exec/src/main/resources/rest/static/img/apache-drill-logo.png > PRE-CREATION > exec/java-exec/src/main/resources/rest/static/login.html PRE-CREATION > exec/java-exec/src/main/resources/rest/status.ftl cafa523 > exec/java-exec/src/main/resources/rest/storage/list.ftl ef97561 > exec/java-exec/src/main/resources/rest/storage/update.ftl 2a276e1 > pom.xml c17e612 > > Diff: https://reviews.apache.org/r/38359/diff/ > > > Testing > ------- > > Currently testing is manual. Rest based unittests are coming in DRILL-2965. > > > Thanks, > > Venki Korukanti > >
