KeyError: 'canonical' using scan-merge-proposals when run by non-canonical user

[Parth Malwankar]

[hydrazine]% ./scan-merge-proposals bzr
loaded existing credentials
Traceback (most recent call last):
  File "./scan-merge-proposals", line 87, in <module>
    sys.exit(main(sys.argv))
  File "./scan-merge-proposals", line 54, in main
    team = launchpad.people[team_name]
  File "/usr/lib/pymodules/python2.6/lazr/restfulclient/resource.py", line 860, in __getitem__
    raise KeyError(key)
KeyError: 'canonical'

[Martin Pool (mbp)]

Interesting bug. This code is trying to check if the person is a Canonical staff member, but ~canonical is a private team, so only members can see if another person is a member.

I would like this script to work for people who aren't in ~canonical but I'm not sure what it should do: perhaps just add them to the "may need to sign the agreement" list?

[Parth Malwankar (parthm)]

The docstring in the script does mention.

 * this script will only work when run by members of ~canonical,
   because it's a private team

I thought that might have caused this wasn't sure. I suppose the
easiest fix would be to just give a clearer error message.

I tried running the script after commenting out ~canonical from
AUTHORIZED_TEAMS and the script seems to work fine.
If ~contributor-agreement-canonical is a superset of ~canonical
(is it?) maybe ~contributor-agreement-canonical is enough?

For e.g. the output I got after
AUTHORIZED_TEAMS = [ 'contributor-agreement-canonical' ]

[hydrazine]% ./scan-merge-proposals bzr
loaded existing credentials
https://edge.launchpad.net/1.0/~doxxx/bzr/572092-ignore-dupes/+merge/24543
<<<< yay, a member of contributor-agreement-canonical
    status Needs review

https://edge.launchpad.net/1.0/~gz/bzr/support_OO_flag_installer/+merge/24484
<<<< yay, a member of contributor-agreement-canonical
    status Needs review

https://edge.launchpad.net/1.0/~parthm/bzr/538868-message-for-heavy-checkout/+merge/24483
<<<< yay, a member of contributor-agreement-canonical
    status Needs review

https://edge.launchpad.net/1.0/~bialix/bzr/clean-tree-bzrdir/+merge/24479
<<<< yay, a member of contributor-agreement-canonical
    status Needs review

https://edge.launchpad.net/1.0/~mbp/bzr/491763-transform-rename-failed/+merge/24474
**** mbp not signed up
    status Needs review

https://edge.launchpad.net/1.0/~jameinel/bzr/remove_zlib_dependency/+merge/24452
<<<< yay, a member of contributor-agreement-canonical
    status Needs review

https://edge.launchpad.net/1.0/~bialix/bzr/relpath-docstring/+merge/24442
<<<< yay, a member of contributor-agreement-canonical
    status Approved

https://edge.launchpad.net/1.0/~parthm/bzr/181124-ls-short-opts/+merge/24414
<<<< yay, a member of contributor-agreement-canonical
    status Needs review

https://edge.launchpad.net/1.0/~jbowtie/bzr/fix-515660/+merge/24290
<<<< yay, a member of contributor-agreement-canonical
    status Approved

https://edge.launchpad.net/1.0/~jbowtie/bzr/fix-555439/+merge/24289
<<<< yay, a member of contributor-agreement-canonical
    status Approved

https://edge.launchpad.net/1.0/~jbowtie/bzr/fix-401605/+merge/24284
<<<< yay, a member of contributor-agreement-canonical
    status Needs review

https://edge.launchpad.net/1.0/~parthm/bzr/549310-mandatory-whoami/+merge/24244
<<<< yay, a member of contributor-agreement-canonical
    status Needs review

https://edge.launchpad.net/1.0/~mbp/bzr/testsubjects-old/+merge/24188
     mbp already needs to sign
    status Needs review

https://edge.launchpad.net/1.0/~garyvdm/bzr/diff_using_gui/+merge/24076
<<<< yay, a member of contributor-agreement-canonical
    status Needs review

https://edge.launchpad.net/1.0/~abentley/bzr/transform-commit-full/+merge/24049
**** abentley not signed up
    status Approved

https://edge.launchpad.net/1.0/~gagern/bzr/encodingSafeTests/+merge/23925
<<<< yay, a member of contributor-agreement-canonical
    status Needs review

https://edge.launchpad.net/1.0/~gagern/bzr/bug560030-include-bash-completion-plugin/+merge/23912
<<<< yay, a member of contributor-agreement-canonical
    s...

[Martin Pool (mbp)]

On 3 May 2010 12:39, Parth Malwankar <email address hidden> wrote:
> The docstring in the script does mention.
>
>  * this script will only work when run by members of ~canonical,
>   because it's a private team
>
> I thought that might have caused this wasn't sure. I suppose the
> easiest fix would be to just give a clearer error message.
>
> I tried running the script after commenting out ~canonical from
> AUTHORIZED_TEAMS and the script seems to work fine.
> If ~contributor-agreement-canonical is a superset of ~canonical
> (is it?) maybe ~contributor-agreement-canonical is enough?

It's not a superset: you can see in your test run that I am not in
~contributor-agreement-canonical. In fact all of the three people
listed are staff. (I don't need to be because I have a separate
employment contract with Canonical that covers copyright etc.)

I think even if it was a member of ~c-a-c that would not be enough,
because you still wouldn't be able to see into the private team.
(imbw.)

Perhaps if you're not allowed to see that team (ie you get a KeyError)
you should continue without it, but print a message along with the
list at the end.

KeyError seems weird but the general security approach is that if you
can't see a thing, normally you just get "not found" rather than being
told you're not allowed. This has pros and cons but for now it's
reasonable that the api is consistent with the web ui.

--
Martin <http://launchpad.net/~mbp/>

[Parth Malwankar (parthm)]

On Mon, May 3, 2010 at 8:24 AM, Martin Pool <email address hidden> wrote:
> On 3 May 2010 12:39, Parth Malwankar <email address hidden> wrote:
>> The docstring in the script does mention.
>>
>>  * this script will only work when run by members of ~canonical,
>>   because it's a private team
>>
>> I thought that might have caused this wasn't sure. I suppose the
>> easiest fix would be to just give a clearer error message.
>>

> Perhaps if you're not allowed to see that team (ie you get a KeyError)
> you should continue without it, but print a message along with the
> list at the end.
>

Sounds like a good solution to me.

Regards,
Parth