mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Adam B <a...@mesosphere.io>
Subject Re: Review Request 47794: Added authorization support for mesos::internal::Files.
Date Wed, 25 May 2016 03:27:04 GMT

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



Looking pretty good. Just a couple of comments about wrapping/indentation, and some Options
that no longer need to be optional.


src/files/files.cpp (lines 152 - 153)
<https://reviews.apache.org/r/47794/#comment199548>

    I'd love a comment explaining what the key/value represent here



src/files/files.cpp (line 304)
<https://reviews.apache.org/r/47794/#comment199552>

    s/attached/requestPath/? since it's not necessarily the attached path until it is stripped
to something that exists in `authorizations`. In fact, they may have requested a path that
doesn't exist
    Ditto elsewhere



src/files/files.cpp (line 307)
<https://reviews.apache.org/r/47794/#comment199560>

    Are we guaranteed that this will always be false on the first iteration? What if the requested
path is "/" or "dirname"?
    Perhaps this should be a `do{}while()`?



src/files/files.cpp (lines 309 - 310)
<https://reviews.apache.org/r/47794/#comment199559>

    Can you wrap on `.then(` instead, please?



src/files/files.cpp (line 327)
<https://reviews.apache.org/r/47794/#comment199555>

    `path` is guaranteed to be Some by here, so you can remove the `Option`



src/files/files.cpp (lines 436 - 439)
<https://reviews.apache.org/r/47794/#comment199558>

    This wrapping/indentation feels off.
    First of all, we usually put `.then(...` on a new line, indented 2 spaces from the previous.
    Secondly, if the `bool authorize...{` was on the same line as the previous, then `if (authorized)
{` would only be indented two spaces in from `.then(`, so I'd think that should hold, even
if we have to wrap the `bool authorized)...{` line.
    
    Perhaps:
    ```
    return authorizations[attached](principal)
      .then([this, offset, length, path, jsonp](bool authorized) 
          -> Future<Response> {
        if (authorized) {
    ```



src/tests/files_tests.cpp (line 334)
<https://reviews.apache.org/r/47794/#comment199561>

    s/reads/browse/



src/tests/files_tests.cpp (line 390)
<https://reviews.apache.org/r/47794/#comment199562>

    s/reads/download/


- Adam B


On May 24, 2016, 2:39 p.m., Alexander Rojas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47794/
> -----------------------------------------------------------
> 
> (Updated May 24, 2016, 2:39 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Mahler, Joerg Schad, Michael Park, and Vinod
Kone.
> 
> 
> Bugs: MESOS-5153
>     https://issues.apache.org/jira/browse/MESOS-5153
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Adds an optional parameter to the `mesos::internal::Files::attach()`
> method. The type of this parameter is a callable object which returns
> a future to a boolean and takes as parameter an optional string
> representing a principal name.
> 
> The parameter is called, if set, whenever one of the routed endpoints
> of the `Files` object is accessed through HTTP. If the callable object
> returns a false boolean, then processing of the request is aborted
> and a `403 Forbidden` response is returned.
> 
> 
> Diffs
> -----
> 
>   src/files/files.hpp 90acb3406c46c164108deb559af71fb109a5773b 
>   src/files/files.cpp e4b0ada00aabba6553810391f4015a896f8a69a5 
>   src/tests/files_tests.cpp 5d6620d13babaf0bb7f9c888bb1b4fa2228b6ccd 
> 
> Diff: https://reviews.apache.org/r/47794/diff/
> 
> 
> Testing
> -------
> 
> On OSX:
> `make check`
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>


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