Re: Review Request 56623: Implemented 'AuthenticationContext' in libprocess.

[Alexander Rojas]

> On Feb. 27, 2017, 12:58 p.m., Alexander Rojas wrote:
> > 3rdparty/libprocess/include/process/authenticator.hpp, line 62
> > 
> >
> > `static` keyword doesn't have that much use in a header.
> 
> Greg Mann wrote:
> In this context, the `static` keyword gives the function internal 
> linkage. I could also use an anonymous namespace; I don't have a strong 
> preference for one or the other.

Anonymous namespace seems to be the preferred way.


> On Feb. 27, 2017, 12:58 p.m., Alexander Rojas wrote:
> > 3rdparty/libprocess/include/process/authenticator.hpp, lines 65-72
> > 
> >
> > I would prefer if either the definition or the whole function could be 
> > moved to `authenticator.cpp`
> 
> Greg Mann wrote:
> In some of Jan's upcoming patches, he's moving 'authenticator.cpp' to 
> 'basic_authenticator.cpp'. I left this definition in the header because it 
> didn't seem worth it to me to maintain the file 'authenticator.cpp' just for 
> this function, but perhaps we should?

I would prefer that, or at least a TODO mentioning that once stuff from 
`authenticator.cpp` goes into `basic_authenticator.cpp` this should go with it. 
Perhaps we even need some stuff in `authenticator.cpp` and this is a prime 
candidate.


- Alexander


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


On Feb. 28, 2017, 7:13 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56623/
> ---
> 
> (Updated Feb. 28, 2017, 7:13 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 adds a new struct, `Principal`, to libprocess
> to represent an authenticated entity in the system.
> The new type contains a string `value` and a map containing
> arbitrary key-value pairs.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/authenticator.hpp 
> e5489c6cb4adc8a822e7dd4515542618c36136f9 
>   3rdparty/libprocess/include/process/http.hpp 
> eb2c87dee2a911085592e0e14a7499d526ad0bfc 
>   3rdparty/libprocess/include/process/process.hpp 
> ceedbe86ccba5c24bf1e9e7f032d7dbcf6acb0f3 
>   3rdparty/libprocess/src/authenticator.cpp 
> cfedb6f7674e0f6690e77a633cdd1bd494c7d2c7 
>   3rdparty/libprocess/src/authenticator_manager.cpp 
> a22acd026a001788dc39b8005a56577e33c6800b 
>   3rdparty/libprocess/src/process.cpp 
> 3ad485f9136cd9edf0a6db76ff1f475039236391 
> 
> Diff: https://reviews.apache.org/r/56623/diff/
> 
> 
> Testing
> ---
> 
> Testing information can be found at the end of this review chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 56623: Implemented 'AuthenticationContext' in libprocess.

[Greg Mann]

> On Feb. 27, 2017, 11:58 a.m., Alexander Rojas wrote:
> > 3rdparty/libprocess/include/process/authenticator.hpp, line 62
> > 
> >
> > `static` keyword doesn't have that much use in a header.

In this context, the `static` keyword gives the function internal linkage. I 
could also use an anonymous namespace; I don't have a strong preference for one 
or the other.


> On Feb. 27, 2017, 11:58 a.m., Alexander Rojas wrote:
> > 3rdparty/libprocess/include/process/authenticator.hpp, lines 65-72
> > 
> >
> > I would prefer if either the definition or the whole function could be 
> > moved to `authenticator.cpp`

In some of Jan's upcoming patches, he's moving 'authenticator.cpp' to 
'basic_authenticator.cpp'. I left this definition in the header because it 
didn't seem worth it to me to maintain the file 'authenticator.cpp' just for 
this function, but perhaps we should?


- Greg


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


On Feb. 28, 2017, 6:13 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56623/
> ---
> 
> (Updated Feb. 28, 2017, 6:13 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 adds a new struct, `Principal`, to libprocess
> to represent an authenticated entity in the system.
> The new type contains a string `value` and a map containing
> arbitrary key-value pairs.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/authenticator.hpp 
> e5489c6cb4adc8a822e7dd4515542618c36136f9 
>   3rdparty/libprocess/include/process/http.hpp 
> eb2c87dee2a911085592e0e14a7499d526ad0bfc 
>   3rdparty/libprocess/include/process/process.hpp 
> ceedbe86ccba5c24bf1e9e7f032d7dbcf6acb0f3 
>   3rdparty/libprocess/src/authenticator.cpp 
> cfedb6f7674e0f6690e77a633cdd1bd494c7d2c7 
>   3rdparty/libprocess/src/authenticator_manager.cpp 
> a22acd026a001788dc39b8005a56577e33c6800b 
>   3rdparty/libprocess/src/process.cpp 
> 3ad485f9136cd9edf0a6db76ff1f475039236391 
> 
> Diff: https://reviews.apache.org/r/56623/diff/
> 
> 
> Testing
> ---
> 
> Testing information can be found at the end of this review chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 56623: Implemented 'AuthenticationContext' in libprocess.

[Greg Mann]

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

(Updated Feb. 28, 2017, 6:13 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 (updated)
---

This patch adds a new struct, `Principal`, to libprocess
to represent an authenticated entity in the system.
The new type contains a string `value` and a map containing
arbitrary key-value pairs.


Diffs
-

  3rdparty/libprocess/include/process/authenticator.hpp 
e5489c6cb4adc8a822e7dd4515542618c36136f9 
  3rdparty/libprocess/include/process/http.hpp 
eb2c87dee2a911085592e0e14a7499d526ad0bfc 
  3rdparty/libprocess/include/process/process.hpp 
ceedbe86ccba5c24bf1e9e7f032d7dbcf6acb0f3 
  3rdparty/libprocess/src/authenticator.cpp 
cfedb6f7674e0f6690e77a633cdd1bd494c7d2c7 
  3rdparty/libprocess/src/authenticator_manager.cpp 
a22acd026a001788dc39b8005a56577e33c6800b 
  3rdparty/libprocess/src/process.cpp 3ad485f9136cd9edf0a6db76ff1f475039236391 

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


Testing
---

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


Thanks,

Greg Mann

Re: Review Request 56623: Implemented 'AuthenticationContext' in libprocess.

[Greg Mann]

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

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


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


Changes
---

Changed 'AuthenticationContext' to 'Principal'.


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


Repository: mesos


Description
---

This patch adds a new struct, `AuthenticationContext`, to
libprocess to represent an authenticated entity in the system.
The new type contains a `principal` and a map containing
arbitrary key-value pairs.


Diffs (updated)
-

  3rdparty/libprocess/include/process/authenticator.hpp 
e5489c6cb4adc8a822e7dd4515542618c36136f9 
  3rdparty/libprocess/include/process/http.hpp 
eb2c87dee2a911085592e0e14a7499d526ad0bfc 
  3rdparty/libprocess/include/process/process.hpp 
ceedbe86ccba5c24bf1e9e7f032d7dbcf6acb0f3 
  3rdparty/libprocess/src/authenticator.cpp 
cfedb6f7674e0f6690e77a633cdd1bd494c7d2c7 
  3rdparty/libprocess/src/authenticator_manager.cpp 
a22acd026a001788dc39b8005a56577e33c6800b 
  3rdparty/libprocess/src/process.cpp 3ad485f9136cd9edf0a6db76ff1f475039236391 

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


Testing
---

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


Thanks,

Greg Mann

Re: Review Request 56623: Implemented 'AuthenticationContext' in libprocess.

[Alexander Rojas]

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




3rdparty/libprocess/include/process/authenticator.hpp (line 62)


`static` keyword doesn't have that much use in a header.



3rdparty/libprocess/include/process/authenticator.hpp (lines 65 - 72)


I would prefer if either the definition or the whole function could be 
moved to `authenticator.cpp`



3rdparty/libprocess/include/process/authenticator.hpp (lines 100 - 101)


Any reason we cannot do this in this patchset?


- Alexander Rojas


On Feb. 24, 2017, 1:05 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56623/
> ---
> 
> (Updated Feb. 24, 2017, 1:05 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 adds a new struct, `AuthenticationContext`, to
> libprocess to represent an authenticated entity in the system.
> The new type contains a `principal` and a map containing
> arbitrary key-value pairs.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/authenticator.hpp 
> e5489c6cb4adc8a822e7dd4515542618c36136f9 
>   3rdparty/libprocess/include/process/http.hpp 
> 3a406963c1defc037d58971f2400e40855503fbb 
>   3rdparty/libprocess/include/process/process.hpp 
> ceedbe86ccba5c24bf1e9e7f032d7dbcf6acb0f3 
>   3rdparty/libprocess/src/authenticator.cpp 
> cfedb6f7674e0f6690e77a633cdd1bd494c7d2c7 
>   3rdparty/libprocess/src/authenticator_manager.cpp 
> a22acd026a001788dc39b8005a56577e33c6800b 
>   3rdparty/libprocess/src/process.cpp 
> 3ad485f9136cd9edf0a6db76ff1f475039236391 
> 
> Diff: https://reviews.apache.org/r/56623/diff/
> 
> 
> Testing
> ---
> 
> Testing information can be found at the end of this review chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 56623: Implemented 'AuthenticationContext' in libprocess.

[Greg Mann]

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

(Updated Feb. 24, 2017, 12:05 a.m.)


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


Changes
---

Updated comments in the code, added reference to a cleanup JIRA.


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


Repository: mesos


Description
---

This patch adds a new struct, `AuthenticationContext`, to
libprocess to represent an authenticated entity in the system.
The new type contains a `principal` and a map containing
arbitrary key-value pairs.


Diffs (updated)
-

  3rdparty/libprocess/include/process/authenticator.hpp 
e5489c6cb4adc8a822e7dd4515542618c36136f9 
  3rdparty/libprocess/include/process/http.hpp 
3a406963c1defc037d58971f2400e40855503fbb 
  3rdparty/libprocess/include/process/process.hpp 
ceedbe86ccba5c24bf1e9e7f032d7dbcf6acb0f3 
  3rdparty/libprocess/src/authenticator.cpp 
cfedb6f7674e0f6690e77a633cdd1bd494c7d2c7 
  3rdparty/libprocess/src/authenticator_manager.cpp 
a22acd026a001788dc39b8005a56577e33c6800b 
  3rdparty/libprocess/src/process.cpp 3ad485f9136cd9edf0a6db76ff1f475039236391 

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


Testing
---

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


Thanks,

Greg Mann

Re: Review Request 56623: Implemented 'AuthenticationContext' in libprocess.

[Vinod Kone]

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


Fix it, then Ship it!





3rdparty/libprocess/include/process/process.hpp (lines 303 - 305)


can you rephrase this comment per discussion?


- Vinod Kone


On Feb. 22, 2017, 1:14 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56623/
> ---
> 
> (Updated Feb. 22, 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 adds a new struct, `AuthenticationContext`, to
> libprocess to represent an authenticated entity in the system.
> The new type contains a `principal` and a map containing
> arbitrary key-value pairs.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/authenticator.hpp 
> e5489c6cb4adc8a822e7dd4515542618c36136f9 
>   3rdparty/libprocess/include/process/http.hpp 
> 3a406963c1defc037d58971f2400e40855503fbb 
>   3rdparty/libprocess/include/process/process.hpp 
> ceedbe86ccba5c24bf1e9e7f032d7dbcf6acb0f3 
>   3rdparty/libprocess/src/authenticator.cpp 
> cfedb6f7674e0f6690e77a633cdd1bd494c7d2c7 
>   3rdparty/libprocess/src/authenticator_manager.cpp 
> a22acd026a001788dc39b8005a56577e33c6800b 
>   3rdparty/libprocess/src/process.cpp 
> 3ad485f9136cd9edf0a6db76ff1f475039236391 
> 
> Diff: https://reviews.apache.org/r/56623/diff/
> 
> 
> Testing
> ---
> 
> Testing information can be found at the end of this review chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 56623: Implemented 'AuthenticationContext' in libprocess.

[Greg Mann]

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

(Updated Feb. 22, 2017, 1:14 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 adds a new struct, `AuthenticationContext`, to
libprocess to represent an authenticated entity in the system.
The new type contains a `principal` and a map containing
arbitrary key-value pairs.


Diffs (updated)
-

  3rdparty/libprocess/include/process/authenticator.hpp 
e5489c6cb4adc8a822e7dd4515542618c36136f9 
  3rdparty/libprocess/include/process/http.hpp 
3a406963c1defc037d58971f2400e40855503fbb 
  3rdparty/libprocess/include/process/process.hpp 
ceedbe86ccba5c24bf1e9e7f032d7dbcf6acb0f3 
  3rdparty/libprocess/src/authenticator.cpp 
cfedb6f7674e0f6690e77a633cdd1bd494c7d2c7 
  3rdparty/libprocess/src/authenticator_manager.cpp 
a22acd026a001788dc39b8005a56577e33c6800b 
  3rdparty/libprocess/src/process.cpp 3ad485f9136cd9edf0a6db76ff1f475039236391 

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


Testing
---

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


Thanks,

Greg Mann

Re: Review Request 56623: Implemented 'AuthenticationContext' in libprocess.

[Adam B]

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



Looks good for a foundation, but I haven't looked at the rest of the patch 
chain yet


3rdparty/libprocess/include/process/authenticator.hpp (line 36)


"claims" or "labels"? I kinda like "claims"


- Adam B


On Feb. 17, 2017, 6:54 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56623/
> ---
> 
> (Updated Feb. 17, 2017, 6:54 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 adds a new struct, `AuthenticationContext`, to
> libprocess to represent an authenticated entity in the system.
> The new type contains a `principal` and a map containing
> arbitrary key-value pairs.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/authenticator.hpp 
> e5489c6cb4adc8a822e7dd4515542618c36136f9 
>   3rdparty/libprocess/include/process/http.hpp 
> 3a406963c1defc037d58971f2400e40855503fbb 
>   3rdparty/libprocess/include/process/process.hpp 
> ceedbe86ccba5c24bf1e9e7f032d7dbcf6acb0f3 
>   3rdparty/libprocess/src/authenticator.cpp 
> cfedb6f7674e0f6690e77a633cdd1bd494c7d2c7 
>   3rdparty/libprocess/src/authenticator_manager.cpp 
> a22acd026a001788dc39b8005a56577e33c6800b 
>   3rdparty/libprocess/src/http.cpp 0e37936eec5b2910e54d40788c26f98665cbe1f1 
>   3rdparty/libprocess/src/process.cpp 
> 3ad485f9136cd9edf0a6db76ff1f475039236391 
> 
> Diff: https://reviews.apache.org/r/56623/diff/
> 
> 
> Testing
> ---
> 
> Testing information can be found at the end of this review chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 56623: Implemented 'AuthenticationContext' in libprocess.

[Greg Mann]

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

(Updated Feb. 18, 2017, 2:54 a.m.)


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


Changes
---

Made use of `jsonify` in `operator<<`.


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


Repository: mesos


Description
---

This patch adds a new struct, `AuthenticationContext`, to
libprocess to represent an authenticated entity in the system.
The new type contains a `principal` and a map containing
arbitrary key-value pairs.


Diffs (updated)
-

  3rdparty/libprocess/include/process/authenticator.hpp 
e5489c6cb4adc8a822e7dd4515542618c36136f9 
  3rdparty/libprocess/include/process/http.hpp 
3a406963c1defc037d58971f2400e40855503fbb 
  3rdparty/libprocess/include/process/process.hpp 
ceedbe86ccba5c24bf1e9e7f032d7dbcf6acb0f3 
  3rdparty/libprocess/src/authenticator.cpp 
cfedb6f7674e0f6690e77a633cdd1bd494c7d2c7 
  3rdparty/libprocess/src/authenticator_manager.cpp 
a22acd026a001788dc39b8005a56577e33c6800b 
  3rdparty/libprocess/src/http.cpp 0e37936eec5b2910e54d40788c26f98665cbe1f1 
  3rdparty/libprocess/src/process.cpp 3ad485f9136cd9edf0a6db76ff1f475039236391 

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


Testing
---

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


Thanks,

Greg Mann

Re: Review Request 56623: Implemented 'AuthenticationContext' in libprocess.

[Greg Mann]

> On Feb. 16, 2017, 10:45 a.m., Alexander Rojas wrote:
> > 3rdparty/libprocess/src/http.cpp, lines 87-109
> > 
> >
> > Why not going the fancy way and use `jsonify()`?
> 
> Alexander Rojas wrote:
> Just add this function in the same namespace as the 
> `AuthorizationContext`:
> 
> ```c++
> static void json(JSON::ObjectWriter* writer, const AuthorizationContext& 
> context)
> {
>   if (context.principal.isSome()) {
> writer->field("principal", context.principal.get());
>   }
>   if (context.labels.isSome()) {
> writer->field("labels", context.labels.get());
>   }
> }
> ```
> 
> Then you can implement the output stream operator as:
> 
> ```c++
> std::ostream& operator<<(
> std::ostream& stream, const AuthenticationContext& context)
> {
>   stream << jsonify(context);
>   return streaml
> }
> ```

Awesome, thx Alexander!!


- Greg


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


On Feb. 17, 2017, 10:33 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56623/
> ---
> 
> (Updated Feb. 17, 2017, 10:33 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 adds a new struct, `AuthenticationContext`, to
> libprocess to represent an authenticated entity in the system.
> The new type contains a `principal` and a map containing
> arbitrary key-value pairs.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/authenticator.hpp 
> e5489c6cb4adc8a822e7dd4515542618c36136f9 
>   3rdparty/libprocess/include/process/http.hpp 
> 3a406963c1defc037d58971f2400e40855503fbb 
>   3rdparty/libprocess/include/process/process.hpp 
> ceedbe86ccba5c24bf1e9e7f032d7dbcf6acb0f3 
>   3rdparty/libprocess/src/authenticator.cpp 
> cfedb6f7674e0f6690e77a633cdd1bd494c7d2c7 
>   3rdparty/libprocess/src/authenticator_manager.cpp 
> a22acd026a001788dc39b8005a56577e33c6800b 
>   3rdparty/libprocess/src/http.cpp 0e37936eec5b2910e54d40788c26f98665cbe1f1 
>   3rdparty/libprocess/src/process.cpp 
> 3ad485f9136cd9edf0a6db76ff1f475039236391 
> 
> Diff: https://reviews.apache.org/r/56623/diff/
> 
> 
> Testing
> ---
> 
> Testing information can be found at the end of this review chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 56623: Implemented 'AuthenticationContext' in libprocess.

[Greg Mann]

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

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


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


Changes
---

Added a comment.


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


Repository: mesos


Description
---

This patch adds a new struct, `AuthenticationContext`, to
libprocess to represent an authenticated entity in the system.
The new type contains a `principal` and a map containing
arbitrary key-value pairs.


Diffs (updated)
-

  3rdparty/libprocess/include/process/authenticator.hpp 
e5489c6cb4adc8a822e7dd4515542618c36136f9 
  3rdparty/libprocess/include/process/http.hpp 
3a406963c1defc037d58971f2400e40855503fbb 
  3rdparty/libprocess/include/process/process.hpp 
ceedbe86ccba5c24bf1e9e7f032d7dbcf6acb0f3 
  3rdparty/libprocess/src/authenticator.cpp 
cfedb6f7674e0f6690e77a633cdd1bd494c7d2c7 
  3rdparty/libprocess/src/authenticator_manager.cpp 
a22acd026a001788dc39b8005a56577e33c6800b 
  3rdparty/libprocess/src/http.cpp 0e37936eec5b2910e54d40788c26f98665cbe1f1 
  3rdparty/libprocess/src/process.cpp 3ad485f9136cd9edf0a6db76ff1f475039236391 

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


Testing
---

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


Thanks,

Greg Mann

Re: Review Request 56623: Implemented 'AuthenticationContext' in libprocess.

[Alexander Rojas]

> On Feb. 16, 2017, 11:45 a.m., Alexander Rojas wrote:
> > 3rdparty/libprocess/src/http.cpp, lines 87-109
> > 
> >
> > Why not going the fancy way and use `jsonify()`?

Just add this function in the same namespace as the `AuthorizationContext`:

```c++
static void json(JSON::ObjectWriter* writer, const AuthorizationContext& 
context)
{
  if (context.principal.isSome()) {
writer->field("principal", context.principal.get());
  }
  if (context.labels.isSome()) {
writer->field("labels", context.labels.get());
  }
}
```

Then you can implement the output stream operator as:

```c++
std::ostream& operator<<(
std::ostream& stream, const AuthenticationContext& context)
{
  stream << jsonify(context);
  return streaml
}
```


- Alexander


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


On Feb. 17, 2017, 6:31 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56623/
> ---
> 
> (Updated Feb. 17, 2017, 6:31 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 adds a new struct, `AuthenticationContext`, to
> libprocess to represent an authenticated entity in the system.
> The new type contains a `principal` and a map containing
> arbitrary key-value pairs.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/authenticator.hpp 
> e5489c6cb4adc8a822e7dd4515542618c36136f9 
>   3rdparty/libprocess/include/process/http.hpp 
> 3a406963c1defc037d58971f2400e40855503fbb 
>   3rdparty/libprocess/include/process/process.hpp 
> ceedbe86ccba5c24bf1e9e7f032d7dbcf6acb0f3 
>   3rdparty/libprocess/src/authenticator.cpp 
> cfedb6f7674e0f6690e77a633cdd1bd494c7d2c7 
>   3rdparty/libprocess/src/authenticator_manager.cpp 
> a22acd026a001788dc39b8005a56577e33c6800b 
>   3rdparty/libprocess/src/http.cpp 0e37936eec5b2910e54d40788c26f98665cbe1f1 
>   3rdparty/libprocess/src/process.cpp 
> 3ad485f9136cd9edf0a6db76ff1f475039236391 
> 
> Diff: https://reviews.apache.org/r/56623/diff/
> 
> 
> Testing
> ---
> 
> Testing information can be found at the end of this review chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 56623: Implemented 'AuthenticationContext' in libprocess.

[Greg Mann]

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

(Updated Feb. 17, 2017, 5:31 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 adds a new struct, `AuthenticationContext`, to
libprocess to represent an authenticated entity in the system.
The new type contains a `principal` and a map containing
arbitrary key-value pairs.


Diffs (updated)
-

  3rdparty/libprocess/include/process/authenticator.hpp 
e5489c6cb4adc8a822e7dd4515542618c36136f9 
  3rdparty/libprocess/include/process/http.hpp 
3a406963c1defc037d58971f2400e40855503fbb 
  3rdparty/libprocess/include/process/process.hpp 
ceedbe86ccba5c24bf1e9e7f032d7dbcf6acb0f3 
  3rdparty/libprocess/src/authenticator.cpp 
cfedb6f7674e0f6690e77a633cdd1bd494c7d2c7 
  3rdparty/libprocess/src/authenticator_manager.cpp 
a22acd026a001788dc39b8005a56577e33c6800b 
  3rdparty/libprocess/src/http.cpp 0e37936eec5b2910e54d40788c26f98665cbe1f1 
  3rdparty/libprocess/src/process.cpp 3ad485f9136cd9edf0a6db76ff1f475039236391 

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


Testing
---

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


Thanks,

Greg Mann

Re: Review Request 56623: Implemented 'AuthenticationContext' in libprocess.

[Greg Mann]

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

(Updated Feb. 17, 2017, 5:20 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 adds a new struct, `AuthenticationContext`, to
libprocess to represent an authenticated entity in the system.
The new type contains a `principal` and a map containing
arbitrary key-value pairs.


Diffs (updated)
-

  3rdparty/libprocess/include/process/authenticator.hpp 
e5489c6cb4adc8a822e7dd4515542618c36136f9 
  3rdparty/libprocess/include/process/http.hpp 
3a406963c1defc037d58971f2400e40855503fbb 
  3rdparty/libprocess/include/process/process.hpp 
ceedbe86ccba5c24bf1e9e7f032d7dbcf6acb0f3 
  3rdparty/libprocess/src/authenticator.cpp 
cfedb6f7674e0f6690e77a633cdd1bd494c7d2c7 
  3rdparty/libprocess/src/authenticator_manager.cpp 
a22acd026a001788dc39b8005a56577e33c6800b 
  3rdparty/libprocess/src/http.cpp 0e37936eec5b2910e54d40788c26f98665cbe1f1 
  3rdparty/libprocess/src/process.cpp 3ad485f9136cd9edf0a6db76ff1f475039236391 

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


Testing
---

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


Thanks,

Greg Mann

Re: Review Request 56623: Implemented 'AuthenticationContext' in libprocess.

[Jan Schlicht]

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




3rdparty/libprocess/include/process/http.hpp (lines 75 - 78)


Make this a const function.

You could also make this a free function, to have better ordering.


- Jan Schlicht


On Feb. 14, 2017, 12:23 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56623/
> ---
> 
> (Updated Feb. 14, 2017, 12:23 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 adds a new struct, `AuthenticationContext`, to
> libprocess to represent an authenticated entity in the system.
> The new type contains a `principal` and a map containing
> arbitrary key-value pairs.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/authenticator.hpp 
> e5489c6cb4adc8a822e7dd4515542618c36136f9 
>   3rdparty/libprocess/include/process/http.hpp 
> 3a406963c1defc037d58971f2400e40855503fbb 
>   3rdparty/libprocess/include/process/process.hpp 
> ceedbe86ccba5c24bf1e9e7f032d7dbcf6acb0f3 
>   3rdparty/libprocess/src/authenticator.cpp 
> cfedb6f7674e0f6690e77a633cdd1bd494c7d2c7 
>   3rdparty/libprocess/src/authenticator_manager.cpp 
> a22acd026a001788dc39b8005a56577e33c6800b 
>   3rdparty/libprocess/src/http.cpp 0e37936eec5b2910e54d40788c26f98665cbe1f1 
>   3rdparty/libprocess/src/process.cpp 
> 3ad485f9136cd9edf0a6db76ff1f475039236391 
> 
> Diff: https://reviews.apache.org/r/56623/diff/
> 
> 
> Testing
> ---
> 
> Testing information can be found at the end of this review chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 56623: Implemented 'AuthenticationContext' in libprocess.

[Alexander Rojas]

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




3rdparty/libprocess/include/process/http.hpp (line 22)


This include is not needed since `iosfwd` already includes forward 
declarations for `std::ostream`.

So I would say, unless you actually use it here, stick to the forward 
declaration.



3rdparty/libprocess/include/process/http.hpp (line 68)


In order to keep things localized, I would prefer this declaration to be in 
`3rdparty/libprocess/include/process/authenticator.hpp`



3rdparty/libprocess/include/process/http.hpp (line 123)


for lambda declarations you don't need to name the parameters, so I tend to 
omit them if their meaning is not clear from the context, but in this case I 
think is clear that it is an `AuthenticationContext`.

Plus you don't name the parameters anywhere else where you use this 
callback type.



3rdparty/libprocess/src/authenticator_manager.cpp (line 100)


Not yours (actually mine) but this message is strange if one doesn't have 
knowledge of the Mesos internals. How about:

```
HTTP Authentication expects either a 'context', an 'unauthorized' response 
or a 'forbidden' response to be set.
```



3rdparty/libprocess/src/http.cpp (line 85)


To keep things localized, this is better suited to 
`3rdparty/libprocess/src/authenticator.cpp`



3rdparty/libprocess/src/http.cpp (lines 87 - 109)


Why not going the fancy way and use `jsonify()`?


- Alexander Rojas


On Feb. 14, 2017, 12:23 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56623/
> ---
> 
> (Updated Feb. 14, 2017, 12:23 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 adds a new struct, `AuthenticationContext`, to
> libprocess to represent an authenticated entity in the system.
> The new type contains a `principal` and a map containing
> arbitrary key-value pairs.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/authenticator.hpp 
> e5489c6cb4adc8a822e7dd4515542618c36136f9 
>   3rdparty/libprocess/include/process/http.hpp 
> 3a406963c1defc037d58971f2400e40855503fbb 
>   3rdparty/libprocess/include/process/process.hpp 
> ceedbe86ccba5c24bf1e9e7f032d7dbcf6acb0f3 
>   3rdparty/libprocess/src/authenticator.cpp 
> cfedb6f7674e0f6690e77a633cdd1bd494c7d2c7 
>   3rdparty/libprocess/src/authenticator_manager.cpp 
> a22acd026a001788dc39b8005a56577e33c6800b 
>   3rdparty/libprocess/src/http.cpp 0e37936eec5b2910e54d40788c26f98665cbe1f1 
>   3rdparty/libprocess/src/process.cpp 
> 3ad485f9136cd9edf0a6db76ff1f475039236391 
> 
> Diff: https://reviews.apache.org/r/56623/diff/
> 
> 
> Testing
> ---
> 
> Testing information can be found at the end of this review chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 56623: Implemented 'AuthenticationContext' in libprocess.

[Jan Schlicht]

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




3rdparty/libprocess/include/process/http.hpp (line 81)


Is the `Option` necessary here? An empty map is IMHO sufficient to cover 
the "no labels set in this context" case.



3rdparty/libprocess/src/authenticator.cpp (line 96)


You can use the `AuthenticationContext(credential[0])` construtor and 
remove the next line.



3rdparty/libprocess/src/authenticator_manager.cpp (line 95)


Please align the `?` in this line with the following ones.


- Jan Schlicht


On Feb. 14, 2017, 12:23 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56623/
> ---
> 
> (Updated Feb. 14, 2017, 12:23 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 adds a new struct, `AuthenticationContext`, to
> libprocess to represent an authenticated entity in the system.
> The new type contains a `principal` and a map containing
> arbitrary key-value pairs.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/authenticator.hpp 
> e5489c6cb4adc8a822e7dd4515542618c36136f9 
>   3rdparty/libprocess/include/process/http.hpp 
> 3a406963c1defc037d58971f2400e40855503fbb 
>   3rdparty/libprocess/include/process/process.hpp 
> ceedbe86ccba5c24bf1e9e7f032d7dbcf6acb0f3 
>   3rdparty/libprocess/src/authenticator.cpp 
> cfedb6f7674e0f6690e77a633cdd1bd494c7d2c7 
>   3rdparty/libprocess/src/authenticator_manager.cpp 
> a22acd026a001788dc39b8005a56577e33c6800b 
>   3rdparty/libprocess/src/http.cpp 0e37936eec5b2910e54d40788c26f98665cbe1f1 
>   3rdparty/libprocess/src/process.cpp 
> 3ad485f9136cd9edf0a6db76ff1f475039236391 
> 
> Diff: https://reviews.apache.org/r/56623/diff/
> 
> 
> Testing
> ---
> 
> Testing information can be found at the end of this review chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>