-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46969/#review131795
-----------------------------------------------------------



Good start, but I have some basic questions.


include/mesos/authorizer/authorizer.proto (lines 38 - 43)
<https://reviews.apache.org/r/46969/#comment195828>

    Is your intention that only one of these optional fields will be set for a 
particular request, or can multiple of them be set?
    If only-one, then you should add a Type enum so modules don't have to guess 
which field to use.



include/mesos/authorizer/authorizer.proto (lines 40 - 42)
<https://reviews.apache.org/r/46969/#comment195826>

    Can you give an example of why we would want/need each of these? 
ExecutorInfo.name is insufficient



src/authorizer/local/authorizer.cpp (line 269)
<https://reviews.apache.org/r/46969/#comment195831>

    ExecutorInfo.name is a silly thing to authorize on, since it's arbitrarily 
set by the user, and there's no control over what values it can be set to.
    FrameworkInfo.role and CommandInfo.user are both the subjects of 
creation-time authorization checks, so authorizing read access by these fields 
is sensible.



src/authorizer/local/authorizer.cpp (line 274)
<https://reviews.apache.org/r/46969/#comment195829>

    s/name field/user field/
    s/ExecutorInfo/CommandInfo/



src/authorizer/local/authorizer.cpp (line 276)
<https://reviews.apache.org/r/46969/#comment195830>

    If not set, use FrameworkInfo.user


- Adam B


On May 4, 2016, 5:58 a.m., Joerg Schad wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46969/
> -----------------------------------------------------------
> 
> (Updated May 4, 2016, 5:58 a.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rojas.
> 
> 
> Bugs: MESOS-5169
>     https://issues.apache.org/jira/browse/MESOS-5169
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This allows us to use common protobug messages for authorization.
> 
> 
> Diffs
> -----
> 
>   include/mesos/authorizer/authorizer.proto 
> 32492a59ad95df3bb673ec42321518f86c11af59 
>   src/authorizer/local/authorizer.cpp 
> e59c11269670a7ed72b780913971b421ee17f33f 
> 
> Diff: https://reviews.apache.org/r/46969/diff/
> 
> 
> Testing
> -------
> 
> tested entire chain (see upcoming patches)
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Reply via email to