Re: Review Request 58096: Added authorization for frameworks in /roles endpoint.

[Jay Guo]

> On May 17, 2017, 4:08 p.m., Adam B wrote:
> > src/master/http.cpp
> > Lines 3524 (patched)
> > 
> >
> > `futures` is an over-vague variable name, especially since neither are 
> > Futures by this point. Can we do better?

I'm running out of vocabulary here... two very different thing to be captured 
in one word. Maybe `collection`?


- Jay


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


On June 22, 2017, 2 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58096/
> ---
> 
> (Updated June 22, 2017, 2 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7260
> https://issues.apache.org/jira/browse/MESOS-7260
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> While /roles displays a list of frameworksIds that register with
> a role, it did NOT filter them based on VIEW_FRAMEWORK ACL, which
> impose a security risk. This patch fixed this issue by taking a
> frameworksApprover in `Master::Http::roles()` which is used to
> filter framework IDs.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 4dd43fd7c3fb986f4eed78bce574b6d3af156b67 
> 
> 
> Diff: https://reviews.apache.org/r/58096/diff/8/
> 
> 
> Testing
> ---
> 
> see next patch in the chain.
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 58096: Added authorization for frameworks in /roles endpoint.

[Jay Guo]

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

(Updated June 22, 2017, 2 a.m.)


Review request for mesos, Adam B, Alexander Rojas, and Benjamin Mahler.


Changes
---

rebase & address comments


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


Repository: mesos


Description
---

While /roles displays a list of frameworksIds that register with
a role, it did NOT filter them based on VIEW_FRAMEWORK ACL, which
impose a security risk. This patch fixed this issue by taking a
frameworksApprover in `Master::Http::roles()` which is used to
filter framework IDs.


Diffs (updated)
-

  src/master/http.cpp 4dd43fd7c3fb986f4eed78bce574b6d3af156b67 


Diff: https://reviews.apache.org/r/58096/diff/8/

Changes: https://reviews.apache.org/r/58096/diff/7-8/


Testing
---

see next patch in the chain.


Thanks,

Jay Guo

Re: Review Request 58096: Added authorization for frameworks in /roles endpoint.

[Adam B]

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



Looks pretty good to me. Just a few minor comments


src/master/http.cpp
Lines 3504 (patched)


What makes a role "active"? Having `active` frameworks registered for that 
role? This function seems to return a list of all roles that have one or more 
weights, quota, reservations, or registered frameworks associated with them. 
More accurate would be to call it `knownRoles`.



src/master/http.cpp
Lines 3507 (patched)


Nit: `ObjectApprover` singular, since it's a single approver that can 
approve/deny multiple frameworks, not an approver per framework.



src/master/http.cpp
Lines 3524 (patched)


`futures` is an over-vague variable name, especially since neither are 
Futures by this point. Can we do better?


- Adam B


On May 10, 2017, 9:33 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58096/
> ---
> 
> (Updated May 10, 2017, 9:33 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7260
> https://issues.apache.org/jira/browse/MESOS-7260
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> While /roles displays a list of frameworksIds that register with
> a role, it did NOT filter them based on VIEW_FRAMEWORK ACL, which
> impose a security risk. This patch fixed this issue by taking a
> frameworksApprover in `Master::Http::roles()` which is used to
> filter framework IDs.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp e2590a17044ac019b24a24629428d4ec8adc0c31 
> 
> 
> Diff: https://reviews.apache.org/r/58096/diff/7/
> 
> 
> Testing
> ---
> 
> see next patch in the chain.
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 58096: Added authorization for frameworks in /roles endpoint.

[Alexander Rojas]

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


Ship it!




Ship It!

- Alexander Rojas


On May 10, 2017, 6:33 p.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58096/
> ---
> 
> (Updated May 10, 2017, 6:33 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7260
> https://issues.apache.org/jira/browse/MESOS-7260
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> While /roles displays a list of frameworksIds that register with
> a role, it did NOT filter them based on VIEW_FRAMEWORK ACL, which
> impose a security risk. This patch fixed this issue by taking a
> frameworksApprover in `Master::Http::roles()` which is used to
> filter framework IDs.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp e2590a17044ac019b24a24629428d4ec8adc0c31 
> 
> 
> Diff: https://reviews.apache.org/r/58096/diff/7/
> 
> 
> Testing
> ---
> 
> see next patch in the chain.
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 58096: Added authorization for frameworks in /roles endpoint.

[Jay Guo]

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

(Updated May 11, 2017, 12:33 a.m.)


Review request for mesos, Adam B, Alexander Rojas, and Benjamin Mahler.


Changes
---

rebase


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


Repository: mesos


Description
---

While /roles displays a list of frameworksIds that register with
a role, it did NOT filter them based on VIEW_FRAMEWORK ACL, which
impose a security risk. This patch fixed this issue by taking a
frameworksApprover in `Master::Http::roles()` which is used to
filter framework IDs.


Diffs (updated)
-

  src/master/http.cpp e2590a17044ac019b24a24629428d4ec8adc0c31 


Diff: https://reviews.apache.org/r/58096/diff/7/

Changes: https://reviews.apache.org/r/58096/diff/6-7/


Testing
---

see next patch in the chain.


Thanks,

Jay Guo

Re: Review Request 58096: Added authorization for frameworks in /roles endpoint.

[Jay Guo]

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

(Updated May 10, 2017, 9:46 p.m.)


Review request for mesos, Adam B, Alexander Rojas, and Benjamin Mahler.


Changes
---

address comments.


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


Repository: mesos


Description
---

While /roles displays a list of frameworksIds that register with
a role, it did NOT filter them based on VIEW_FRAMEWORK ACL, which
impose a security risk. This patch fixed this issue by taking a
frameworksApprover in `Master::Http::roles()` which is used to
filter framework IDs.


Diffs (updated)
-

  src/master/http.cpp e2590a17044ac019b24a24629428d4ec8adc0c31 


Diff: https://reviews.apache.org/r/58096/diff/6/

Changes: https://reviews.apache.org/r/58096/diff/5-6/


Testing
---

see next patch in the chain.


Thanks,

Jay Guo

Re: Review Request 58096: Added authorization for frameworks in /roles endpoint.

[Alexander Rojas]

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




src/master/http.cpp
Line 406 (original), 412-418 (patched)


More compact and readable if you do:

```c++
if (approveViewFrameworkInfo(frameworkApprover, framework->info)) {
  writer->element(frameworkId.value());
}
```



src/master/http.cpp
Lines 422 (patched)


I think this comment should be in the previous patch.


- Alexander Rojas


On May 8, 2017, 9:56 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58096/
> ---
> 
> (Updated May 8, 2017, 9:56 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7260
> https://issues.apache.org/jira/browse/MESOS-7260
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> While /roles displays a list of frameworksIds that register with
> a role, it did NOT filter them based on VIEW_FRAMEWORK ACL, which
> impose a security risk. This patch fixed this issue by taking a
> frameworksApprover in `Master::Http::roles()` which is used to
> filter framework IDs.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp e2590a17044ac019b24a24629428d4ec8adc0c31 
> 
> 
> Diff: https://reviews.apache.org/r/58096/diff/5/
> 
> 
> Testing
> ---
> 
> see next patch in the chain.
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 58096: Added authorization for frameworks in /roles endpoint.

[Jay Guo]

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

(Updated May 8, 2017, 3:56 p.m.)


Review request for mesos, Adam B, Alexander Rojas, and Benjamin Mahler.


Changes
---

rebase


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


Repository: mesos


Description
---

While /roles displays a list of frameworksIds that register with
a role, it did NOT filter them based on VIEW_FRAMEWORK ACL, which
impose a security risk. This patch fixed this issue by taking a
frameworksApprover in `Master::Http::roles()` which is used to
filter framework IDs.


Diffs (updated)
-

  src/master/http.cpp e2590a17044ac019b24a24629428d4ec8adc0c31 


Diff: https://reviews.apache.org/r/58096/diff/5/

Changes: https://reviews.apache.org/r/58096/diff/4-5/


Testing
---

see next patch in the chain.


Thanks,

Jay Guo

Re: Review Request 58096: Added authorization for frameworks in /roles endpoint.

[Jay Guo]

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

(Updated April 19, 2017, 11:37 a.m.)


Review request for mesos, Adam B, Alexander Rojas, and Benjamin Mahler.


Changes
---

rebase


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


Repository: mesos


Description
---

While /roles displays a list of frameworksIds that register with
a role, it did NOT filter them based on VIEW_FRAMEWORK ACL, which
impose a security risk. This patch fixed this issue by taking a
frameworksApprover in `Master::Http::roles()` which is used to
filter framework IDs.


Diffs (updated)
-

  src/master/http.cpp 5aae52870451d883ef1ea1fda5a27764d7f318e8 


Diff: https://reviews.apache.org/r/58096/diff/4/

Changes: https://reviews.apache.org/r/58096/diff/3-4/


Testing
---

see next patch in the chain.


Thanks,

Jay Guo

Re: Review Request 58096: Added authorization for frameworks in /roles endpoint.

[Jay Guo]

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

(Updated April 12, 2017, 1:38 p.m.)


Review request for mesos, Adam B, Alexander Rojas, and Benjamin Mahler.


Changes
---

rebase


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


Repository: mesos


Description
---

While /roles displays a list of frameworksIds that register with
a role, it did NOT filter them based on VIEW_FRAMEWORK ACL, which
impose a security risk. This patch fixed this issue by taking a
frameworksApprover in `Master::Http::roles()` which is used to
filter framework IDs.


Diffs (updated)
-

  src/master/http.cpp 0e84d363b346de1ef05101dac066c2f0cf31609d 


Diff: https://reviews.apache.org/r/58096/diff/3/

Changes: https://reviews.apache.org/r/58096/diff/2-3/


Testing
---

see next patch in the chain.


Thanks,

Jay Guo

Re: Review Request 58096: Added authorization for frameworks in /roles endpoint.

[Jay Guo]

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

(Updated April 5, 2017, 1:46 a.m.)


Review request for mesos, Adam B, Alexander Rojas, and Benjamin Mahler.


Changes
---

rebase


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


Repository: mesos


Description
---

While /roles displays a list of frameworksIds that register with
a role, it did NOT filter them based on VIEW_FRAMEWORK ACL, which
impose a security risk. This patch fixed this issue by taking a
frameworksApprover in `Master::Http::roles()` which is used to
filter framework IDs.


Diffs (updated)
-

  src/master/http.cpp 6cf9d350446d1b2d4a6e67d552217daff32657ff 


Diff: https://reviews.apache.org/r/58096/diff/2/

Changes: https://reviews.apache.org/r/58096/diff/1-2/


Testing
---

see next patch in the chain.


Thanks,

Jay Guo