Obsolete code-structure/comments are confusing

[Michael Nelson]

There's quite a bit of code and comments that indicating that to moderate reviews a user needs to be a member of a specific Launchpad group (rnr-moderators), but the current implementation of LPAuthorizer.user_can_moderate just checks for membership in the local Django group (and doesn't use a webservice).

I'm assuming the implementation is correct (much nicer than requiring a LP group membership and having to verify via API etc.), but we should remove all the old code and comments (I was quite confused).

If this is the case, we should also remove LPAuthorizer.

[Michael Vogt (mvo)]

My memory is a bit rusty, but I'm pretty sure the way this code works (or is supposed to work) is that the user uses openid for the moderation. So the django app syncs the group membership with launchpad automatically so only a local check against the DB is needed.

[Michael Vogt (mvo)]

The LPAuthorizer in the current form can indeed go. We have two cases where we need authorization:

1. Moderation (moderator verification)
For this we use a webapp anyway and can leverage OpenID with launchpad to get the data synced. AFAIK ubuntu-sso does not have a "group" notion so for moderators we will require a launchpad account. I think that is acceptable.

2. Submit new review (user/reviewer verification)
For this we only require a ubuntu SSO account. Most users will either have the account token stored via ubuntu-sso already or need to create a new account at this point. At this point we need to present "rnr-server" something that it can use to verify that I'm really "Michael Vogt". The current implementation is doing that by forcing the client to send the token over to the rnr-server. With that token the rnr-server makes a API call to launchpad (or ubuntu-sso later) to get name/display_name. This way we ensure that there are no fake reviews. Sending over the token is obviously pretty bad. A possible alternative is to create a request on the client side that is then send to the rnr-server. Together with the oauth sha1/nonce/timestamp feature this should actually be secure as the request can not be re-used (because of the nounce). And all that can be done with it is to ask "what is my name". But IIRC/AFAIK nonce/timestamp is not support (yet) by our infrastructure.

[Michael Nelson (michael.nelson)]

On Fri, Nov 12, 2010 at 2:30 PM, Michael Vogt <email address hidden> wrote:
> The LPAuthorizer in the current form can indeed go. We have two cases
> where we need authorization:

Thanks for the info Michael!

>
> 1. Moderation (moderator verification)
> For this we use a webapp anyway and can leverage OpenID with launchpad to get the data synced. AFAIK ubuntu-sso does not have a "group" notion so for moderators we will require a launchpad account. I think that is acceptable.

I think I'm confused as to why we need to even use Launchpad groups
(or involve launchpad at all). We could simply add people to a local
django 'moderators' group on the server.

http://docs.djangoproject.com/en/1.2/topics/auth/#groups

(which seems to be what the current code is doing - although as you
mentioned, I might have missed the fact that it was expecting a
Launchpad group to be synced locally).

>
> 2. Submit new review (user/reviewer verification)
> For this we only require a ubuntu SSO account. Most users will either have the account token stored via ubuntu-sso already or need to create a new account at this point. At this point we need to present "rnr-server" something that it can use to verify that I'm really "Michael Vogt". The current implementation is doing that by forcing the client to send the token over to the rnr-server. With that token the rnr-server makes a API call to launchpad (or ubuntu-sso later) to get name/display_name. This way we ensure that there are no fake reviews. Sending over the token is obviously pretty bad. A possible alternative is to create a request on the client side that is then send to the rnr-server. Together with the oauth sha1/nonce/timestamp feature this should actually be secure as the request can not be re-used (because of the nounce). And all that can be done with it is to ask "what is my name". But IIRC/AFAIK nonce/timestamp is not support (yet) by our infrastructure.
>

Yes, given that the client has already authenticated with SSO, the
single-signon service should allow you to do a normal authenticated
API request to the rnr-server that the rnr-server verifies with sso
(ie. without the client having to do anything special). I think that's
what Anthony added to the SCA for bug 609025 (which seems to be
solving the same issue for the SCA?)

Also, another option that might be worth considering: move the UI for
writing a review to the browser? So clicking the review link on the
client would open a browser, and if it's the first time, they see "You
are authenticating with Ubuntu Ratings and Reviews as Michael Vogt"
etc., as per normal. Or what are the benefits of doing the review UI
in the client (integrated experience I guess?)

I'd be keener for the former, but if that's not possible...

[Anthony Lenton (elachuni)]

Michael, (sorry I couldn't help myself! :) )

About our two situations
1. Moderation (moderator verification)
Agreed, we've already landed a decorator that verifies the user has a Django permission for bug #674114. We can now assign this permission to users individually, or to Django groups. By using django-openid-auth we can also map LP teams to groups with this permission, so we should have a flexible system for whatever we need down the road, without needing to use LPLib at all.

2. Submit new review (user/reviewer verification)
I'd agree we want to add OAuth authentication to our API, instead of requiring the token to be provided as an argument in our calls. And yup, we can reuse code from SCA here, hopefully that code's going to be split out shortly into a separate project.

As this bug is about obsolete/confusing code I'm going to remove LPAuthorizer in the related branch, and we can then add a bug for protecting the necessary api calls with a token.

About moving the UI for writing a review to the browser, I think the integrated experience is the way we want to go here (so far it seems like we'd be able to avoid an embedded browser for ratings/reviews altogether). But, we could provide a simple view so that people can write a review in a browser if they prefer, it would be quite straight forward afaict. Still I think this is topic for another bug :)