mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Mann <g...@mesosphere.io>
Subject Re: Review Request 46501: Updated authorization.md to reflect current changes.
Date Tue, 26 Apr 2016 05:20:28 GMT

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




docs/authorization.md (line 12)
<https://reviews.apache.org/r/46501/#comment194321>

    s/JSON based/JSON-based/
    
    It might be helpful at the beginning to give a broad picture of how these ACLs work, something
like: "When authorizing an action, Mesos proceeds through the list of relevant ACLs until
it finds one that can either grant or deny permission to the  subject making the request."



docs/authorization.md (line 14)
<https://reviews.apache.org/r/46501/#comment194322>

    I would prefer to avoid mentioning 'acls.proto' here; I think that we should try to create
documentation that provides adequate guidance for users without referring them to the code
itself.
    
    I think that we could provide a schema here, or we might be able to get rid of this passage
(except for the explanation of the `permissive` field). The two passages following this one
do a good job of explaining the structure of the ACLs.
    
    What do you think?



docs/authorization.md (lines 16 - 18)
<https://reviews.apache.org/r/46501/#comment194324>

    s/consist on an array/consists of an array/
    s/Each of this objects/Each of these objects/
    
    I would recommend splitting the second sentence into three: "Each of these objects has
two entries. The first, `principals`, is common to all actions and describes the subjects
which wish to perform the given action. The second entry varies among actions and describes
the object on which the action will be executed."
    
    I would prefer to avoid mentioning the `Entity` protobuf message directly. I think it
would be easier to understand if we simply describe the characteristics of the subject and
object fields, as you do in line 18, without mentioning `Entity`. What do you think?



docs/authorization.md (line 21)
<https://reviews.apache.org/r/46501/#comment194325>

    s/which is default/which is the default/
    s/In case permissive/If permissive/
    s/set to false all/set to false, all/
    s/non-matching request/non-matching requests/



docs/authorization.md (line 24)
<https://reviews.apache.org/r/46501/#comment194326>

    The ACL doesn't imply that `foo` can register in no other role besides `analytics`. If
`foo` tries to register in another role, it won't match any ACL, and since `permissive` is
true by default, the request will be authorized.



docs/authorization.md (line 49)
<https://reviews.apache.org/r/46501/#comment194328>

    s/set to `false` and hence principals/set to `false`; hence, principals/
    
    /but not as any other user/and not as any other user/



docs/authorization.md (lines 51 - 61)
<https://reviews.apache.org/r/46501/#comment194327>

    I think the doc would benefit from a couple more examples here. An example showcasing
the order of ACLs might be helpful (i.e., a case where the order of the ACLs really matters).
Also, perhaps an example with multiple principals/objects in those JSON lists?



docs/authorization.md (line 65)
<https://reviews.apache.org/r/46501/#comment194329>

    s/Currently _local authorizer_/Currently the _local authorizer_/
    
    s/for following/for the following/



docs/authorization.md (line 92)
<https://reviews.apache.org/r/46501/#comment194330>

    I would change the actions to be consistent in tense. The preceding items use the progressive
tense - i.e., "Destroying quotas" - while this one doesn't - "Reserve resources".



docs/authorization.md (line 94)
<https://reviews.apache.org/r/46501/#comment194331>

    Could also be an operator



docs/authorization.md (line 99)
<https://reviews.apache.org/r/46501/#comment194332>

    Can also be an operator



docs/authorization.md (line 104)
<https://reviews.apache.org/r/46501/#comment194333>

    Can also be an operator



docs/authorization.md (line 109)
<https://reviews.apache.org/r/46501/#comment194334>

    Can also be an operator



docs/authorization.md (line 117)
<https://reviews.apache.org/r/46501/#comment194335>

    s/Implementing Authorizer/Implementing an Authorizer/



docs/authorization.md (line 119)
<https://reviews.apache.org/r/46501/#comment194336>

    Is this section targeted at people who will write their own authorizer modules? If so,
we could make this more explicit like so: "In case you plan to implement your own authorizer
module, the authorization interface consists of two parts:"



docs/authorization.md (line 120)
<https://reviews.apache.org/r/46501/#comment194337>

    s/First the/First, the/
    s/interface defining the/interface defines the/
    s/and _local authorizer_/and the _local authorizer_/
    s/In case the request ... otherwise./This function should return `false` if the request
is rejected and `true` otherwise./



docs/authorization.md (line 122)
<https://reviews.apache.org/r/46501/#comment194338>

    s/Secondly the/Second, the/
    s/message representing/message represents/
    
    Actually, since `authorization::Request` occurs in the definition of `authorized()`, perhaps
it's better for this part of the interface to be explained first, what do you think?



docs/authorization.md (line 141)
<https://reviews.apache.org/r/46501/#comment194339>

    s/is kept optional ... quantification./is optional; if it isn't set, then the `Subject`
or `Object` will only match an ACL with ANY or NONE in the corresponding location./
    
    s/This allows users can construct/This allows users to construct/



docs/authorization.md (line 143)
<https://reviews.apache.org/r/46501/#comment194340>

    s/valid action is necessary/valid action necessary/



docs/authorization.md (line 147)
<https://reviews.apache.org/r/46501/#comment194341>

    Do we really need this here?


- Greg Mann


On April 22, 2016, 12:13 p.m., Alexander Rojas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46501/
> -----------------------------------------------------------
> 
> (Updated April 22, 2016, 12:13 p.m.)
> 
> 
> Review request for mesos, Adam B, Greg Mann, and Neil Conway.
> 
> 
> Bugs: MESOS-4785
>     https://issues.apache.org/jira/browse/MESOS-4785
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The API of the authorization has been changing constantly over the
> last few versions. This patch attempts to update the documentation to
> the those changes into account.
> 
> 
> Diffs
> -----
> 
>   docs/authorization.md 0db5c345b3239814b3b9d2e8a87601ff69d0f869 
> 
> Diff: https://reviews.apache.org/r/46501/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message