894537 - Modify ReviewBoard to authenticate against Bugzilla

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Mark Côté [:mcote]]

For experimentation, we want to set up a ReviewBoard server.  For starters, it will be lightly integrated with Bugzilla--just users for now.

There are a couple ways to get user data into Bugzilla:
* Upon request (authenticating, adding reviewers, etc.)
* Copy active users (perhaps only those with edit-bug permission), then set up a push notification for changes.

In either case, authentication should only be against Bugzilla; we should *not* store passwords in the ReviewBoard user table.

Comment 1

[Mark Côté [:mcote]]

Attachment: Bugzilla auth back end, fetch users on request

This implements the first solution.  Going forward, as discussed, it's probably better to do the second, but this will work as is with no other infrastructure (e.g. on bugzilla-dev).

This patch primarily accomplishes the following:
* Authenticates against Bugzilla and logs the user into Bugzilla by preserving the login cookies
* If not already logged into RB, logs in automatically if valid Bugzilla cookies are detected.
* Grabs users from Bugzilla when authenticating, requested by the API (used by autocomplete), and when set as reviewers.
* Bubbles up a 401 error if there's an error talking to Bugzilla when fetching a user.  This isn't handled properly by the UI yet, though.

Comment 2

[Mark Côté [:mcote]]

Oh I should add that the error handler that raises a PermissionDenied exception (later translated to a 401 error) logs the user out automatically, which I think is the sane thing to do.  It's probably pretty rare for a user's session to expire or for them to log out of Bugzilla, which is why I haven't prioritized the UI notification.

Comment 3

[Mark Côté [:mcote]]

Attachment: Bugzilla auth back end

Decided to split this into two patches, for easier digestibility but also because we probably won't need the automatic-user-fetching part when we put this on prod.

While I was at it, I also changed the auth stuff to record the value of can_login as user.is_active, and to prevent inactive/can't-login users from logging into RB (although it won't log them out until they get an error trying to access a Bugzilla user).

Comment 4

[Mark Côté [:mcote]]

Attachment: Always update local db with Bugzilla user data

This part specifically fetches data from Bugzilla every time a user is requested, either via the auth back end (when a user is added to a review) or the API (autocomplete).

Comment 5

[Steven MacLeod [:smacleod]]

Comment on attachment 776700 [details] [diff] [review]
Bugzilla auth back end

Review of attachment 776700 [details] [diff] [review]:
-----------------------------------------------------------------

I did a quick pass and things look good to me. I talked to the RB guys about the things we'd need to make this an extension, and we shouldn't have problems getting patches for them into the Review Board core.

::: contrib/conf/settings_local.py.tmpl
@@ +83,5 @@
>  #SELENIUM_LIVE_SERVER_ADDRESS = "127.0.0.1"
> +
> +BUGZILLA_URL = "https://bugzilla.mozilla.org"
> +BUGZILLA_XMLRPC_URL = BUGZILLA_URL + "/xmlrpc.cgi"
> +BUGZILLA_HTML_LOGIN_URL = BUGZILLA_URL + "/index.cgi?GoAheadAndLogIn=1"

Generally it's frowned upon sticking extra configuration like this in the review board settings.py file. If we can change this to an extension that shouldn't be a problem, we can just have it in the extensions "settings" http://www.reviewboard.org/docs/codebase/dev/extending/extensions/#settings

::: reviewboard/accounts/backends.py
@@ +42,4 @@
>      def authenticate(self, username, password):
>          raise NotImplementedError
>  
> +    def get_or_create_user(self, username, request):

Good news, I talked to the other RB devs and they are open to passing request objects to auth backends in Review Board core. We should be able to break a patch out which does this, and then write this whole patch as an extension.

::: reviewboard/accounts/middleware.py
@@ +17,5 @@
>              except Profile.DoesNotExist:
>                  pass
> +
> +
> +class BugzillaCookieAuthMiddleware(object):

Currently Review Board extensions don't support loading custom middleware, but it's something we'd like to have, so we can get a patch for that into core as well.

::: reviewboard/bugzilla/xmlrpc/transports.py
@@ +16,5 @@
> +    requests.
> +    """
> +
> +    # Inspiration drawn from
> +    # http://blog.godson.in/2010/09/how-to-make-python-xmlrpclib-client.html

This link appears to be dead now.

Comment 6

[Mark Côté [:mcote]]

(In reply to Steven MacLeod [:smacleod] from comment #5)
> Comment on attachment 776700 [details] [diff] [review]
> Bugzilla auth back end
> 
> Review of attachment 776700 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I did a quick pass and things look good to me. I talked to the RB guys about
> the things we'd need to make this an extension, and we shouldn't have
> problems getting patches for them into the Review Board core.

Great!  Thanks for the feedback.

> ::: contrib/conf/settings_local.py.tmpl
> @@ +83,5 @@
> >  #SELENIUM_LIVE_SERVER_ADDRESS = "127.0.0.1"
> > +
> > +BUGZILLA_URL = "https://bugzilla.mozilla.org"
> > +BUGZILLA_XMLRPC_URL = BUGZILLA_URL + "/xmlrpc.cgi"
> > +BUGZILLA_HTML_LOGIN_URL = BUGZILLA_URL + "/index.cgi?GoAheadAndLogIn=1"
> 
> Generally it's frowned upon sticking extra configuration like this in the
> review board settings.py file. If we can change this to an extension that
> shouldn't be a problem, we can just have it in the extensions "settings"
> http://www.reviewboard.org/docs/codebase/dev/extending/extensions/#settings

Sounds good.

> ::: reviewboard/accounts/backends.py
> @@ +42,4 @@
> >      def authenticate(self, username, password):
> >          raise NotImplementedError
> >  
> > +    def get_or_create_user(self, username, request):
> 
> Good news, I talked to the other RB devs and they are open to passing
> request objects to auth backends in Review Board core. We should be able to
> break a patch out which does this, and then write this whole patch as an
> extension.

Excellent, this was the part I was most worried about when it came to making this an extension.  Would you like me to submit this patch?

> ::: reviewboard/accounts/middleware.py
> @@ +17,5 @@
> >              except Profile.DoesNotExist:
> >                  pass
> > +
> > +
> > +class BugzillaCookieAuthMiddleware(object):
> 
> Currently Review Board extensions don't support loading custom middleware,
> but it's something we'd like to have, so we can get a patch for that into
> core as well.

Cool.

> ::: reviewboard/bugzilla/xmlrpc/transports.py
> @@ +16,5 @@
> > +    requests.
> > +    """
> > +
> > +    # Inspiration drawn from
> > +    # http://blog.godson.in/2010/09/how-to-make-python-xmlrpclib-client.html
> 
> This link appears to be dead now.

Hm works for me.  Also this comment is from the original code (as linked in the "Taken from ..." line at the top of it), so even if it were dead I would probably leave it anyway.

Comment 7

[Mark Côté [:mcote]]

Comment on attachment 776700 [details] [diff] [review]
Bugzilla auth back end

I'm working closely with the RB people, so no need for review from BMO folks, at least at the moment.

Comment 8

[Mark Côté [:mcote]]

Got this working in my fork, and now it lives in its own extension at https://github.com/mozilla/rbbz.