Fix policy_groups policy authorization to pull from the correct org

[jkeiser]

The case that fails:

1. Upload `/organizations/A/policy_groups/foo/policies/bar
2. Upload `/organizations/B/policy_groups/foo/policies/bar
3. Access /organizations/A/policy_groups/foo/policies/bar from a client in A
4. Access /organizations/B/policy_groups/foo/policies/bar from a client in B

We don't have Pedant multi-org tests yet, so this is hard to add to Pedant.

[markan]

Looks reasonable to me.

[marcparadise]

Wow, nice find. I can help you out setting up a multi-org test, there are a couple of examples kicking around in there.

[stevendanna]

👍 Let's make sure we do get those regression tests soon though.

[marcparadise]

Is this ready to go?

[stevendanna]

@marcparadise Only question is pedant tests for this.

[marcparadise]

@jkeiser I know we talked about testing this, and I think we were able to get tests straightened out. Do you recall any details around that?

[tylercloke]

We definitely have multiorg tests: https://github.com/chef/chef-server/blob/master/oc-chef-pedant/spec/api/keys/user_keys_spec.rb#L459-L471

Should probably just write some pedant tests for this.

[stevendanna]

@marcparadise @tylercloke Thoughts on a plan forward here. In my reading this is seriously incorrect behavior. My vote is to either rebase and merge OR make an explicit card for SPOOL to finish the tests if there isn't one already.

[coderanger]

Just bumping this on the behalf of Bloomberg, caused multiple days of confusion for us :-( I think it's already been escalated internally but public vis++.

[mcole]

+1! We hit this one too, spent a day ferreting through the code / database to work out what was going on.

[coderanger]

Travis failure looked unrelated, likely transient VM fell down.

[danielsdeleo]

I'm 👍 on merging and we can add testing the the SPOOL backlog alongside

[marcparadise]

Also 👍. I'll rebase and run through CI, then go ahead with the merge.

[johnbellone]

👍 Let's merge this!

[stevendanna]

Merged. @chef/chef-server-maintainers I think we should hold a short post-mortem on why this wasn't merged for so long and how we can have a faster turn around in the future. This was a security related patch that went unmerged for too long in my opinion.

[jkeiser]

For my part, I waited because I hadn't added tests; I hadn't added tests because I would have had to modify test infrastructure more than I had time for; and then the patch simply dropped off my radar. Sorry about that!

[stevendanna]

Reviewing some chat logs, it looks like the recent history is more or less this:

• We all agreed to merge this without tests on 3 Aug. (following a customer-escalated ticket that we tracked down to this problem) but at the time of the conversation we weren't sure this fix was the complete fix.
• Noah reported that this fix did indeed work, I reported that to the channel, but failed to merge it (I think assuming that SPOOL was on it because they were handling some build-related churn at the time)

I think the take aways for me are:

• In future we need to assign a specific maintainer to continue to shepherd security-related patches through.
• Regardless of our recent fumbling, in my opinion we should have provided support to @jkeiser to get the tests written or wrote the tests ourselves shortly after this was submitted.

FWIW, I'm happy to use this ticket as a discussion for follow-up actions rather than doing a realtime post-mortem since I know schedules can be difficult.