Status

defect
P1
normal
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: mcote, Unassigned)

Tracking

Production
Dependency tree / graph

Details

Attachments

(12 attachments)

39 bytes, text/x-review-board-request
smacleod
: review+
Details
39 bytes, text/x-review-board-request
gps
: review+
Details
39 bytes, text/x-review-board-request
smacleod
: review+
Details
39 bytes, text/x-review-board-request
gps
: review+
Details
40 bytes, text/x-review-board-request
gps
: review+
Details
40 bytes, text/x-review-board-request
dminor
: review+
gps
: review+
smacleod
: review+
Details
40 bytes, text/x-review-board-request
smacleod
: review+
Details
40 bytes, text/x-review-board-request
smacleod
: review+
Details
40 bytes, text/x-review-board-request
gps
: review+
smacleod
: review+
Details
40 bytes, text/x-review-board-request
smacleod
: review+
Details
40 bytes, text/x-review-board-request
smacleod
: review+
Details
40 bytes, text/x-review-board-request
smacleod
: review+
Details
Assignee

Description

5 years ago
It would be nice to be able to log into Review Board via web API using a valid Bugzilla auth token instead of passing Bugzilla login info.  It isn't critical, though, as long as the transport is encrypted, just nice to have.

See http://www.reviewboard.org/docs/manual/dev/webapi/2.0/authenticating/ and https://github.com/djblets/djblets/blob/master/djblets/webapi/auth.py

Note that this presupposes REST support; the current implementation of rbbz uses XMLRPC, since native REST wasn't available when work started.
Assignee

Comment 1

5 years ago
This should probably wait until we have proper API keys in BMO, at some undetermined point in the future.
Assignee

Updated

5 years ago
Keywords: bmo-big
Product: bugzilla.mozilla.org → Developer Services
Assignee

Updated

5 years ago
No longer blocks: 515210
Assignee

Updated

4 years ago
Priority: P2 → P1
Summary: Allow Bugzilla auth tokens to be used with Review Board web API calls → Use BMO API keys
Assignee

Updated

4 years ago
Assignee: nobody → mcote
Status: NEW → ASSIGNED
Assignee

Comment 2

4 years ago
mozreview: User-creation mach command creates API key (Bug 993233). r?smacleod
Attachment #8614311 - Flags: review?(smacleod)
Assignee

Comment 3

4 years ago
mozreview: Add api_key field to BugzillaUserMap (Bug 993233). r?smacleod
Attachment #8614312 - Flags: review?(smacleod)
Assignee

Comment 4

4 years ago
mozreview: View for BMO auth delegation callback (Bug 993233). r?smacleod
Attachment #8614313 - Flags: review?(smacleod)
Assignee

Comment 5

4 years ago
mozreview: Authenticate to BMO with API key (Bug 993233). r?smacleod
Attachment #8614314 - Flags: review?(smacleod)
Comment on attachment 8614311 [details]
MozReview Request: mozreview: Authenticate to BMO with API key (bug 993233). r?smacleod

https://reviewboard.mozilla.org/r/9883/#review8819

::: testing/vcttesting/mozreview_mach_commands.py:163
(Diff revision 1)
> -        print('Created user %s' % u['bugzilla']['id'])
> +        print('Created user %s with API key %s' % (u['bugzilla']['id'],
> +                                                   u['bugzilla']['api_key']))

Will this be deterministic? if not it's going to cause serious issues with the test output - Also, this should be changing the test output but you're not including the test updates in this commit - please make sure tests pass on every commit.
Attachment #8614311 - Flags: review?(smacleod)
Attachment #8614312 - Flags: review?(smacleod)
Comment on attachment 8614312 [details]
MozReview Request: mozreview: Use Bugzilla's public URL in the rbweb container (bug 993233). r=gps

https://reviewboard.mozilla.org/r/9885/#review8823

Clearing review flag for now as this is just a WIP posting for my benefit.
Comment on attachment 8614313 [details]
MozReview Request: mozreview: Add auth-delegation callback view (bug 993233). r?smacleod

https://reviewboard.mozilla.org/r/9887/#review8825

Clearing review flag for now as this is just a WIP posting for my benefit.
Attachment #8614313 - Flags: review?(smacleod)
Attachment #8614314 - Flags: review?(smacleod)
Comment on attachment 8614314 [details]
MozReview Request: mozreview: Use "mozreview" instead of "adminbugzilla" when creating users (bug 993233). r=gps

https://reviewboard.mozilla.org/r/9889/#review8827

Clearing review flag for now as this is just a WIP posting for my benefit.
Assignee

Updated

4 years ago
Depends on: 1145238
Assignee

Updated

4 years ago
Duplicate of this bug: 1148295
Assignee

Comment 11

4 years ago
Comment on attachment 8614312 [details]
MozReview Request: mozreview: Use Bugzilla's public URL in the rbweb container (bug 993233). r=gps

mozreview: Use Bugzilla's public URL in the rbweb container (bug 993233). r?gps
Attachment #8614312 - Attachment description: MozReview Request: mozreview: Add api_key field to BugzillaUserMap (Bug 993233). r?smacleod → MozReview Request: mozreview: Use Bugzilla's public URL in the rbweb container (bug 993233). r?gps
Attachment #8614312 - Flags: review?(gps)
Assignee

Comment 12

4 years ago
mozreview: Enable auth delegation in bmoweb (bug 993233). r?gps
Attachment #8634076 - Flags: review?(gps)
Assignee

Comment 13

4 years ago
mozreview: Move Bugzilla client code over to mozreview extension (bug 993233). r?dminor

Bump mozreview to 0.1.1alpha and rbbz to 0.2.9.
Attachment #8634077 - Flags: review?(dminor)
Assignee

Comment 14

4 years ago
mozreview: Add api_key field to BugzillaUserMap (bug 993233). r?smacleod

Bump mozreview version to 0.1.2alpha.  This requires a database schema
migration to add a nullable varchar(40) field with the name api_key to the
mozreview_bugzillausermap table.
Attachment #8634078 - Flags: review?(smacleod)
Assignee

Comment 15

4 years ago
mozreview: Add or update API key when getting or creating Bugzilla users (bug 993233). r?smacleod
Attachment #8634079 - Flags: review?(smacleod)
Assignee

Comment 16

4 years ago
Comment on attachment 8614314 [details]
MozReview Request: mozreview: Use "mozreview" instead of "adminbugzilla" when creating users (bug 993233). r=gps

mozreview: Authenticate to BMO with API key (bug 993233). r?smacleod
Attachment #8614314 - Attachment description: MozReview Request: mozreview: Authenticate to BMO with API key (Bug 993233). r?smacleod → MozReview Request: mozreview: Authenticate to BMO with API key (bug 993233). r?smacleod
Attachment #8614314 - Flags: review?(smacleod)
Assignee

Comment 17

4 years ago
mozreview: Add function to Bugzilla client to validate api keys (bug 993233). r?smacleod
Attachment #8634080 - Flags: review?(smacleod)
Assignee

Comment 18

4 years ago
Comment on attachment 8614313 [details]
MozReview Request: mozreview: Add auth-delegation callback view (bug 993233). r?smacleod

mozreview: Add auth-delegation callback view (bug 993233). r?smacleod

This provides the necessary endpoint for Bugzilla's auth delegation.  It
calls BugzillaBackend.authenticate() to verify the API key and, if it checks
out, calls login() to create a Review Board session.

The local docker setup has been updated to set this new endpoint in the
Bugzilla config.

Note: we should probably not deploy this until bug 1175643 has landed in
BMO and the MozReview callback function is updated appropriately.  This will
be a minor change, though, so the bulk of this commit will still apply and
hence can be reviewed now.
Attachment #8614313 - Attachment description: MozReview Request: mozreview: View for BMO auth delegation callback (Bug 993233). r?smacleod → MozReview Request: mozreview: Add auth-delegation callback view (bug 993233). r?smacleod
Attachment #8614313 - Flags: review?(smacleod)
Assignee

Updated

4 years ago
Attachment #8614311 - Attachment description: MozReview Request: mozreview: User-creation mach command creates API key (Bug 993233). r?smacleod → MozReview Request: mozreview: User-creation mach command creates API key (Bug 993233). r?dminor
Attachment #8614311 - Flags: review?(dminor)
Assignee

Comment 19

4 years ago
Comment on attachment 8614311 [details]
MozReview Request: mozreview: Authenticate to BMO with API key (bug 993233). r?smacleod

mozreview: User-creation mach command creates API key (Bug 993233). r?dminor

Fix up tests to ensure API keys are created where needed.
Assignee

Comment 20

4 years ago
mozreview: Override login page to use Bugzilla auth delegation (bug 993233). r?smacleod

Passing a "full" query-string parameter with any value, e.g. "full=true",
in the login URL results in the full login page being displayed. This exists
primarily to allow the admin user to log in, which does not have a
corresponding Bugzilla account.

Also override the "Log Out" link to not redirect to the login page, since if
you still have an open Bugzilla session, you'll be automatically logged back
into Review Board.

Selenium tests have been updated, but not all appear to be passing on tip
right now, so there are still failures.
Attachment #8634081 - Flags: review?(smacleod)
Assignee

Comment 21

4 years ago
mozreview: Do not use Bugzilla cookies for anything other than verifying a login (bug 993233). r?smacleod
Attachment #8634082 - Flags: review?(smacleod)
Comment on attachment 8634077 [details]
MozReview Request: mozreview: Move Bugzilla client code over to mozreview extension (bug 993233). r=dminor

https://reviewboard.mozilla.org/r/13273/#review11977

This looks good, but I think the commit description could use a sentence or two on the motivation behind moving the code.
Attachment #8634077 - Flags: review?(dminor) → review+
Comment on attachment 8614311 [details]
MozReview Request: mozreview: Authenticate to BMO with API key (bug 993233). r?smacleod

https://reviewboard.mozilla.org/r/9883/#review12007

::: testing/vcttesting/mozreview.py:481
(Diff revision 2)
> +             email]).strip()

I think you should put an assert here to verify that the api key is what we expect to see so the failure is localized here if something goes wrong.

::: testing/vcttesting/mozreview.py:486
(Diff revision 2)
> +

You should assert that the return code here is a 200 as well.

::: testing/vcttesting/mozreview.py:477
(Diff revision 2)
> +    def create_user_api_key(self, email):

Add a comment or docstring explaining this has the side effect of logging the user in using the api key.

::: testing/vcttesting/mozreview.py:262
(Diff revision 2)
> +        self.create_user_api_key(bugzilla.username)

I think you should either add a self.admin_api_key to store the result of this or add a comment to the effect that this has the side effect of authenticating using the api key.
Attachment #8614311 - Flags: review?(dminor)
Comment on attachment 8614312 [details]
MozReview Request: mozreview: Use Bugzilla's public URL in the rbweb container (bug 993233). r=gps

https://reviewboard.mozilla.org/r/9885/#review12009

We should at least document the setting of auth_bz_xmlrpc_url in ansible/roles/docker-rbweb/files/entrypoint.py because its existence after this patch only serves to confuse. Ideally, we would delete that assignment. Although, it may interfere with the activation of the MozReview extension. Best to double check things still work after removing the assignment!
Attachment #8614312 - Flags: review?(gps) → review+
Comment on attachment 8634076 [details]
MozReview Request: mozreview: Enable auth delegation in bmoweb (bug 993233). r=gps

https://reviewboard.mozilla.org/r/13271/#review12011

Ship It!
Attachment #8634076 - Flags: review?(gps) → review+
Comment on attachment 8634077 [details]
MozReview Request: mozreview: Move Bugzilla client code over to mozreview extension (bug 993233). r=dminor

https://reviewboard.mozilla.org/r/13273/#review12013

Ship It!
Attachment #8634077 - Flags: review+
https://reviewboard.mozilla.org/r/13275/#review12015

So we only support 1 API key per user? I'm not sure if you've had the RRA, but 1 API key per device per service would be the ideal security I would be aiming for. That way, if a device is compromised, you still have valid API keys on other devices. This assumes a compromised API key can't be used to obtain other API keys, however. I'm not sure how Bugzilla API keys work!
Comment on attachment 8634080 [details]
MozReview Request: mozreview: Add function to Bugzilla client to validate api keys (bug 993233). r=smacleod

https://reviewboard.mozilla.org/r/13279/#review12017

Ship It!
Attachment #8634080 - Flags: review+
https://reviewboard.mozilla.org/r/9883/#review12021

I really wish you would have split out the create-ldap-user -> create-user conversion to its own commit. This diff is a bit much to grok.

::: testing/vcttesting/mozreview.py:484
(Diff revision 2)
> +                     'mozreview/bmo_auth_callback/?client_api_login=%s'
> +                     '&client_api_key=%s' % (email, api_key))

Are API keys really sent to the server as part of the URL in the query string? That means they'll get logged on the server. Not cool.

Also, there is a way to pass query string or POST parameters to requests.get(). This will handle encoding, etc. Use it or face the consequences when URL unsafe characters start appearing in emails or API keys.
https://reviewboard.mozilla.org/r/13283/#review12023

I'm pretty certain there are some docs references to cookies that should be removed.
https://reviewboard.mozilla.org/r/13283/#review12025

Also, the reviewboard client extension will use cookies automatically if they are present. We need to update a lot of code on client and server to not use cookies. I can do this if you need help.
Assignee

Comment 32

4 years ago
https://reviewboard.mozilla.org/r/9883/#review12021

> Are API keys really sent to the server as part of the URL in the query string? That means they'll get logged on the server. Not cool.
> 
> Also, there is a way to pass query string or POST parameters to requests.get(). This will handle encoding, etc. Use it or face the consequences when URL unsafe characters start appearing in emails or API keys.

This is how they work right now, but I agree it's terrible, which is why we decided to do https://bugzilla.mozilla.org/show_bug.cgi?id=1175643, creating a one-time token used to fetch the API key.  I don't plan on landing API-key support in MozReview until that bug is fixed and deployed to BMO, but it should be a relatively small change from MozReview's perspective, so I wanted to start this review process now.
Assignee

Comment 34

4 years ago
https://reviewboard.mozilla.org/r/13283/#review12025

So something that I've been meaning to clarify: this commit series is just about using API keys for Review Board -> BMO communication.  It doesn't touch * -> Review Board, e.g. pushing to the hg review repo, which continues to use Bugzilla password or cookie, just to identify with Review Board.  We definitely need to fix that as well, but this commit series is the first step and at least removes the storage of cookies from Review Board (and removes expired-session errors).
Assignee

Comment 35

4 years ago
https://reviewboard.mozilla.org/r/13275/#review12015

To build upon my earlier comment, this commit series doesn't implement the standard use of API keys, which, as I understand it, is a way for client A on a specific machine to authenticate/identify with service B without storing an authoritative password.  This commit series is just replacing the storage (in the Review Board database) of a temporary (read: inconvenient) and not easily revokable session token with an easily revokable and long-lived API key that has (slightly) restricted permissions (restricted to API usage only, which is a subset of actions that can be performed via the UI, to which a session cookie grants full access).  We plan on restricting access based on MozReview-specific API keys this quarter to build upon this layer (see bug 1184332).

Adding API keys to access Review Board itself is a separate, subsequent (and important) goal.  Please ping me if you'd like to discuss further.
Comment on attachment 8634077 [details]
MozReview Request: mozreview: Move Bugzilla client code over to mozreview extension (bug 993233). r=dminor

https://reviewboard.mozilla.org/r/13273/#review12077

Ship It!
Attachment #8634077 - Flags: review+
Comment on attachment 8634078 [details]
MozReview Request: mozreview: Add api_key field to BugzillaUserMap (bug 993233). r=smacleod

https://reviewboard.mozilla.org/r/13275/#review12079

::: pylib/mozreview/mozreview/bugzilla/models.py:25
(Diff revision 1)
> +    api_key = models.CharField(max_length=40, null=True)

I want to say that all new per user data goes in the MozReviewUserProfile table, but I'd be willing to let this slide for this field since it's directly tied to the bugzilla auth.

Could you add a comment to this model though indicating that it should not be updated going forward, and all new data should be put in the profile table?
Attachment #8634078 - Flags: review?(smacleod)
Comment on attachment 8634079 [details]
MozReview Request: mozreview: Add function to store Bugzilla API keys (bug 993233). r=smacleod

https://reviewboard.mozilla.org/r/13277/#review12081

::: pylib/mozreview/mozreview/bugzilla/models.py:35
(Diff revision 1)
> -def get_or_create_bugzilla_users(user_data):
> +def get_or_create_bugzilla_users(user_data, api_key=None):

This seems like quite the footgun (allowing multiple users, but giving them all the same API key). I could see a security bug slipping in quite easily if we mess up a call to this function.

I'd prefer we make assigning the api key a more explicit operation for only a single user.
Attachment #8634079 - Flags: review?(smacleod)
Attachment #8614314 - Flags: review?(smacleod)
Comment on attachment 8614314 [details]
MozReview Request: mozreview: Use "mozreview" instead of "adminbugzilla" when creating users (bug 993233). r=gps

https://reviewboard.mozilla.org/r/9889/#review12087

::: pylib/mozreview/mozreview/bugzilla/client.py:60
(Diff revision 2)
> -    def __init__(self, login=None, logincookie=None, xmlrpc_url=None):
> +    def __init__(self, user_id=None, xmlrpc_url=None):

Something I appreciated about this class, and used for some manual scripting / testing, was that I could import and use it outside the context of Review Board / MozReview.

By passing the user id and doing a db lookup we lose that functionality. Could you refactor this a bit so that it takes an API key instead of looking it up with the user id.
Comment on attachment 8634080 [details]
MozReview Request: mozreview: Add function to Bugzilla client to validate api keys (bug 993233). r=smacleod

https://reviewboard.mozilla.org/r/13279/#review12133

Ship It!
Attachment #8634080 - Flags: review?(smacleod) → review+
Comment on attachment 8614313 [details]
MozReview Request: mozreview: Add auth-delegation callback view (bug 993233). r?smacleod

https://reviewboard.mozilla.org/r/9887/#review12135

::: pylib/mozreview/mozreview/views.py:17
(Diff revision 2)
> +    bmo_login = request.GET.get('client_api_login', None)

Can we call this `bmo_username` instead, I like it being more explicit so we don't have to lookup whether it is a user id or whatnot.

::: pylib/mozreview/mozreview/views.py:51
(Diff revision 2)
> +    if not user.is_active:
> +        logging.warning('Validated API key but user %s is inactive.' %
> +                        bmo_login)
> +        return HttpResponseBadRequest()

I'm not really sure what this flag is used for / does in the bmo sense, but would it be worth notifying the user they can't login because their account is inactive?

::: pylib/mozreview/mozreview/extension.py:116
(Diff revision 2)
> +        urlpatterns = patterns('',
> +                               url(r'^mozreview/', include('mozreview.urls')))
> +        URLHook(self, urlpatterns)

cut out the intermediate `urlpatterns` variable.
Attachment #8614313 - Flags: review?(smacleod)
Attachment #8634081 - Flags: review?(smacleod)
Comment on attachment 8634081 [details]
MozReview Request: mozreview: Override login page to use Bugzilla auth delegation (bug 993233). r=smacleod

https://reviewboard.mozilla.org/r/13281/#review12137

Please use "hg mv" / "hg cp" when renaming / copying files so that the diffs displayed will be intelligent. Right now mozreview.js looks like a giant add to me rather than a rename then not modified *or* modifies (I'd rather not have to inspect every line manually).

::: pylib/mozreview/mozreview/templatetags/bugzilla.py:21
(Diff revision 1)
> +    callback_uri = request.build_absolute_uri(reverse('bmo-auth-callback'))

do we need to encode this?

::: pylib/mozreview/mozreview/templatetags/bugzilla.py:30
(Diff revision 1)
> +    uri = '%s://%s%sauth.cgi?description=mozreview&callback=%s' % (
> +        u.scheme, u.netloc.rstrip('/'), bugzilla_root, callback_uri)

aren't there urllib methods for building querystring urls that deal with encoding and everything? might be better to use one of those.

::: pylib/mozreview/mozreview/tests/test_login.py:20
(Diff revision 1)
> -        self.assertInvalidLogin()
> +        # Because Bugzilla requires an email address, the form can't
> +        # even be submitted.

I don't understand this comment.

::: testing/vcttesting/unittest.py:146
(Diff revision 1)
> +    def load_bzurl(self, path):
> +        """Load the specified Review Board URL."""

This function's name vs docstring is problematic.

::: testing/vcttesting/unittest.py:155
(Diff revision 1)
> +    def verify_bzurl(self, path):

docstring please
https://reviewboard.mozilla.org/r/13283/#review12025

You should make your commit messages more verbose - this distinction and explanation should exist there.
Comment on attachment 8634082 [details]
MozReview Request: mozreview: Stop storing Bugzilla cookies (bug 993233). r=smacleod

https://reviewboard.mozilla.org/r/13283/#review12141

It's still possible (and necessary) to login with cookies through the API (the BugzillaCookieLoginResource allows this). We should make that clear in the commit message.
Attachment #8634082 - Flags: review?(smacleod)
Assignee

Comment 45

4 years ago
https://reviewboard.mozilla.org/r/9885/#review12009

Yeah, it appears to be needed to create the admin user.  I get a few test failures without it, so I'll leave it in and add a comment.
Assignee

Comment 46

4 years ago
https://reviewboard.mozilla.org/r/9887/#review12135

> Can we call this `bmo_username` instead, I like it being more explicit so we don't have to lookup whether it is a user id or whatnot.

This is a Bugzilla-specific term, but yeah makes sense.

> I'm not really sure what this flag is used for / does in the bmo sense, but would it be worth notifying the user they can't login because their account is inactive?

This is actually from Review Board/Django (https://docs.djangoproject.com/en/1.7/ref/contrib/auth/#django.contrib.auth.models.User.is_active).  What you're saying probably still applies though... but given that we defer to Bugzilla for everything account-related right now, I put this in just for completeness.  I'll add a TODO, though, to note it, just in case we some day disable users only in Review Board.
Assignee

Comment 47

4 years ago
https://reviewboard.mozilla.org/r/9883/#review12007

> Add a comment or docstring explaining this has the side effect of logging the user in using the api key.

I added a note, although I don't think there's much a side effect here. True, the user is logged in, but we don't preserve the session cookie anywhere, so it shouldn't affect subsequent interactions with Review Board.

> I think you should either add a self.admin_api_key to store the result of this or add a comment to the effect that this has the side effect of authenticating using the api key.

I'm not sure what you mean here.  As I noted in the comment below, I don't think this really has any side effects.  We don't need to record the API key anywhere; it's used implicitly in future interactions with Bugzilla whenever the admin user (or any other user with a registered API key) logs into Review Board using the API.
Assignee

Comment 48

4 years ago
https://reviewboard.mozilla.org/r/9883/#review12021

I split the test-user-creation modifications into their own commit and flagged you (coming shortly).
Assignee

Comment 49

4 years ago
Comment on attachment 8614312 [details]
MozReview Request: mozreview: Use Bugzilla's public URL in the rbweb container (bug 993233). r=gps

mozreview: Use Bugzilla's public URL in the rbweb container (bug 993233). r=gps

The public URL is required in order to redirect the user to Bugzilla to
log in.
Attachment #8614312 - Attachment description: MozReview Request: mozreview: Use Bugzilla's public URL in the rbweb container (bug 993233). r?gps → MozReview Request: mozreview: Use Bugzilla's public URL in the rbweb container (bug 993233). r=gps
Attachment #8614312 - Flags: review+
Assignee

Comment 50

4 years ago
Comment on attachment 8634076 [details]
MozReview Request: mozreview: Enable auth delegation in bmoweb (bug 993233). r=gps

mozreview: Enable auth delegation in bmoweb (bug 993233). r=gps
Attachment #8634076 - Attachment description: MozReview Request: mozreview: Enable auth delegation in bmoweb (bug 993233). r?gps → MozReview Request: mozreview: Enable auth delegation in bmoweb (bug 993233). r=gps
Attachment #8634076 - Flags: review+
Assignee

Comment 51

4 years ago
Comment on attachment 8634077 [details]
MozReview Request: mozreview: Move Bugzilla client code over to mozreview extension (bug 993233). r=dminor

mozreview: Move Bugzilla client code over to mozreview extension (bug 993233). r=dminor

This is part of a larger effort to consolidate all Review Board customizations
in a single extension (mozreview).

Bump mozreview to 0.1.1alpha and rbbz to 0.2.9.
Attachment #8634077 - Attachment description: MozReview Request: mozreview: Move Bugzilla client code over to mozreview extension (bug 993233). r?dminor → MozReview Request: mozreview: Move Bugzilla client code over to mozreview extension (bug 993233). r=dminor
Attachment #8634077 - Flags: review+
Assignee

Comment 52

4 years ago
Comment on attachment 8634078 [details]
MozReview Request: mozreview: Add api_key field to BugzillaUserMap (bug 993233). r=smacleod

mozreview: Add api_key field to BugzillaUserMap (bug 993233). r?smacleod

Bump mozreview version to 0.1.2alpha.  This requires a database schema
migration to add a nullable varchar(40) field with the name api_key to the
mozreview_bugzillausermap table.
Attachment #8634078 - Flags: review?(smacleod)
Assignee

Comment 53

4 years ago
Comment on attachment 8634079 [details]
MozReview Request: mozreview: Add function to store Bugzilla API keys (bug 993233). r=smacleod

mozreview: Add or update API key when getting or creating Bugzilla users (bug 993233). r?smacleod
Attachment #8634079 - Flags: review?(smacleod)
Assignee

Comment 54

4 years ago
Comment on attachment 8634080 [details]
MozReview Request: mozreview: Add function to Bugzilla client to validate api keys (bug 993233). r=smacleod

mozreview: Add function to Bugzilla client to validate api keys (bug 993233). r=smacleod
Attachment #8634080 - Attachment description: MozReview Request: mozreview: Add function to Bugzilla client to validate api keys (bug 993233). r?smacleod → MozReview Request: mozreview: Add function to Bugzilla client to validate api keys (bug 993233). r=smacleod
Attachment #8634080 - Flags: review+
Assignee

Comment 55

4 years ago
Comment on attachment 8614313 [details]
MozReview Request: mozreview: Add auth-delegation callback view (bug 993233). r?smacleod

mozreview: Add auth-delegation callback view (bug 993233). r?smacleod

This provides the necessary endpoint for Bugzilla's auth delegation.  It
calls BugzillaBackend.authenticate() to verify the API key and, if it checks
out, calls login() to create a Review Board session.

The local docker setup has been updated to set this new endpoint in the
Bugzilla config.

Note: we should probably not deploy this until bug 1175643 has landed in
BMO and the MozReview callback function is updated appropriately.  This will
be a minor change, though, so the bulk of this commit will still apply and
hence can be reviewed now.
Attachment #8614313 - Flags: review?(smacleod)
Assignee

Comment 56

4 years ago
Comment on attachment 8614314 [details]
MozReview Request: mozreview: Use "mozreview" instead of "adminbugzilla" when creating users (bug 993233). r=gps

mozreview: Use "mozreview" instead of "adminbugzilla" when creating users (bug 993233). r?gps

The "mozreview create-user" mach command has options to create all aspects
of a mozreview user, so we consolidate all the "adminbugzilla create-user"
and "mozreview create-ldap-user" calls into just "mozreview create-user"
for simplicity.
Attachment #8614314 - Attachment description: MozReview Request: mozreview: Authenticate to BMO with API key (bug 993233). r?smacleod → MozReview Request: mozreview: Use "mozreview" instead of "adminbugzilla" when creating users (bug 993233). r?gps
Attachment #8614314 - Flags: review?(gps)
Assignee

Updated

4 years ago
Attachment #8614311 - Attachment description: MozReview Request: mozreview: User-creation mach command creates API key (Bug 993233). r?dminor → MozReview Request: mozreview: Authenticate to BMO with API key (bug 993233). r?smacleod
Attachment #8614311 - Flags: review?(smacleod)
Assignee

Comment 57

4 years ago
Comment on attachment 8614311 [details]
MozReview Request: mozreview: Authenticate to BMO with API key (bug 993233). r?smacleod

mozreview: Authenticate to BMO with API key (bug 993233). r?smacleod

Send Bugzilla API keys with authenticated requests rather than cookies.
Fix up tests to ensure API keys are created where needed.
Assignee

Comment 58

4 years ago
mozreview: Rename common.js to init_rr.js and clean it up (bug 993233). r?smacleod

"common.js" is misleading because it is only used on the review-request view,
and furthermore it doesn't describe what it does.
Attachment #8636774 - Flags: review?(smacleod)
Assignee

Comment 59

4 years ago
Comment on attachment 8634081 [details]
MozReview Request: mozreview: Override login page to use Bugzilla auth delegation (bug 993233). r=smacleod

mozreview: Override login page to use Bugzilla auth delegation (bug 993233). r?smacleod

Passing a "full" query-string parameter with any value, e.g. "full=true",
in the login URL results in the full login page being displayed. This exists
primarily to allow the admin user to log in, which does not have a
corresponding Bugzilla account.

Also override the "Log Out" link to not redirect to the login page, since if
you still have an open Bugzilla session, you'll be automatically logged back
into Review Board.

Selenium tests have been updated, but not all appear to be passing on tip
right now, so there are still failures.
Attachment #8634081 - Flags: review?(smacleod)
Assignee

Comment 60

4 years ago
Comment on attachment 8634082 [details]
MozReview Request: mozreview: Stop storing Bugzilla cookies (bug 993233). r=smacleod

mozreview: Do not use Bugzilla cookies for anything other than verifying a login (bug 993233). r?smacleod

This just changes the mechanism that the rbbz and mozreview extensions use
for communicating with Bugzilla. For the purposes of identification with
MozReview, logging in with username and password or login cookies is still
possible via the API and with /account/login/?full=true (or any other value
for "full"). The latter is intended only for the admin account, as it will
not cause an API key to be generated and sent over from Bugzilla, meaning
most calls from Review Board to Bugzilla will fail.
Attachment #8634082 - Flags: review?(smacleod)
Comment on attachment 8614312 [details]
MozReview Request: mozreview: Use Bugzilla's public URL in the rbweb container (bug 993233). r=gps

https://reviewboard.mozilla.org/r/9885/#review12437

Ship It!
Attachment #8614312 - Flags: review+
Comment on attachment 8634076 [details]
MozReview Request: mozreview: Enable auth delegation in bmoweb (bug 993233). r=gps

https://reviewboard.mozilla.org/r/13271/#review12439

Ship It!
Attachment #8634076 - Flags: review+
Comment on attachment 8614314 [details]
MozReview Request: mozreview: Use "mozreview" instead of "adminbugzilla" when creating users (bug 993233). r=gps

https://reviewboard.mozilla.org/r/9889/#review12441

Nice!
Attachment #8614314 - Flags: review?(gps) → review+
Comment on attachment 8634078 [details]
MozReview Request: mozreview: Add api_key field to BugzillaUserMap (bug 993233). r=smacleod

https://reviewboard.mozilla.org/r/13275/#review12667

Ship It!
Attachment #8634078 - Flags: review?(smacleod) → review+
Comment on attachment 8634079 [details]
MozReview Request: mozreview: Add function to store Bugzilla API keys (bug 993233). r=smacleod

https://reviewboard.mozilla.org/r/13277/#review12671

::: pylib/mozreview/mozreview/bugzilla/models.py:118
(Diff revision 2)
> +def set_bugzilla_api_key(user, api_key):

The commit message needs to be rewritten to reflect what this commit now does.
Attachment #8634079 - Flags: review?(smacleod) → review+
Comment on attachment 8614313 [details]
MozReview Request: mozreview: Add auth-delegation callback view (bug 993233). r?smacleod

https://reviewboard.mozilla.org/r/9887/#review12669

Looks good for the most part, just a few small nits.

::: pylib/mozreview/mozreview/models.py:19
(Diff revision 3)
> +    'set_bugzilla_api_key',

This stuff should be in the commit that creates the function.

::: pylib/mozreview/mozreview/views.py:50
(Diff revision 3)
> +    user = users[0]

We should assert that this user matches the username which we're supposed to be logging in with.

::: pylib/mozreview/mozreview/views.py:54
(Diff revision 3)
> +    if not user.is_active:
> +        # TODO: We should probably display a warning to the user, but we don't
> +        # currently use Review Board's is_active flag, so this isn't terribly
> +        # important. This clause is here just for completeness at the
> +        # moment.
> +        logging.warning('Validated API key but user %s is inactive.' %
> +                        bmo_username)
> +        return HttpResponseBadRequest()

I think we should move this to before the call where we associate the API key.

::: pylib/mozreview/mozreview/views.py:13
(Diff revision 3)
> +def bmo_auth_callback(request):

It might be good to catch any bugzilla errors due to stupid deadlocks on bugzilla's end, or other unexpected things like bugzilla going down during someone logging in, and display something nice to the user.
Attachment #8614313 - Flags: review?(smacleod)
Comment on attachment 8614311 [details]
MozReview Request: mozreview: Authenticate to BMO with API key (bug 993233). r?smacleod

https://reviewboard.mozilla.org/r/9883/#review12677

::: testing/vcttesting/mozreview.py:494
(Diff revision 3)
> +        count = 1
> +        url = self.reviewboard_url + 'mozreview/bmo_auth_callback/'
> +        params = {'client_api_login': email, 'client_api_key': api_key}
> +        r = requests.get(url, params=params)
> +
> +        while r.status_code == 500 and count < 3:
> +            count += 1
> +            r = requests.get(url, params=params)
> +
> +        assert r.status_code == 200
> +
> +        return api_key

I think something like the following would be cleaner:
```
        url = self.reviewboard_url + 'mozreview/bmo_auth_callback/'
        params = {'client_api_login': email, 'client_api_key': api_key}

        for i in range(3):
            if requests.get(url, params=params).status_code == 200:
                return api_key
        else:
            # Raise / return some error here
```

::: testing/vcttesting/mozreview_mach_commands.py:208
(Diff revision 3)
> +                           api_key=not no_api_key)

"not no api key" - heh... I *guess* I'm fine with it, but it makes me feel dirty.

::: hgext/reviewboard/tests/test-auth.t:151
(Diff revision 3)
> -  dashboard_columns: 
> +  dashboard_columns: selected,new_updates,ship_it,my_comments,summary,submitter,last_updated_since
>    default_use_rich_text: None
>    extra_data: {}
>    first_time_setup_done: 0
>    group_columns: 
> -  id: 2
> +  id: 3
>    is_private: 1
>    open_an_issue: 1
>    review_request_columns: 
>    should_send_email: 1
>    should_send_own_updates: 1
>    show_closed: 1
> -  sort_dashboard_columns: 
> +  sort_dashboard_columns: -last_updated

I don't really understand these changes? is it because the user is no being logged in before this runs or something? what's going on here?

::: hgext/reviewboard/tests/test-user-apis.t:40
(Diff revision 3)
> +    url: /users/default%2B5/
> +    username: default+5
> +  - id: 4

Again, I don't see why this is changing, can you explain?
Attachment #8614311 - Flags: review?(smacleod)
Comment on attachment 8636774 [details]
MozReview Request: mozreview: Rename common.js to init_rr.js and clean it up (bug 993233). r=smacleod

https://reviewboard.mozilla.org/r/13693/#review12679

Ship It!
Attachment #8636774 - Flags: review?(smacleod) → review+
Comment on attachment 8634081 [details]
MozReview Request: mozreview: Override login page to use Bugzilla auth delegation (bug 993233). r=smacleod

https://reviewboard.mozilla.org/r/13281/#review12683

::: pylib/mozreview/mozreview/static/mozreview/js/logout.js:3
(Diff revision 2)
> +  $("#accountnav li").css("visibility", "visible");

Why is this required?

::: pylib/mozreview/mozreview/templatetags/bugzilla.py:9
(Diff revision 2)
> +from djblets.siteconfig.models import SiteConfiguration
> +
> +register = template.Library()

2 blanks between?

::: testing/vcttesting/unittest.py:11
(Diff revision 2)
>  import unittest
>  
> +from urllib import urlencode

why the blank?
Attachment #8634081 - Flags: review?(smacleod) → review+
Comment on attachment 8634082 [details]
MozReview Request: mozreview: Stop storing Bugzilla cookies (bug 993233). r=smacleod

https://reviewboard.mozilla.org/r/13283/#review12685

I'm still not 100% happy with the commit message summary - how about "Stop storing bugzilla cookies used for initial authentication"
Attachment #8634082 - Flags: review?(smacleod) → review+
Assignee

Comment 71

4 years ago
https://reviewboard.mozilla.org/r/13281/#review12683

> why the blank?

For some reason I thought there was a convention to add a line between 'import foo' and 'from foo import bar' sections, but apparently I just made that up.

> Why is this required?

It's a similar trick to what you've used.  Because we have to override the logout link to go to our custom view (in order to not have it try to log back in again, which would immediately succeed if the user also had an active Bugzilla session) in JavaScript, owing to there being no nice way to modify the template, I am hiding the #accountnav items (there's no ID for the logout item specifically) via CSS in common.less (https://reviewboard.mozilla.org/r/13281/diff/2#1).  After the JS executes and modifies the logout link, I unhide them.

It might be overkill since a user would have to be very fast to hit the logout link before it was updated, but it feels like the right thing to do.
Assignee

Comment 72

4 years ago
https://reviewboard.mozilla.org/r/9883/#review12677

> Again, I don't see why this is changing, can you explain?

See above.

> I don't really understand these changes? is it because the user is no being logged in before this runs or something? what's going on here?

This is a result of the fact that 'mozreview create-user' now logs the user in once, by directly calling the auth-delegation callback, which in turn causes a redirect to the dashboard, which explains the changes to dashboard_columns.  An extra user is also created in Review Board (the "default" user in helpers.sh) where before only an entry in Bugzilla was created until the user was actually used in a Review Board context.  This explains the incremented user id above (and in your comment below).

> "not no api key" - heh... I *guess* I'm fine with it, but it makes me feel dirty.

Heh I know, but I wanted the 'api_key' arg to the internal function to match the others (defaulting to True) and require the user to explicitly say "don't create an API key".  Since it's a boolean option it ended up being weird like this.
Assignee

Comment 73

4 years ago
Comment on attachment 8614312 [details]
MozReview Request: mozreview: Use Bugzilla's public URL in the rbweb container (bug 993233). r=gps

mozreview: Use Bugzilla's public URL in the rbweb container (bug 993233). r=gps

The public URL is required in order to redirect the user to Bugzilla to
log in.
Assignee

Comment 74

4 years ago
Comment on attachment 8634076 [details]
MozReview Request: mozreview: Enable auth delegation in bmoweb (bug 993233). r=gps

mozreview: Enable auth delegation in bmoweb (bug 993233). r=gps
Assignee

Comment 75

4 years ago
Comment on attachment 8634077 [details]
MozReview Request: mozreview: Move Bugzilla client code over to mozreview extension (bug 993233). r=dminor

mozreview: Move Bugzilla client code over to mozreview extension (bug 993233). r=dminor

This is part of a larger effort to consolidate all Review Board customizations
in a single extension (mozreview).

Bump mozreview to 0.1.1alpha and rbbz to 0.2.9.
Attachment #8634077 - Flags: review?(smacleod)
Attachment #8634077 - Flags: review?(gps)
Attachment #8634077 - Flags: review?(dminor)
Assignee

Comment 76

4 years ago
Comment on attachment 8634078 [details]
MozReview Request: mozreview: Add api_key field to BugzillaUserMap (bug 993233). r=smacleod

mozreview: Add api_key field to BugzillaUserMap (bug 993233). r=smacleod

Bump mozreview version to 0.1.2alpha.  This requires a database schema
migration to add a nullable varchar(40) field with the name api_key to the
mozreview_bugzillausermap table.
Attachment #8634078 - Attachment description: MozReview Request: mozreview: Add api_key field to BugzillaUserMap (bug 993233). r?smacleod → MozReview Request: mozreview: Add api_key field to BugzillaUserMap (bug 993233). r=smacleod
Assignee

Comment 77

4 years ago
Comment on attachment 8634079 [details]
MozReview Request: mozreview: Add function to store Bugzilla API keys (bug 993233). r=smacleod

mozreview: Add function to store Bugzilla API keys (bug 993233). r=smacleod
Attachment #8634079 - Attachment description: MozReview Request: mozreview: Add or update API key when getting or creating Bugzilla users (bug 993233). r?smacleod → MozReview Request: mozreview: Add function to store Bugzilla API keys (bug 993233). r=smacleod
Assignee

Updated

4 years ago
Attachment #8634080 - Flags: review?(smacleod)
Attachment #8634080 - Flags: review?(gps)
Assignee

Comment 78

4 years ago
Comment on attachment 8634080 [details]
MozReview Request: mozreview: Add function to Bugzilla client to validate api keys (bug 993233). r=smacleod

mozreview: Add function to Bugzilla client to validate api keys (bug 993233). r=smacleod
Assignee

Comment 79

4 years ago
Comment on attachment 8614313 [details]
MozReview Request: mozreview: Add auth-delegation callback view (bug 993233). r?smacleod

mozreview: Add auth-delegation callback view (bug 993233). r?smacleod

This provides the necessary endpoint for Bugzilla's auth delegation.  It
calls BugzillaBackend.authenticate() to verify the API key and, if it checks
out, calls login() to create a Review Board session.

The local docker setup has been updated to set this new endpoint in the
Bugzilla config.

Note: we should probably not deploy this until bug 1175643 has landed in
BMO and the MozReview callback function is updated appropriately.  This will
be a minor change, though, so the bulk of this commit will still apply and
hence can be reviewed now.
Attachment #8614313 - Flags: review?(smacleod)
Assignee

Updated

4 years ago
Attachment #8614314 - Attachment description: MozReview Request: mozreview: Use "mozreview" instead of "adminbugzilla" when creating users (bug 993233). r?gps → MozReview Request: mozreview: Use "mozreview" instead of "adminbugzilla" when creating users (bug 993233). r=gps
Assignee

Comment 80

4 years ago
Comment on attachment 8614314 [details]
MozReview Request: mozreview: Use "mozreview" instead of "adminbugzilla" when creating users (bug 993233). r=gps

mozreview: Use "mozreview" instead of "adminbugzilla" when creating users (bug 993233). r=gps

The "mozreview create-user" mach command has options to create all aspects
of a mozreview user, so we consolidate all the "adminbugzilla create-user"
and "mozreview create-ldap-user" calls into just "mozreview create-user"
for simplicity.
Assignee

Comment 81

4 years ago
Comment on attachment 8614311 [details]
MozReview Request: mozreview: Authenticate to BMO with API key (bug 993233). r?smacleod

mozreview: Authenticate to BMO with API key (bug 993233). r?smacleod

Send Bugzilla API keys with authenticated requests rather than cookies.
Fix up tests to ensure API keys are created where needed.
Attachment #8614311 - Flags: review?(smacleod)
Assignee

Updated

4 years ago
Attachment #8636774 - Attachment description: MozReview Request: mozreview: Rename common.js to init_rr.js and clean it up (bug 993233). r?smacleod → MozReview Request: mozreview: Rename common.js to init_rr.js and clean it up (bug 993233). r=smacleod
Assignee

Comment 82

4 years ago
Comment on attachment 8636774 [details]
MozReview Request: mozreview: Rename common.js to init_rr.js and clean it up (bug 993233). r=smacleod

mozreview: Rename common.js to init_rr.js and clean it up (bug 993233). r=smacleod

"common.js" is misleading because it is only used on the review-request view,
and furthermore it doesn't describe what it does.
Assignee

Comment 83

4 years ago
Comment on attachment 8634081 [details]
MozReview Request: mozreview: Override login page to use Bugzilla auth delegation (bug 993233). r=smacleod

mozreview: Override login page to use Bugzilla auth delegation (bug 993233). r=smacleod

Passing a "full" query-string parameter with any value, e.g. "full=true",
in the login URL results in the full login page being displayed. This exists
primarily to allow the admin user to log in, which does not have a
corresponding Bugzilla account.

Also override the "Log Out" link to not redirect to the login page, since if
you still have an open Bugzilla session, you'll be automatically logged back
into Review Board.

Selenium tests have been updated, but not all appear to be passing on tip
right now, so there are still failures.
Attachment #8634081 - Attachment description: MozReview Request: mozreview: Override login page to use Bugzilla auth delegation (bug 993233). r?smacleod → MozReview Request: mozreview: Override login page to use Bugzilla auth delegation (bug 993233). r=smacleod
Assignee

Comment 84

4 years ago
Comment on attachment 8634082 [details]
MozReview Request: mozreview: Stop storing Bugzilla cookies (bug 993233). r=smacleod

mozreview: Stop storing Bugzilla cookies (bug 993233). r=smacleod

Since all communication is now done via API keys, we no longer need to
store cookies in the user object.

For the purposes of identification with MozReview, logging in with username
and password or login cookies is still possible via the API and with
/account/login/?full=true (or any other value for "full"). The latter is
intended only for the admin account, as it will not cause an API key to be
generated and sent over from Bugzilla, meaning most calls from Review Board
to Bugzilla will fail. In both cases, the cookie/password is only used to
verify the account with Bugzilla and is discarded immediately after.
Attachment #8634082 - Attachment description: MozReview Request: mozreview: Do not use Bugzilla cookies for anything other than verifying a login (bug 993233). r?smacleod → MozReview Request: mozreview: Stop storing Bugzilla cookies (bug 993233). r=smacleod
Comment on attachment 8634077 [details]
MozReview Request: mozreview: Move Bugzilla client code over to mozreview extension (bug 993233). r=dminor

https://reviewboard.mozilla.org/r/13273/#review12987

Ship It!
Attachment #8634077 - Flags: review?(gps) → review+
Comment on attachment 8634080 [details]
MozReview Request: mozreview: Add function to Bugzilla client to validate api keys (bug 993233). r=smacleod

https://reviewboard.mozilla.org/r/13279/#review12989

Ship It!
Attachment #8634080 - Flags: review?(gps) → review+
Comment on attachment 8634077 [details]
MozReview Request: mozreview: Move Bugzilla client code over to mozreview extension (bug 993233). r=dminor

https://reviewboard.mozilla.org/r/13273/#review13023

Ship It!
Attachment #8634077 - Flags: review?(dminor) → review+
Comment on attachment 8614311 [details]
MozReview Request: mozreview: Authenticate to BMO with API key (bug 993233). r?smacleod

https://reviewboard.mozilla.org/r/9883/#review14193

Ship It!
Attachment #8614311 - Flags: review?(smacleod) → review+
Attachment #8634080 - Flags: review?(smacleod) → review+
Attachment #8634077 - Flags: review?(smacleod) → review+
Attachment #8614313 - Flags: review?(smacleod)
Comment on attachment 8614313 [details]
MozReview Request: mozreview: Add auth-delegation callback view (bug 993233). r?smacleod

https://reviewboard.mozilla.org/r/9887/#review14195

as discussed in irc I will review when updated
Assignee

Comment 90

4 years ago
Comment on attachment 8614312 [details]
MozReview Request: mozreview: Use Bugzilla's public URL in the rbweb container (bug 993233). r=gps

mozreview: Use Bugzilla's public URL in the rbweb container (bug 993233). r=gps

The public URL is required in order to redirect the user to Bugzilla to
log in.
Assignee

Comment 91

4 years ago
Comment on attachment 8634076 [details]
MozReview Request: mozreview: Enable auth delegation in bmoweb (bug 993233). r=gps

mozreview: Enable auth delegation in bmoweb (bug 993233). r=gps
Assignee

Comment 92

4 years ago
Comment on attachment 8634077 [details]
MozReview Request: mozreview: Move Bugzilla client code over to mozreview extension (bug 993233). r=dminor

mozreview: Move Bugzilla client code over to mozreview extension (bug 993233). r=dminor

This is part of a larger effort to consolidate all Review Board customizations
in a single extension (mozreview).

Bump mozreview to 0.1.1alpha and rbbz to 0.2.9.
Assignee

Comment 93

4 years ago
Comment on attachment 8634078 [details]
MozReview Request: mozreview: Add api_key field to BugzillaUserMap (bug 993233). r=smacleod

mozreview: Add api_key field to BugzillaUserMap (bug 993233). r=smacleod

Bump mozreview version to 0.1.2alpha.  This requires a database schema
migration to add a nullable varchar(40) field with the name api_key to the
mozreview_bugzillausermap table.
Assignee

Comment 94

4 years ago
Comment on attachment 8634079 [details]
MozReview Request: mozreview: Add function to store Bugzilla API keys (bug 993233). r=smacleod

mozreview: Add function to store Bugzilla API keys (bug 993233). r=smacleod
Assignee

Comment 95

4 years ago
Comment on attachment 8634080 [details]
MozReview Request: mozreview: Add function to Bugzilla client to validate api keys (bug 993233). r=smacleod

mozreview: Add function to Bugzilla client to validate api keys (bug 993233). r=smacleod
Assignee

Comment 96

4 years ago
Comment on attachment 8614313 [details]
MozReview Request: mozreview: Add auth-delegation callback view (bug 993233). r?smacleod

mozreview: Add auth-delegation callback view (bug 993233). r?smacleod

This provides the necessary endpoint for Bugzilla's auth delegation.  It
calls BugzillaBackend.authenticate() to verify the API key and, if it checks
out, calls login() to create a Review Board session.

The local docker setup has been updated to set this new endpoint in the
Bugzilla config.

It requires a new table, mozreview_unverifiedbugzillaapikey, defined as such:

CREATE TABLE "mozreview_unverifiedbugzillaapikey" (
    "id" integer NOT NULL PRIMARY KEY,
    "login" varchar(255) NOT NULL,
    "api_key" varchar(40) NOT NULL,
    "remote_addr" char(39) NOT NULL,
    "timestamp" datetime NOT NULL
);
CREATE INDEX "mozreview_unverifiedbugzillaapikey_26440ee6" ON "mozreview_unverifiedbugzillaapikey" ("login");
Attachment #8614313 - Flags: review?(smacleod)
Assignee

Comment 97

4 years ago
Comment on attachment 8614314 [details]
MozReview Request: mozreview: Use "mozreview" instead of "adminbugzilla" when creating users (bug 993233). r=gps

mozreview: Use "mozreview" instead of "adminbugzilla" when creating users (bug 993233). r=gps

The "mozreview create-user" mach command has options to create all aspects
of a mozreview user, so we consolidate all the "adminbugzilla create-user"
and "mozreview create-ldap-user" calls into just "mozreview create-user"
for simplicity.
Assignee

Comment 98

4 years ago
Comment on attachment 8614311 [details]
MozReview Request: mozreview: Authenticate to BMO with API key (bug 993233). r?smacleod

mozreview: Authenticate to BMO with API key (bug 993233). r?smacleod

Send Bugzilla API keys with authenticated requests rather than cookies.
Fix up tests to ensure API keys are created where needed.
Assignee

Comment 99

4 years ago
Comment on attachment 8636774 [details]
MozReview Request: mozreview: Rename common.js to init_rr.js and clean it up (bug 993233). r=smacleod

mozreview: Rename common.js to init_rr.js and clean it up (bug 993233). r=smacleod

"common.js" is misleading because it is only used on the review-request view,
and furthermore it doesn't describe what it does.
Assignee

Comment 100

4 years ago
Comment on attachment 8634081 [details]
MozReview Request: mozreview: Override login page to use Bugzilla auth delegation (bug 993233). r=smacleod

mozreview: Override login page to use Bugzilla auth delegation (bug 993233). r=smacleod

Passing a "full" query-string parameter with any value, e.g. "full=true",
in the login URL results in the full login page being displayed. This exists
primarily to allow the admin user to log in, which does not have a
corresponding Bugzilla account.

Also override the "Log Out" link to not redirect to the login page, since if
you still have an open Bugzilla session, you'll be automatically logged back
into Review Board.

Selenium tests have been updated, but not all appear to be passing on tip
right now, so there are still failures.
Assignee

Comment 101

4 years ago
Comment on attachment 8634082 [details]
MozReview Request: mozreview: Stop storing Bugzilla cookies (bug 993233). r=smacleod

mozreview: Stop storing Bugzilla cookies (bug 993233). r=smacleod

Since all communication is now done via API keys, we no longer need to
store cookies in the user object.

For the purposes of identification with MozReview, logging in with username
and password or login cookies is still possible via the API and with
/account/login/?full=true (or any other value for "full"). The latter is
intended only for the admin account, as it will not cause an API key to be
generated and sent over from Bugzilla, meaning most calls from Review Board
to Bugzilla will fail. In both cases, the cookie/password is only used to
verify the account with Bugzilla and is discarded immediately after.
Comment on attachment 8614313 [details]
MozReview Request: mozreview: Add auth-delegation callback view (bug 993233). r?smacleod

https://reviewboard.mozilla.org/r/9887/#review14665

::: pylib/mozreview/mozreview/bugzilla/models.py:38
(Diff revision 5)
> +class UnverifiedBugzillaApiKey(models.Model):

docstring

::: pylib/mozreview/mozreview/bugzilla/models.py:39
(Diff revision 5)
> +    login = models.CharField(max_length=255, db_index=True)

why "login" here? it seems like this value is named "login", "bmo_username", and "client_api_login" throughout this patch. Can we consolidate these into a nice descriptive name (at least the names we control)

::: pylib/mozreview/mozreview/bugzilla/models.py:48
(Diff revision 5)
> +        s = '%s%s%s%s%s' % (self.login, self.api_key, self.remote_addr,
> +                            self.timestamp, settings.SECRET_KEY)
> +        return hashlib.md5(s).hexdigest()

This stuff needs a serious docstring / justification for hash choice and the security requirements around this.

as it is I'm not comfortable with the secret key / api key being part of this hash.

to me it seems this purpose is just some random GUID to match up the keys, can we just generate something like that instead?

::: pylib/mozreview/mozreview/views.py:32
(Diff revision 5)
> +    This will change slightly when bug 1175643 lands in BMO.

Is this comment still valid?

::: pylib/mozreview/mozreview/views.py:42
(Diff revision 5)
> +def get_bmo_auth_callback(request):

this really needs a docstring explaining how this fits into everything, it's quite a large function.

I'd love to see the high level steps called out for how this process works.

::: pylib/mozreview/mozreview/views.py:123
(Diff revision 5)
> +def post_bmo_auth_callback(request):

docstring please
Attachment #8614313 - Flags: review?(smacleod)
Assignee

Comment 103

4 years ago
https://reviewboard.mozilla.org/r/9887/#review14665

> This stuff needs a serious docstring / justification for hash choice and the security requirements around this.
> 
> as it is I'm not comfortable with the secret key / api key being part of this hash.
> 
> to me it seems this purpose is just some random GUID to match up the keys, can we just generate something like that instead?

Yeah in my original design I wanted something reproducible that we didn't have to store... but of course we have to store the API key itself until it's validated.  So yeah, I'll just generate a UUID and store that.
Assignee

Comment 104

4 years ago
Comment on attachment 8614312 [details]
MozReview Request: mozreview: Use Bugzilla's public URL in the rbweb container (bug 993233). r=gps

mozreview: Use Bugzilla's public URL in the rbweb container (bug 993233). r=gps

The public URL is required in order to redirect the user to Bugzilla to
log in.
Assignee

Comment 105

4 years ago
Comment on attachment 8634076 [details]
MozReview Request: mozreview: Enable auth delegation in bmoweb (bug 993233). r=gps

mozreview: Enable auth delegation in bmoweb (bug 993233). r=gps
Assignee

Comment 106

4 years ago
Comment on attachment 8634077 [details]
MozReview Request: mozreview: Move Bugzilla client code over to mozreview extension (bug 993233). r=dminor

mozreview: Move Bugzilla client code over to mozreview extension (bug 993233). r=dminor

This is part of a larger effort to consolidate all Review Board customizations
in a single extension (mozreview).

Bump mozreview to 0.1.1alpha and rbbz to 0.2.9.
Assignee

Comment 107

4 years ago
Comment on attachment 8634078 [details]
MozReview Request: mozreview: Add api_key field to BugzillaUserMap (bug 993233). r=smacleod

mozreview: Add api_key field to BugzillaUserMap (bug 993233). r=smacleod

Bump mozreview version to 0.1.2alpha.  This requires a database schema
migration to add a nullable varchar(40) field with the name api_key to the
mozreview_bugzillausermap table.
Assignee

Comment 108

4 years ago
Comment on attachment 8634079 [details]
MozReview Request: mozreview: Add function to store Bugzilla API keys (bug 993233). r=smacleod

mozreview: Add function to store Bugzilla API keys (bug 993233). r=smacleod
Assignee

Comment 109

4 years ago
Comment on attachment 8634080 [details]
MozReview Request: mozreview: Add function to Bugzilla client to validate api keys (bug 993233). r=smacleod

mozreview: Add function to Bugzilla client to validate api keys (bug 993233). r=smacleod
Assignee

Comment 110

4 years ago
Comment on attachment 8614313 [details]
MozReview Request: mozreview: Add auth-delegation callback view (bug 993233). r?smacleod

mozreview: Add auth-delegation callback view (bug 993233). r?smacleod

This provides the necessary endpoint for Bugzilla's auth delegation.  It
calls BugzillaBackend.authenticate() to verify the API key and, if it checks
out, calls login() to create a Review Board session.

The local docker setup has been updated to set this new endpoint in the
Bugzilla config.

It requires a new table, mozreview_unverifiedbugzillaapikey, defined as such:

CREATE TABLE "mozreview_unverifiedbugzillaapikey" (
    "id" integer NOT NULL PRIMARY KEY,
    "bmo_username" varchar(255) NOT NULL,
    "api_key" varchar(40) NOT NULL,
    "timestamp" datetime NOT NULL,
    "callback_result" varchar(36) NOT NULL
);
CREATE INDEX "mozreview_unverifiedbugzillaapikey_26440ee6" ON "mozreview_unverifiedbugzillaapikey" ("bmo_username");
Attachment #8614313 - Flags: review?(smacleod)
Assignee

Comment 111

4 years ago
Comment on attachment 8614314 [details]
MozReview Request: mozreview: Use "mozreview" instead of "adminbugzilla" when creating users (bug 993233). r=gps

mozreview: Use "mozreview" instead of "adminbugzilla" when creating users (bug 993233). r=gps

The "mozreview create-user" mach command has options to create all aspects
of a mozreview user, so we consolidate all the "adminbugzilla create-user"
and "mozreview create-ldap-user" calls into just "mozreview create-user"
for simplicity.
Assignee

Comment 112

4 years ago
Comment on attachment 8614311 [details]
MozReview Request: mozreview: Authenticate to BMO with API key (bug 993233). r?smacleod

mozreview: Authenticate to BMO with API key (bug 993233). r?smacleod

Send Bugzilla API keys with authenticated requests rather than cookies.
Fix up tests to ensure API keys are created where needed.
Assignee

Comment 113

4 years ago
Comment on attachment 8636774 [details]
MozReview Request: mozreview: Rename common.js to init_rr.js and clean it up (bug 993233). r=smacleod

mozreview: Rename common.js to init_rr.js and clean it up (bug 993233). r=smacleod

"common.js" is misleading because it is only used on the review-request view,
and furthermore it doesn't describe what it does.
Assignee

Comment 114

4 years ago
Comment on attachment 8634081 [details]
MozReview Request: mozreview: Override login page to use Bugzilla auth delegation (bug 993233). r=smacleod

mozreview: Override login page to use Bugzilla auth delegation (bug 993233). r=smacleod

Passing a "full" query-string parameter with any value, e.g. "full=true",
in the login URL results in the full login page being displayed. This exists
primarily to allow the admin user to log in, which does not have a
corresponding Bugzilla account.

Also override the "Log Out" link to not redirect to the login page, since if
you still have an open Bugzilla session, you'll be automatically logged back
into Review Board.

Selenium tests have been updated, but not all appear to be passing on tip
right now, so there are still failures.
Assignee

Comment 115

4 years ago
Comment on attachment 8634082 [details]
MozReview Request: mozreview: Stop storing Bugzilla cookies (bug 993233). r=smacleod

mozreview: Stop storing Bugzilla cookies (bug 993233). r=smacleod

Since all communication is now done via API keys, we no longer need to
store cookies in the user object.

For the purposes of identification with MozReview, logging in with username
and password or login cookies is still possible via the API and with
/account/login/?full=true (or any other value for "full"). The latter is
intended only for the admin account, as it will not cause an API key to be
generated and sent over from Bugzilla, meaning most calls from Review Board
to Bugzilla will fail. In both cases, the cookie/password is only used to
verify the account with Bugzilla and is discarded immediately after.
Attachment #8614313 - Flags: review?(smacleod)
Comment on attachment 8614313 [details]
MozReview Request: mozreview: Add auth-delegation callback view (bug 993233). r?smacleod

https://reviewboard.mozilla.org/r/9887/#review15101

::: pylib/mozreview/mozreview/views.py:45
(Diff revision 6)
> +    back to MozReview.  This handler then returns a unique result string,

the fact that "This handler *then* returns ..." comes after "when the user is redirected back to MozReview" makes the ordering a little confusing / ambiguous.

Can you reword things a bit to make them more clear?

::: pylib/mozreview/mozreview/views.py:99
(Diff revision 6)
> +    if not unverified_keys:
> +        msg = 'No unverified keys found.'
> +        logging.error(msg)
> +        return show_error_page(request, msg)

Errors like this means either we're being maliciously attacked, or something went very wrong. We should avoid explaining exactly what has failed when the process fails, as none of it is really user actionable, and it could help attackers.

Example: I can make requests to this endpoint with email addresses and check if they have an unverified key or not. Since we don't actually clean these up I could find a user with one and try to guess the uuid or something (I don't think there is an actual viable attack here, but I'd rather not provide information that could help)

::: pylib/mozreview/mozreview/views.py:112
(Diff revision 6)
> +        msg = 'Callback result does not match.'

ya, the user facing error should be generic, and just tell them to retry logging in or something.

::: pylib/mozreview/mozreview/views.py:127
(Diff revision 6)
> +        return show_error_page(request, 'Error validating API key: %s' % e.msg)

just log it, generic error.

::: pylib/mozreview/mozreview/views.py:134
(Diff revision 6)
> +        return show_error_page(request, 'Error getting user data: %s' % e.msg)

At this point we've verified the API key, so the login is *kind* of authentic now, so you could show more verbose errors at this point if you want (I'd only suggest it if they're actionable though)
Assignee

Comment 117

4 years ago
Comment on attachment 8614313 [details]
MozReview Request: mozreview: Add auth-delegation callback view (bug 993233). r?smacleod

mozreview: Add auth-delegation callback view (bug 993233). r?smacleod

This provides the necessary endpoint for Bugzilla's auth delegation.  It
calls BugzillaBackend.authenticate() to verify the API key and, if it checks
out, calls login() to create a Review Board session.

The local docker setup has been updated to set this new endpoint in the
Bugzilla config.

It requires a new table, mozreview_unverifiedbugzillaapikey, defined as such:

CREATE TABLE "mozreview_unverifiedbugzillaapikey" (
    "id" integer NOT NULL PRIMARY KEY,
    "bmo_username" varchar(255) NOT NULL,
    "api_key" varchar(40) NOT NULL,
    "timestamp" datetime NOT NULL,
    "callback_result" varchar(36) NOT NULL
);
CREATE INDEX "mozreview_unverifiedbugzillaapikey_26440ee6" ON "mozreview_unverifiedbugzillaapikey" ("bmo_username");
Attachment #8614313 - Flags: review?(smacleod)
Assignee

Comment 118

4 years ago
Comment on attachment 8614314 [details]
MozReview Request: mozreview: Use "mozreview" instead of "adminbugzilla" when creating users (bug 993233). r=gps

mozreview: Use "mozreview" instead of "adminbugzilla" when creating users (bug 993233). r=gps

The "mozreview create-user" mach command has options to create all aspects
of a mozreview user, so we consolidate all the "adminbugzilla create-user"
and "mozreview create-ldap-user" calls into just "mozreview create-user"
for simplicity.
Assignee

Comment 119

4 years ago
Comment on attachment 8614311 [details]
MozReview Request: mozreview: Authenticate to BMO with API key (bug 993233). r?smacleod

mozreview: Authenticate to BMO with API key (bug 993233). r?smacleod

Send Bugzilla API keys with authenticated requests rather than cookies.
Fix up tests to ensure API keys are created where needed.
Assignee

Comment 120

4 years ago
Comment on attachment 8636774 [details]
MozReview Request: mozreview: Rename common.js to init_rr.js and clean it up (bug 993233). r=smacleod

mozreview: Rename common.js to init_rr.js and clean it up (bug 993233). r=smacleod

"common.js" is misleading because it is only used on the review-request view,
and furthermore it doesn't describe what it does.
Assignee

Comment 121

4 years ago
Comment on attachment 8634081 [details]
MozReview Request: mozreview: Override login page to use Bugzilla auth delegation (bug 993233). r=smacleod

mozreview: Override login page to use Bugzilla auth delegation (bug 993233). r=smacleod

Passing a "full" query-string parameter with any value, e.g. "full=true",
in the login URL results in the full login page being displayed. This exists
primarily to allow the admin user to log in, which does not have a
corresponding Bugzilla account.

Also override the "Log Out" link to not redirect to the login page, since if
you still have an open Bugzilla session, you'll be automatically logged back
into Review Board.

Selenium tests have been updated, but not all appear to be passing on tip
right now, so there are still failures.
Assignee

Comment 122

4 years ago
Comment on attachment 8634082 [details]
MozReview Request: mozreview: Stop storing Bugzilla cookies (bug 993233). r=smacleod

mozreview: Stop storing Bugzilla cookies (bug 993233). r=smacleod

Since all communication is now done via API keys, we no longer need to
store cookies in the user object.

For the purposes of identification with MozReview, logging in with username
and password or login cookies is still possible via the API and with
/account/login/?full=true (or any other value for "full"). The latter is
intended only for the admin account, as it will not cause an API key to be
generated and sent over from Bugzilla, meaning most calls from Review Board
to Bugzilla will fail. In both cases, the cookie/password is only used to
verify the account with Bugzilla and is discarded immediately after.
Comment on attachment 8614313 [details]
MozReview Request: mozreview: Add auth-delegation callback view (bug 993233). r?smacleod

https://reviewboard.mozilla.org/r/9887/#review15137

::: pylib/mozreview/mozreview/views.py:101
(Diff revisions 6 - 7)
> -        msg = 'No unverified keys found.'
> +        logging.error('No unverified keys found.')

It would be useful to give information about which username etc so we can actually followup a bit more.

::: pylib/mozreview/mozreview/views.py:112
(Diff revisions 6 - 7)
> -        msg = 'Callback result does not match.'
> +        logging.error('Callback result does not match.')

Maybe we should log the unmatched callback_results here, so we could track what's going on? or at least the one that was provided by the request?

Or maybe that's insecure... hmmm
Attachment #8614313 - Flags: review?(smacleod)
Assignee

Comment 124

4 years ago
https://reviewboard.mozilla.org/r/9887/#review15137

> Maybe we should log the unmatched callback_results here, so we could track what's going on? or at least the one that was provided by the request?
> 
> Or maybe that's insecure... hmmm

I'll add this if needed later.  I did add the username though.
Assignee

Comment 125

4 years ago
Comment on attachment 8614313 [details]
MozReview Request: mozreview: Add auth-delegation callback view (bug 993233). r?smacleod

mozreview: Add auth-delegation callback view (bug 993233). r?smacleod

This provides the necessary endpoint for Bugzilla's auth delegation.  It
calls BugzillaBackend.authenticate() to verify the API key and, if it checks
out, calls login() to create a Review Board session.

The local docker setup has been updated to set this new endpoint in the
Bugzilla config.

It requires a new table, mozreview_unverifiedbugzillaapikey, defined as such:

CREATE TABLE "mozreview_unverifiedbugzillaapikey" (
    "id" integer NOT NULL PRIMARY KEY,
    "bmo_username" varchar(255) NOT NULL,
    "api_key" varchar(40) NOT NULL,
    "timestamp" datetime NOT NULL,
    "callback_result" varchar(36) NOT NULL
);
CREATE INDEX "mozreview_unverifiedbugzillaapikey_26440ee6" ON "mozreview_unverifiedbugzillaapikey" ("bmo_username");
Attachment #8614313 - Flags: review?(smacleod)
Assignee

Comment 126

4 years ago
Comment on attachment 8614314 [details]
MozReview Request: mozreview: Use "mozreview" instead of "adminbugzilla" when creating users (bug 993233). r=gps

mozreview: Use "mozreview" instead of "adminbugzilla" when creating users (bug 993233). r=gps

The "mozreview create-user" mach command has options to create all aspects
of a mozreview user, so we consolidate all the "adminbugzilla create-user"
and "mozreview create-ldap-user" calls into just "mozreview create-user"
for simplicity.
Assignee

Comment 127

4 years ago
Comment on attachment 8614311 [details]
MozReview Request: mozreview: Authenticate to BMO with API key (bug 993233). r?smacleod

mozreview: Authenticate to BMO with API key (bug 993233). r?smacleod

Send Bugzilla API keys with authenticated requests rather than cookies.
Fix up tests to ensure API keys are created where needed.
Assignee

Comment 128

4 years ago
Comment on attachment 8636774 [details]
MozReview Request: mozreview: Rename common.js to init_rr.js and clean it up (bug 993233). r=smacleod

mozreview: Rename common.js to init_rr.js and clean it up (bug 993233). r=smacleod

"common.js" is misleading because it is only used on the review-request view,
and furthermore it doesn't describe what it does.
Assignee

Comment 129

4 years ago
Comment on attachment 8634081 [details]
MozReview Request: mozreview: Override login page to use Bugzilla auth delegation (bug 993233). r=smacleod

mozreview: Override login page to use Bugzilla auth delegation (bug 993233). r=smacleod

Passing a "full" query-string parameter with any value, e.g. "full=true",
in the login URL results in the full login page being displayed. This exists
primarily to allow the admin user to log in, which does not have a
corresponding Bugzilla account.

Also override the "Log Out" link to not redirect to the login page, since if
you still have an open Bugzilla session, you'll be automatically logged back
into Review Board.

Selenium tests have been updated, but not all appear to be passing on tip
right now, so there are still failures.
Assignee

Comment 130

4 years ago
Comment on attachment 8634082 [details]
MozReview Request: mozreview: Stop storing Bugzilla cookies (bug 993233). r=smacleod

mozreview: Stop storing Bugzilla cookies (bug 993233). r=smacleod

Since all communication is now done via API keys, we no longer need to
store cookies in the user object.

For the purposes of identification with MozReview, logging in with username
and password or login cookies is still possible via the API and with
/account/login/?full=true (or any other value for "full"). The latter is
intended only for the admin account, as it will not cause an API key to be
generated and sent over from Bugzilla, meaning most calls from Review Board
to Bugzilla will fail. In both cases, the cookie/password is only used to
verify the account with Bugzilla and is discarded immediately after.
Comment on attachment 8614313 [details]
MozReview Request: mozreview: Add auth-delegation callback view (bug 993233). r?smacleod

https://reviewboard.mozilla.org/r/9887/#review15155

Ship It!
Attachment #8614313 - Flags: review?(smacleod) → review+
Assignee

Comment 132

4 years ago
Comment on attachment 8614312 [details]
MozReview Request: mozreview: Use Bugzilla's public URL in the rbweb container (bug 993233). r=gps

mozreview: Use Bugzilla's public URL in the rbweb container (bug 993233). r=gps

The public URL is required in order to redirect the user to Bugzilla to
log in.
Assignee

Comment 133

4 years ago
Comment on attachment 8634076 [details]
MozReview Request: mozreview: Enable auth delegation in bmoweb (bug 993233). r=gps

mozreview: Enable auth delegation in bmoweb (bug 993233). r=gps
Assignee

Comment 134

4 years ago
Comment on attachment 8634077 [details]
MozReview Request: mozreview: Move Bugzilla client code over to mozreview extension (bug 993233). r=dminor

mozreview: Move Bugzilla client code over to mozreview extension (bug 993233). r=dminor

This is part of a larger effort to consolidate all Review Board customizations
in a single extension (mozreview).

Bump mozreview to 0.1.1alpha and rbbz to 0.2.9.
Assignee

Comment 135

4 years ago
Comment on attachment 8634078 [details]
MozReview Request: mozreview: Add api_key field to BugzillaUserMap (bug 993233). r=smacleod

mozreview: Add api_key field to BugzillaUserMap (bug 993233). r=smacleod

Bump mozreview version to 0.1.2alpha.  This requires a database schema
migration to add a nullable varchar(40) field with the name api_key to the
mozreview_bugzillausermap table.
Assignee

Comment 136

4 years ago
Comment on attachment 8634079 [details]
MozReview Request: mozreview: Add function to store Bugzilla API keys (bug 993233). r=smacleod

mozreview: Add function to store Bugzilla API keys (bug 993233). r=smacleod
Assignee

Comment 137

4 years ago
Comment on attachment 8634080 [details]
MozReview Request: mozreview: Add function to Bugzilla client to validate api keys (bug 993233). r=smacleod

mozreview: Add function to Bugzilla client to validate api keys (bug 993233). r=smacleod
Assignee

Comment 138

4 years ago
Comment on attachment 8614313 [details]
MozReview Request: mozreview: Add auth-delegation callback view (bug 993233). r?smacleod

mozreview: Add auth-delegation callback view (bug 993233). r?smacleod

This provides the necessary endpoint for Bugzilla's auth delegation.  It
calls BugzillaBackend.authenticate() to verify the API key and, if it checks
out, calls login() to create a Review Board session.

The local docker setup has been updated to set this new endpoint in the
Bugzilla config.

It requires a new table, mozreview_unverifiedbugzillaapikey, defined as such:

CREATE TABLE "mozreview_unverifiedbugzillaapikey" (
    "id" integer NOT NULL PRIMARY KEY,
    "bmo_username" varchar(255) NOT NULL,
    "api_key" varchar(40) NOT NULL,
    "timestamp" datetime NOT NULL,
    "callback_result" varchar(36) NOT NULL
);
CREATE INDEX "mozreview_unverifiedbugzillaapikey_26440ee6" ON "mozreview_unverifiedbugzillaapikey" ("bmo_username");
Assignee

Comment 139

4 years ago
Comment on attachment 8614314 [details]
MozReview Request: mozreview: Use "mozreview" instead of "adminbugzilla" when creating users (bug 993233). r=gps

mozreview: Use "mozreview" instead of "adminbugzilla" when creating users (bug 993233). r=gps

The "mozreview create-user" mach command has options to create all aspects
of a mozreview user, so we consolidate all the "adminbugzilla create-user"
and "mozreview create-ldap-user" calls into just "mozreview create-user"
for simplicity.
Assignee

Comment 140

4 years ago
Comment on attachment 8614311 [details]
MozReview Request: mozreview: Authenticate to BMO with API key (bug 993233). r?smacleod

mozreview: Authenticate to BMO with API key (bug 993233). r?smacleod

Send Bugzilla API keys with authenticated requests rather than cookies.
Fix up tests to ensure API keys are created where needed.
Assignee

Comment 141

4 years ago
Comment on attachment 8636774 [details]
MozReview Request: mozreview: Rename common.js to init_rr.js and clean it up (bug 993233). r=smacleod

mozreview: Rename common.js to init_rr.js and clean it up (bug 993233). r=smacleod

"common.js" is misleading because it is only used on the review-request view,
and furthermore it doesn't describe what it does.
Assignee

Comment 142

4 years ago
Comment on attachment 8634081 [details]
MozReview Request: mozreview: Override login page to use Bugzilla auth delegation (bug 993233). r=smacleod

mozreview: Override login page to use Bugzilla auth delegation (bug 993233). r=smacleod

Passing a "full" query-string parameter with any value, e.g. "full=true",
in the login URL results in the full login page being displayed. This exists
primarily to allow the admin user to log in, which does not have a
corresponding Bugzilla account.

Also override the "Log Out" link to not redirect to the login page, since if
you still have an open Bugzilla session, you'll be automatically logged back
into Review Board.

Selenium tests have been updated, but not all appear to be passing on tip
right now, so there are still failures.
Assignee

Comment 143

4 years ago
Comment on attachment 8634082 [details]
MozReview Request: mozreview: Stop storing Bugzilla cookies (bug 993233). r=smacleod

mozreview: Stop storing Bugzilla cookies (bug 993233). r=smacleod

Since all communication is now done via API keys, we no longer need to
store cookies in the user object.

For the purposes of identification with MozReview, logging in with username
and password or login cookies is still possible via the API and with
/account/login/?full=true (or any other value for "full"). The latter is
intended only for the admin account, as it will not cause an API key to be
generated and sent over from Bugzilla, meaning most calls from Review Board
to Bugzilla will fail. In both cases, the cookie/password is only used to
verify the account with Bugzilla and is discarded immediately after.
Assignee

Updated

4 years ago
Attachment #8634081 - Flags: review+ → review?(smacleod)
Assignee

Updated

4 years ago
Depends on: 1198490
Assignee

Comment 144

4 years ago
Comment on attachment 8634081 [details]
MozReview Request: mozreview: Override login page to use Bugzilla auth delegation (bug 993233). r=smacleod

mozreview: Override login page to use Bugzilla auth delegation (bug 993233). r=smacleod

Passing a "full" query-string parameter with any value, e.g. "full=true",
in the login URL results in the full login page being displayed. This exists
primarily to allow the admin user to log in, which does not have a
corresponding Bugzilla account.

Also override the "Log Out" link to not redirect to the login page, since if
you still have an open Bugzilla session, you'll be automatically logged back
into Review Board.

Selenium tests have been updated, but not all appear to be passing on tip
right now, so there are still failures.
Assignee

Comment 145

4 years ago
Comment on attachment 8634082 [details]
MozReview Request: mozreview: Stop storing Bugzilla cookies (bug 993233). r=smacleod

mozreview: Stop storing Bugzilla cookies (bug 993233). r=smacleod

Since all communication is now done via API keys, we no longer need to
store cookies in the user object.

For the purposes of identification with MozReview, logging in with username
and password or login cookies is still possible via the API and with
/account/login/?full=true (or any other value for "full"). The latter is
intended only for the admin account, as it will not cause an API key to be
generated and sent over from Bugzilla, meaning most calls from Review Board
to Bugzilla will fail. In both cases, the cookie/password is only used to
verify the account with Bugzilla and is discarded immediately after.

Updated

4 years ago
Duplicate of this bug: 1198702
https://reviewboard.mozilla.org/r/9887/#review15361

::: pylib/mozreview/mozreview/views.py:61
(Diff revision 9)
> +        timestamp=timezone.now(),

I believe you can default the timestamp on the model to whenever it's created, that way you don't have to perform this. I don't really care too much though, so use your judgement.
Assignee

Comment 149

4 years ago
This is live on prod.
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Attachment #8634081 - Flags: review?(smacleod) → review+
Product: Developer Services → MozReview
You need to log in before you can comment on or make changes to this bug.