[
https://issues.apache.org/jira/browse/MAPREDUCE-1455?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12836193#action_12836193
]
Vinod K V commented on MAPREDUCE-1455:
--------------------------------------
The patch looks good functionally. Some code-level comments:
JSPUtil.java
- What about the configuration {{webinterface.private.actions}}? It was
originally added as part of HADOOP-1484 'cause authentication/authorization
were missing back then. Now that we have them in place, it doesn't look like we
really need it anymore. I'm fine fixing this in another issue.
- The variable 'conf' should actually be removed, instead of just putting a
warning comment about its usage. We should fix the usage of this conf object in
this patch itself, which I find is only at one place in JSPUtil.
- Can we move the UGI.doAs() checks also from all the JSPs into
{{JSPUtil.checkAccessAndGetJob()}}? Right now, the authorization code for
checking view-job-acl is sprinkled and is inconsistent across various JSPs. For
e.g., unnecessary AccessControlException handling is being done in
TaskGraphServlet. All this will go away if we simply encapsulate *all* the
authorization code inside {{JSPUtil.checkAccessAndGetJob()}} and let it return
JIP on success or if authorization is disabled and null if authorization fails.
Once we do this, all the authorizaton code will be in JSPUtil and once this
call returns, we can simply then return back from JSPs.
- Given above, we can even overload {{JSPUtil.checkAccessAndGetJob()}}, (add a
new JobOperation enum?) and make it something like
{{JSPUtil.checkAccessAndDoOperation(JobOperation)}}. That will make things much
much simpler, I think.
JobACLsManager.java
- Very minor: " Access Control List configured for this job : " should instead
be "Access control list ... " (letter-case)
JobTracker.java
- Can we add the call to {{refreshUserToGroupsMappings(conf)}} in
{{JobTracker.offerService()}} in another JIRA issue? 'Coz I a can think of
test-cases for this and adding them here would stretch this issue.
TaskLog.java
- Rename taskLogAclFile to jobACLsFile?
- TaskLogsServlet is also be now passed through servlet filters.Should we add
a testcase for web-authentication for this servlet. Split the test into two?
One for authentication and one for authorization? Should we enhance the tests
in general w.r.t authentication. For e.g. we now are not verifying
authentication failures from the filters.
TaskLogServlet.java
- We should replace the method {{checkAccessForTaskLogs()}} with calls to
{{JobACLsManager}} now. Perhaps after refactoring around code in JobACLsManager
a bit. TaskTracker should also instantiate a JobACLsManager object similar to
JT.
TaskRunner.java
- Rename {{writeTaskLogAcls()}} to {{writeJobACLs()}}?
- New test in TestTrackerLocaliztion to verify proper writing of the acls file
on the local disk? Also verify the read back?
> Authorization for servlets
> --------------------------
>
> Key: MAPREDUCE-1455
> URL: https://issues.apache.org/jira/browse/MAPREDUCE-1455
> Project: Hadoop Map/Reduce
> Issue Type: Sub-task
> Components: jobtracker, security, tasktracker
> Reporter: Devaraj Das
> Assignee: Ravi Gummadi
> Fix For: 0.22.0
>
> Attachments: 1455.patch, 1455.v1.patch
>
>
> This jira is about building the authorization for servlets (on top of
> MAPREDUCE-1307). That is, the JobTracker/TaskTracker runs authorization
> checks on web requests based on the configured job permissions. For e.g., if
> the job permission is 600, then no one except the authenticated user can look
> at the job details via the browser. The authenticated user in the servlet can
> be obtained using the HttpServletRequest method.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
