branch visibility queries do not consider visibility of stacked on branches

This almost certainly means we have a bug where the logged in query is returning a MP that the security adapters think you can't access. As its a public team, and our defense-in-depth precautions are working, this doesn't imply a security vulnerability.

Did you get an OOPS id on the page?

No OOPS. Here's a screenshot.

I am not sure if this is about a private branch or a private team. The purple squad has a tasks to ensure private teams work with branches. If issue is about private teams, it might be fixed indirectly. I am adding the disclosure tag to track this issue.

OOPS-38b7c8c2b9576ab5692e2c6adfbc8f37

+activereviews was failing to show a merge proposal for lp:~michael.nelson/software-center-agent/scaclient-add-distroseries-filter into lp:~canonical-isd-hackers/software-center-agent/scaclient. ~canonical-isd-hackers is subscribed to the proposed branch, owns the target branch, and has a requested review on the merge proposal.

The problem is that noodles' branch is stacked on lp:software-center-agent, which used to be lp:~canonical-isd-hackers/software-center-agent/trunk, but was recently moved to lp:~canonical-ca-hackers/software-center-agent/trunk -- its owner was changed. Access transitivity over stacking results in ~canonical-isd-hackers not being able to see the source branch, nor the MP.

To fix the immediate +activereviews issue, subscribe ~canoncial-isd-hackers to lp:software-center-agent. If ISD shouldn't have access any more, the project's branch visibility policies will need tweaking to point at ~canonical-ca-hackers instead.

The bug here is that the visibility checks in LP's branch queries don't include the transitive stacking check. WITH RECURSIVE ftw?

On Tue, Dec 6, 2011 at 2:07 PM, William Grant <email address hidden> wrote:
> +activereviews was failing to show a merge proposal for
> lp:~michael.nelson/software-center-agent/scaclient-add-distroseries-
> filter into lp:~canonical-isd-hackers/software-center-agent/scaclient.
> ~canonical-isd-hackers is subscribed to the proposed branch, owns the
> target branch, and has a requested review on the merge proposal.
>
> The problem is that noodles' branch is stacked on lp:software-center-
> agent, which used to be lp:~canonical-isd-hackers/software-center-
> agent/trunk, but was recently moved to lp:~canonical-ca-hackers
> /software-center-agent/trunk -- its owner was changed. Access
> transitivity over stacking results in ~canonical-isd-hackers not being
> able to see the source branch, nor the MP.
>
> To fix the immediate +activereviews issue, subscribe ~canoncial-isd-
> hackers to lp:software-center-agent. If ISD shouldn't have access any
> more, the project's branch visibility policies will need tweaking to
> point at ~canonical-ca-hackers instead.
>
> The bug here is that the visibility checks in LP's branch queries don't
> include the transitive stacking check. WITH RECURSIVE ftw?

I thought rvba recently overhauled this?

-Rob

Please remember to remove the security contact when you mark a bug no longer security-related.

What I've done recently on this page (+activrevreviews) is preload things to avoid repeated sql queries… looking at the oops, this bug is not related to those modifications.

LP is meant to remove the security contact subscription automatically :(

"The bug here is that the visibility checks in LP's branch queries don't include the transitive stacking check. WITH RECURSIVE ftw?"
This highlights a defect in the denormalisation of is_private: we actually have to scan and include access to all the stacked on branches; just permitting access based on the top level one is insufficient.

This suggests we may which to undo the denormalisation of is_private : but that can be left up to whomever works on this bug.