You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Greg Mann <gr...@mesosphere.io> on 2017/02/14 00:14:54 UTC

Review Request 56619: Updated Mesos handlers to use 'AuthenticationContext'.

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

Review request for mesos, Adam B, Alexander Rojas, Jan Schlicht, Till Toenshoff, and Vinod Kone.


Bugs: MESOS-7003
    https://issues.apache.org/jira/browse/MESOS-7003


Repository: mesos


Description
-------

This patch updates the HTTP endpoint handlers in Mesos
to accept an `AuthenticationContext` instead of an
`Option<string>& principal`.


Diffs
-----

  src/files/files.hpp 8cffc26fc7d674187e55663f23f1e10bed40229e 
  src/files/files.cpp 8327f8002fbfa3be77a4bbe4aa83a73d0f170f7a 
  src/master/http.cpp c5324abc0db82275fd65d3f7d361ad8ee9e017d1 
  src/master/master.hpp 764adb18ddf09b62529c5c96f8e4dfaf7803483e 
  src/master/master.cpp dff320acd1a13995d51cc0c303edea0cdfaa336d 
  src/master/quota_handler.cpp 3ad28e4a9363a877d0610b529a6c17fb30ece37a 
  src/master/registrar.cpp d7134eea34102ab7b24d2f0131363bdd9005cfd3 
  src/master/weights_handler.cpp da0b995e1866e7eb57b8ef885864c6c6d66405d6 
  src/slave/http.cpp ccd489d700a605dc41037e00dbd6a7cb3b6edcc6 
  src/slave/slave.hpp 3b0aea4e3e9a17501077beccbccaab4abbe11af2 
  src/slave/slave.cpp ebba8e16bc9ec45781183e78cb5a3c351a5f65f5 

Diff: https://reviews.apache.org/r/56619/diff/


Testing
-------

Testing information can be found at the end of this review chain.


Thanks,

Greg Mann

Re: Review Request 56619: Updated Mesos handlers to use 'AuthenticationContext'.

Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56619/#review165509
-----------------------------------------------------------



This look great already. But I'll need more time to deeply review this, e.g. I'll do another round, this are the few things I found while quickly looking over the patch.


src/files/files.cpp (line 800)
<https://reviews.apache.org/r/56619/#comment237351>

    Remove the space between `context` and `)`.



src/master/master.cpp (line 1613)
<https://reviews.apache.org/r/56619/#comment237349>

    Check for `context.isSome()` first.



src/master/weights_handler.cpp (line 369)
<https://reviews.apache.org/r/56619/#comment237350>

    Remove `<< context.get()` here.


- Jan Schlicht


On Feb. 14, 2017, 1:14 a.m., Greg Mann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56619/
> -----------------------------------------------------------
> 
> (Updated Feb. 14, 2017, 1:14 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Jan Schlicht, Till Toenshoff, and Vinod Kone.
> 
> 
> Bugs: MESOS-7003
>     https://issues.apache.org/jira/browse/MESOS-7003
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This patch updates the HTTP endpoint handlers in Mesos
> to accept an `AuthenticationContext` instead of an
> `Option<string>& principal`.
> 
> 
> Diffs
> -----
> 
>   src/files/files.hpp 8cffc26fc7d674187e55663f23f1e10bed40229e 
>   src/files/files.cpp 8327f8002fbfa3be77a4bbe4aa83a73d0f170f7a 
>   src/master/http.cpp c5324abc0db82275fd65d3f7d361ad8ee9e017d1 
>   src/master/master.hpp 764adb18ddf09b62529c5c96f8e4dfaf7803483e 
>   src/master/master.cpp dff320acd1a13995d51cc0c303edea0cdfaa336d 
>   src/master/quota_handler.cpp 3ad28e4a9363a877d0610b529a6c17fb30ece37a 
>   src/master/registrar.cpp d7134eea34102ab7b24d2f0131363bdd9005cfd3 
>   src/master/weights_handler.cpp da0b995e1866e7eb57b8ef885864c6c6d66405d6 
>   src/slave/http.cpp ccd489d700a605dc41037e00dbd6a7cb3b6edcc6 
>   src/slave/slave.hpp 3b0aea4e3e9a17501077beccbccaab4abbe11af2 
>   src/slave/slave.cpp ebba8e16bc9ec45781183e78cb5a3c351a5f65f5 
> 
> Diff: https://reviews.apache.org/r/56619/diff/
> 
> 
> Testing
> -------
> 
> Testing information can be found at the end of this review chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 56619: Updated 'Files' handlers to use the 'Principal' type.

Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56619/#review167901
-----------------------------------------------------------


Ship it!




Ship It!

- Vinod Kone


On Feb. 28, 2017, 6:32 p.m., Greg Mann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56619/
> -----------------------------------------------------------
> 
> (Updated Feb. 28, 2017, 6:32 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Jan Schlicht, Till Toenshoff, and Vinod Kone.
> 
> 
> Bugs: MESOS-7003
>     https://issues.apache.org/jira/browse/MESOS-7003
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This patch updates the HTTP endpoint handlers in the
> Mesos `Files` process to accept the `Principal` type
> instead of an `Option<string>& principal`.
> 
> 
> Diffs
> -----
> 
>   src/files/files.hpp 8cffc26fc7d674187e55663f23f1e10bed40229e 
>   src/files/files.cpp 8327f8002fbfa3be77a4bbe4aa83a73d0f170f7a 
> 
> 
> Diff: https://reviews.apache.org/r/56619/diff/8/
> 
> 
> Testing
> -------
> 
> Testing information can be found at the end of this review chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 56619: Updated 'Files' handlers to use the 'Principal' type.

Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56619/
-----------------------------------------------------------

(Updated Feb. 28, 2017, 6:32 p.m.)


Review request for mesos, Adam B, Alexander Rojas, Jan Schlicht, Till Toenshoff, and Vinod Kone.


Bugs: MESOS-7003
    https://issues.apache.org/jira/browse/MESOS-7003


Repository: mesos


Description
-------

This patch updates the HTTP endpoint handlers in the
Mesos `Files` process to accept the `Principal` type
instead of an `Option<string>& principal`.


Diffs (updated)
-----

  src/files/files.hpp 8cffc26fc7d674187e55663f23f1e10bed40229e 
  src/files/files.cpp 8327f8002fbfa3be77a4bbe4aa83a73d0f170f7a 

Diff: https://reviews.apache.org/r/56619/diff/


Testing
-------

Testing information can be found at the end of this review chain.


Thanks,

Greg Mann

Re: Review Request 56619: Updated 'Files' handlers to use the 'Principal' type.

Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56619/
-----------------------------------------------------------

(Updated Feb. 28, 2017, 6:34 a.m.)


Review request for mesos, Adam B, Alexander Rojas, Jan Schlicht, Till Toenshoff, and Vinod Kone.


Changes
-------

Changed 'AuthenticationContext' to 'Principal'.


Summary (updated)
-----------------

Updated 'Files' handlers to use the 'Principal' type.


Bugs: MESOS-7003
    https://issues.apache.org/jira/browse/MESOS-7003


Repository: mesos


Description (updated)
-------

This patch updates the HTTP endpoint handlers in the
Mesos `Files` process to accept the `Principal` type
instead of an `Option<string>& principal`.


Diffs (updated)
-----

  src/files/files.hpp 8cffc26fc7d674187e55663f23f1e10bed40229e 
  src/files/files.cpp 8327f8002fbfa3be77a4bbe4aa83a73d0f170f7a 

Diff: https://reviews.apache.org/r/56619/diff/


Testing
-------

Testing information can be found at the end of this review chain.


Thanks,

Greg Mann

Re: Review Request 56619: Updated 'Files' handlers to use 'AuthenticationContext'.

Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56619/#review166883
-----------------------------------------------------------


Ship it!




Ship It!

- Alexander Rojas


On Feb. 25, 2017, 12:06 a.m., Greg Mann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56619/
> -----------------------------------------------------------
> 
> (Updated Feb. 25, 2017, 12:06 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Jan Schlicht, Till Toenshoff, and Vinod Kone.
> 
> 
> Bugs: MESOS-7003
>     https://issues.apache.org/jira/browse/MESOS-7003
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This patch updates the HTTP endpoint handlers in the
> Mesos `Files` process to accept an `AuthenticationContext`
> instead of an `Option<string>& principal`.
> 
> 
> Diffs
> -----
> 
>   src/files/files.hpp 8cffc26fc7d674187e55663f23f1e10bed40229e 
>   src/files/files.cpp 8327f8002fbfa3be77a4bbe4aa83a73d0f170f7a 
> 
> Diff: https://reviews.apache.org/r/56619/diff/
> 
> 
> Testing
> -------
> 
> Testing information can be found at the end of this review chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 56619: Updated 'Files' handlers to use 'AuthenticationContext'.

Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56619/
-----------------------------------------------------------

(Updated Feb. 24, 2017, 11:06 p.m.)


Review request for mesos, Adam B, Alexander Rojas, Jan Schlicht, Till Toenshoff, and Vinod Kone.


Bugs: MESOS-7003
    https://issues.apache.org/jira/browse/MESOS-7003


Repository: mesos


Description
-------

This patch updates the HTTP endpoint handlers in the
Mesos `Files` process to accept an `AuthenticationContext`
instead of an `Option<string>& principal`.


Diffs (updated)
-----

  src/files/files.hpp 8cffc26fc7d674187e55663f23f1e10bed40229e 
  src/files/files.cpp 8327f8002fbfa3be77a4bbe4aa83a73d0f170f7a 

Diff: https://reviews.apache.org/r/56619/diff/


Testing
-------

Testing information can be found at the end of this review chain.


Thanks,

Greg Mann

Re: Review Request 56619: Updated 'Files' handlers to use 'AuthenticationContext'.

Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56619/#review166634
-----------------------------------------------------------


Ship it!




Ship It!

- Vinod Kone


On Feb. 22, 2017, 1:17 a.m., Greg Mann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56619/
> -----------------------------------------------------------
> 
> (Updated Feb. 22, 2017, 1:17 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Jan Schlicht, Till Toenshoff, and Vinod Kone.
> 
> 
> Bugs: MESOS-7003
>     https://issues.apache.org/jira/browse/MESOS-7003
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This patch updates the HTTP endpoint handlers in the
> Mesos `Files` process to accept an `AuthenticationContext`
> instead of an `Option<string>& principal`.
> 
> 
> Diffs
> -----
> 
>   src/files/files.hpp 8cffc26fc7d674187e55663f23f1e10bed40229e 
>   src/files/files.cpp 8327f8002fbfa3be77a4bbe4aa83a73d0f170f7a 
> 
> Diff: https://reviews.apache.org/r/56619/diff/
> 
> 
> Testing
> -------
> 
> Testing information can be found at the end of this review chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 56619: Updated 'Files' handlers to use 'AuthenticationContext'.

Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56619/
-----------------------------------------------------------

(Updated Feb. 22, 2017, 1:17 a.m.)


Review request for mesos, Adam B, Alexander Rojas, Jan Schlicht, Till Toenshoff, and Vinod Kone.


Changes
-------

Changed `context` to `authContext`.


Bugs: MESOS-7003
    https://issues.apache.org/jira/browse/MESOS-7003


Repository: mesos


Description
-------

This patch updates the HTTP endpoint handlers in the
Mesos `Files` process to accept an `AuthenticationContext`
instead of an `Option<string>& principal`.


Diffs (updated)
-----

  src/files/files.hpp 8cffc26fc7d674187e55663f23f1e10bed40229e 
  src/files/files.cpp 8327f8002fbfa3be77a4bbe4aa83a73d0f170f7a 

Diff: https://reviews.apache.org/r/56619/diff/


Testing
-------

Testing information can be found at the end of this review chain.


Thanks,

Greg Mann

Re: Review Request 56619: Updated 'Files' handlers to use 'AuthenticationContext'.

Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56619/#review166119
-----------------------------------------------------------


Ship it!




Ship It!

- Adam B


On Feb. 17, 2017, 7 p.m., Greg Mann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56619/
> -----------------------------------------------------------
> 
> (Updated Feb. 17, 2017, 7 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Jan Schlicht, Till Toenshoff, and Vinod Kone.
> 
> 
> Bugs: MESOS-7003
>     https://issues.apache.org/jira/browse/MESOS-7003
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This patch updates the HTTP endpoint handlers in the
> Mesos `Files` process to accept an `AuthenticationContext`
> instead of an `Option<string>& principal`.
> 
> 
> Diffs
> -----
> 
>   src/files/files.hpp 8cffc26fc7d674187e55663f23f1e10bed40229e 
>   src/files/files.cpp 8327f8002fbfa3be77a4bbe4aa83a73d0f170f7a 
> 
> Diff: https://reviews.apache.org/r/56619/diff/
> 
> 
> Testing
> -------
> 
> Testing information can be found at the end of this review chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 56619: Updated 'Files' handlers to use 'AuthenticationContext'.

Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56619/
-----------------------------------------------------------

(Updated Feb. 18, 2017, 3 a.m.)


Review request for mesos, Adam B, Alexander Rojas, Jan Schlicht, Till Toenshoff, and Vinod Kone.


Changes
-------

Split endpoint changes into 3 separate patches.


Summary (updated)
-----------------

Updated 'Files' handlers to use 'AuthenticationContext'.


Bugs: MESOS-7003
    https://issues.apache.org/jira/browse/MESOS-7003


Repository: mesos


Description (updated)
-------

This patch updates the HTTP endpoint handlers in the
Mesos `Files` process to accept an `AuthenticationContext`
instead of an `Option<string>& principal`.


Diffs (updated)
-----

  src/files/files.hpp 8cffc26fc7d674187e55663f23f1e10bed40229e 
  src/files/files.cpp 8327f8002fbfa3be77a4bbe4aa83a73d0f170f7a 

Diff: https://reviews.apache.org/r/56619/diff/


Testing
-------

Testing information can be found at the end of this review chain.


Thanks,

Greg Mann

Re: Review Request 56619: Updated Mesos handlers to use 'AuthenticationContext'.

Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56619/
-----------------------------------------------------------

(Updated Feb. 17, 2017, 10:36 p.m.)


Review request for mesos, Adam B, Alexander Rojas, Jan Schlicht, Till Toenshoff, and Vinod Kone.


Bugs: MESOS-7003
    https://issues.apache.org/jira/browse/MESOS-7003


Repository: mesos


Description
-------

This patch updates the HTTP endpoint handlers in Mesos
to accept an `AuthenticationContext` instead of an
`Option<string>& principal`.


Diffs (updated)
-----

  src/files/files.hpp 8cffc26fc7d674187e55663f23f1e10bed40229e 
  src/files/files.cpp 8327f8002fbfa3be77a4bbe4aa83a73d0f170f7a 
  src/master/http.cpp e2fd71c5ae4178564b9a08756df5175aec5d6ca1 
  src/master/master.hpp 764adb18ddf09b62529c5c96f8e4dfaf7803483e 
  src/master/master.cpp 5534674fa2740b0a6e37fc1df34b582f1e691133 
  src/master/quota_handler.cpp 3ad28e4a9363a877d0610b529a6c17fb30ece37a 
  src/master/registrar.cpp d7134eea34102ab7b24d2f0131363bdd9005cfd3 
  src/master/weights_handler.cpp da0b995e1866e7eb57b8ef885864c6c6d66405d6 
  src/slave/http.cpp af70b6f294a04f23b04cd1d8c36c1c3e86d7d5e6 
  src/slave/slave.hpp 3b0aea4e3e9a17501077beccbccaab4abbe11af2 
  src/slave/slave.cpp ebba8e16bc9ec45781183e78cb5a3c351a5f65f5 

Diff: https://reviews.apache.org/r/56619/diff/


Testing
-------

Testing information can be found at the end of this review chain.


Thanks,

Greg Mann

Re: Review Request 56619: Updated Mesos handlers to use 'AuthenticationContext'.

Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56619/
-----------------------------------------------------------

(Updated Feb. 17, 2017, 6:03 a.m.)


Review request for mesos, Adam B, Alexander Rojas, Jan Schlicht, Till Toenshoff, and Vinod Kone.


Changes
-------

Addressed comments.


Bugs: MESOS-7003
    https://issues.apache.org/jira/browse/MESOS-7003


Repository: mesos


Description
-------

This patch updates the HTTP endpoint handlers in Mesos
to accept an `AuthenticationContext` instead of an
`Option<string>& principal`.


Diffs (updated)
-----

  src/files/files.hpp 8cffc26fc7d674187e55663f23f1e10bed40229e 
  src/files/files.cpp 8327f8002fbfa3be77a4bbe4aa83a73d0f170f7a 
  src/master/http.cpp e2fd71c5ae4178564b9a08756df5175aec5d6ca1 
  src/master/master.hpp 764adb18ddf09b62529c5c96f8e4dfaf7803483e 
  src/master/master.cpp 5534674fa2740b0a6e37fc1df34b582f1e691133 
  src/master/quota_handler.cpp 3ad28e4a9363a877d0610b529a6c17fb30ece37a 
  src/master/registrar.cpp d7134eea34102ab7b24d2f0131363bdd9005cfd3 
  src/master/weights_handler.cpp da0b995e1866e7eb57b8ef885864c6c6d66405d6 
  src/slave/http.cpp af70b6f294a04f23b04cd1d8c36c1c3e86d7d5e6 
  src/slave/slave.hpp 3b0aea4e3e9a17501077beccbccaab4abbe11af2 
  src/slave/slave.cpp ebba8e16bc9ec45781183e78cb5a3c351a5f65f5 

Diff: https://reviews.apache.org/r/56619/diff/


Testing
-------

Testing information can be found at the end of this review chain.


Thanks,

Greg Mann

Re: Review Request 56619: Updated Mesos handlers to use 'AuthenticationContext'.

Posted by Greg Mann <gr...@mesosphere.io>.

> On Feb. 16, 2017, 3:56 p.m., Alexander Rojas wrote:
> > src/master/http.cpp, lines 796-798
> > <https://reviews.apache.org/r/56619/diff/1/?file=1632590#file1632590line796>
> >
> >     This appears here, there and everywhere, that I'm considering it may be a good idea to abstract it to something like:
> >     
> >     ```
> >     static Option<string> AuthenticationContext::extractPrincipal(const Option<AuthenticationContext>& context);
> >     ```
> >     
> >     That will make it easier if one day we need to modify how the extraction works.
> 
> Greg Mann wrote:
>     Good call, will do.

I think I would actually prefer to update the validation code to accept the `AuthenticationContext` instead of adding this helper method.


> On Feb. 16, 2017, 3:56 p.m., Alexander Rojas wrote:
> > src/master/http.cpp, lines 1479-1481
> > <https://reviews.apache.org/r/56619/diff/1/?file=1632590#file1632590line1479>
> >
> >     likewise, this looks like a prime candidate to be abstracted away.

As I suggested in https://reviews.apache.org/r/56618/, let's avoid passing default-initialized `Subject`s at all, and instead use a helper which returns `Option<authorization::Subject>`.


- Greg


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


On Feb. 17, 2017, 10:36 p.m., Greg Mann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56619/
> -----------------------------------------------------------
> 
> (Updated Feb. 17, 2017, 10:36 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Jan Schlicht, Till Toenshoff, and Vinod Kone.
> 
> 
> Bugs: MESOS-7003
>     https://issues.apache.org/jira/browse/MESOS-7003
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This patch updates the HTTP endpoint handlers in Mesos
> to accept an `AuthenticationContext` instead of an
> `Option<string>& principal`.
> 
> 
> Diffs
> -----
> 
>   src/files/files.hpp 8cffc26fc7d674187e55663f23f1e10bed40229e 
>   src/files/files.cpp 8327f8002fbfa3be77a4bbe4aa83a73d0f170f7a 
>   src/master/http.cpp e2fd71c5ae4178564b9a08756df5175aec5d6ca1 
>   src/master/master.hpp 764adb18ddf09b62529c5c96f8e4dfaf7803483e 
>   src/master/master.cpp 5534674fa2740b0a6e37fc1df34b582f1e691133 
>   src/master/quota_handler.cpp 3ad28e4a9363a877d0610b529a6c17fb30ece37a 
>   src/master/registrar.cpp d7134eea34102ab7b24d2f0131363bdd9005cfd3 
>   src/master/weights_handler.cpp da0b995e1866e7eb57b8ef885864c6c6d66405d6 
>   src/slave/http.cpp af70b6f294a04f23b04cd1d8c36c1c3e86d7d5e6 
>   src/slave/slave.hpp 3b0aea4e3e9a17501077beccbccaab4abbe11af2 
>   src/slave/slave.cpp ebba8e16bc9ec45781183e78cb5a3c351a5f65f5 
> 
> Diff: https://reviews.apache.org/r/56619/diff/
> 
> 
> Testing
> -------
> 
> Testing information can be found at the end of this review chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 56619: Updated Mesos handlers to use 'AuthenticationContext'.

Posted by Greg Mann <gr...@mesosphere.io>.

> On Feb. 16, 2017, 3:56 p.m., Alexander Rojas wrote:
> > I personally find this patch way too big, so at the end I found myself just going through the changeset, perhaps breaking it into the _master_, _agent_ and _files_ components will help reviewers. What do you think?

Good idea, I'll split this up.


> On Feb. 16, 2017, 3:56 p.m., Alexander Rojas wrote:
> > src/master/http.cpp, line 520
> > <https://reviews.apache.org/r/56619/diff/1/?file=1632590#file1632590line520>
> >
> >     Any readon why `validation::master::call::validate(...)` cannot take the whole context as parameter?

I considered changing the signatures of the validation helpers as well, but ended up leaving them as-is, since they only require the principal currently. However, it would be less brittle to pass the context instead. Perhaps I'll post separate patches for the validation code, since these reviews are already so large.


> On Feb. 16, 2017, 3:56 p.m., Alexander Rojas wrote:
> > src/master/http.cpp, lines 796-798
> > <https://reviews.apache.org/r/56619/diff/1/?file=1632590#file1632590line796>
> >
> >     This appears here, there and everywhere, that I'm considering it may be a good idea to abstract it to something like:
> >     
> >     ```
> >     static Option<string> AuthenticationContext::extractPrincipal(const Option<AuthenticationContext>& context);
> >     ```
> >     
> >     That will make it easier if one day we need to modify how the extraction works.

Good call, will do.


> On Feb. 16, 2017, 3:56 p.m., Alexander Rojas wrote:
> > src/master/weights_handler.cpp, line 52
> > <https://reviews.apache.org/r/56619/diff/1/?file=1632595#file1632595line52>
> >
> >     Not sure if using `using` here is the way to go, we don't seem to use it for `http::Request`. This goes with my idea of just using `http::authentication::Context` as the name type.

I left the name as-is for the moment, but am open to changing it if we come up with something better. Let's consider our options and I can do a sweep and update all the reviews if we decide to change it.


- Greg


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


On Feb. 17, 2017, 6:03 a.m., Greg Mann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56619/
> -----------------------------------------------------------
> 
> (Updated Feb. 17, 2017, 6:03 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Jan Schlicht, Till Toenshoff, and Vinod Kone.
> 
> 
> Bugs: MESOS-7003
>     https://issues.apache.org/jira/browse/MESOS-7003
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This patch updates the HTTP endpoint handlers in Mesos
> to accept an `AuthenticationContext` instead of an
> `Option<string>& principal`.
> 
> 
> Diffs
> -----
> 
>   src/files/files.hpp 8cffc26fc7d674187e55663f23f1e10bed40229e 
>   src/files/files.cpp 8327f8002fbfa3be77a4bbe4aa83a73d0f170f7a 
>   src/master/http.cpp e2fd71c5ae4178564b9a08756df5175aec5d6ca1 
>   src/master/master.hpp 764adb18ddf09b62529c5c96f8e4dfaf7803483e 
>   src/master/master.cpp 5534674fa2740b0a6e37fc1df34b582f1e691133 
>   src/master/quota_handler.cpp 3ad28e4a9363a877d0610b529a6c17fb30ece37a 
>   src/master/registrar.cpp d7134eea34102ab7b24d2f0131363bdd9005cfd3 
>   src/master/weights_handler.cpp da0b995e1866e7eb57b8ef885864c6c6d66405d6 
>   src/slave/http.cpp af70b6f294a04f23b04cd1d8c36c1c3e86d7d5e6 
>   src/slave/slave.hpp 3b0aea4e3e9a17501077beccbccaab4abbe11af2 
>   src/slave/slave.cpp ebba8e16bc9ec45781183e78cb5a3c351a5f65f5 
> 
> Diff: https://reviews.apache.org/r/56619/diff/
> 
> 
> Testing
> -------
> 
> Testing information can be found at the end of this review chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 56619: Updated Mesos handlers to use 'AuthenticationContext'.

Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56619/#review165838
-----------------------------------------------------------



I personally find this patch way too big, so at the end I found myself just going through the changeset, perhaps breaking it into the _master_, _agent_ and _files_ components will help reviewers. What do you think?


src/files/files.hpp (line 89)
<https://reviews.apache.org/r/56619/#comment237673>

    Could you indent this more... or less... not sure how, but it is hard to tell wether `authorized` is part of the type declaration or the name of the parameter.



src/files/files.cpp (line 188)
<https://reviews.apache.org/r/56619/#comment237684>

    Same here, it is not clear that `authorizations` is not part of type. Although this is what clang format returns I think either:
    
    ```c++
    hashmap<
          string,
          lambda::function<Future<bool>(const Option<AuthenticationContext>&)>>
              authorizations;
    ```
    
    or
    
    ```c++
    hashmap<
          string,
          lambda::function<Future<bool>(const Option<AuthenticationContext>&)>
        > authorizations;
    ```
    
    are more readable.



src/master/http.cpp (line 520)
<https://reviews.apache.org/r/56619/#comment237685>

    Any readon why `validation::master::call::validate(...)` cannot take the whole context as parameter?



src/master/http.cpp (lines 655 - 657)
<https://reviews.apache.org/r/56619/#comment237686>

    Note to self: This may be a bug that violates the semantics of `Subject`, write a test.



src/master/http.cpp (lines 795 - 797)
<https://reviews.apache.org/r/56619/#comment237687>

    This appears here, there and everywhere, that I'm considering it may be a good idea to abstract it to something like:
    
    ```
    static Option<string> AuthenticationContext::extractPrincipal(const Option<AuthenticationContext>& context);
    ```
    
    That will make it easier if one day we need to modify how the extraction works.



src/master/http.cpp (lines 1477 - 1479)
<https://reviews.apache.org/r/56619/#comment237689>

    likewise, this looks like a prime candidate to be abstracted away.



src/master/weights_handler.cpp (line 52)
<https://reviews.apache.org/r/56619/#comment237692>

    Not sure if using `using` here is the way to go, we don't seem to use it for `http::Request`. This goes with my idea of just using `http::authentication::Context` as the name type.


- Alexander Rojas


On Feb. 14, 2017, 1:14 a.m., Greg Mann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56619/
> -----------------------------------------------------------
> 
> (Updated Feb. 14, 2017, 1:14 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Jan Schlicht, Till Toenshoff, and Vinod Kone.
> 
> 
> Bugs: MESOS-7003
>     https://issues.apache.org/jira/browse/MESOS-7003
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This patch updates the HTTP endpoint handlers in Mesos
> to accept an `AuthenticationContext` instead of an
> `Option<string>& principal`.
> 
> 
> Diffs
> -----
> 
>   src/files/files.hpp 8cffc26fc7d674187e55663f23f1e10bed40229e 
>   src/files/files.cpp 8327f8002fbfa3be77a4bbe4aa83a73d0f170f7a 
>   src/master/http.cpp c5324abc0db82275fd65d3f7d361ad8ee9e017d1 
>   src/master/master.hpp 764adb18ddf09b62529c5c96f8e4dfaf7803483e 
>   src/master/master.cpp dff320acd1a13995d51cc0c303edea0cdfaa336d 
>   src/master/quota_handler.cpp 3ad28e4a9363a877d0610b529a6c17fb30ece37a 
>   src/master/registrar.cpp d7134eea34102ab7b24d2f0131363bdd9005cfd3 
>   src/master/weights_handler.cpp da0b995e1866e7eb57b8ef885864c6c6d66405d6 
>   src/slave/http.cpp ccd489d700a605dc41037e00dbd6a7cb3b6edcc6 
>   src/slave/slave.hpp 3b0aea4e3e9a17501077beccbccaab4abbe11af2 
>   src/slave/slave.cpp ebba8e16bc9ec45781183e78cb5a3c351a5f65f5 
> 
> Diff: https://reviews.apache.org/r/56619/diff/
> 
> 
> Testing
> -------
> 
> Testing information can be found at the end of this review chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>