1244448 - All the diffset in a review request should be verified

Status: RESOLVED FIXED

(MozReview Graveyard :: General, defect, P1)

Description

[Mauro Doglio [:mdoglio]]

To solve bug 1243484 we need to verify that what's shown in a diffset is the same as what's stored in the reviewboard-hg server.

Comment 1

[Steven MacLeod [:smacleod]]

Attachment: Part 01: mozreview: switch import style

Comment 2

[Steven MacLeod [:smacleod]]

Attachment: Part 02: mozreview: Add models to hold commit information

To remove our use of built-in, API accessible, extra_data attributes
we need to store all of this information in the database. Create a
number of models for holding this information.

With the new data storage we'll have control over indices and should be
able to take advantage of this to query things directly and add features
that weren't possible.

An important concept introduced with these models is that of a verified
DiffSet: Any DiffSet uploaded to MozReview by the hg server will have
a verification entry created for it (This operation will require the
special permission introduced). This will allow us to prevent
unauthorized diffs from being published, guaranteeing any diff shown
on MozReview reflects the commit backing it in hg.

Use of the VerifiedDiffSet, as well as the other models, will come in
following changes.

Comment 3

[Steven MacLeod [:smacleod]]

Attachment: Part 03: mozreview: grant the mozreview user the verify_diffset permission

Comment 4

[Steven MacLeod [:smacleod]]

Attachment: Part 04: mozreview: refactor API key verification into standalone function

We're going to be verifying a Bugzilla API key in other resources so
pull out the code into a function we can call. It is a little bit
strange that we return either a user object or a WebAPIResponseError,
but we'll only be using this in API calls which will want to generate
those responses anyways.

Comment 5

[Steven MacLeod [:smacleod]]

Attachment: Part 05: mozreview: require special permission to access `BatchReviewRequestResource.create`

We now require the mozreview.verify_diffset permission to access the
batch review request creation resource. This means we can trust the
actions of this resource as coming from the hg server. This will be
important in future commits where we mark diffsets as verified using
the resource.

Also fixes the missing i in the spelling of privileged_rb_username and
privileged_rb_password.

Comment 6

[Steven MacLeod [:smacleod]]

Attachment: Part 02: mozreview: Add models to hold commit information

To remove our use of built-in, API accessible, extra_data attributes
we need to store all of this information in the database. Create a
number of models for holding this information.

With the new data storage we'll have control over indices and should be
able to take advantage of this to query things directly and add features
that weren't possible.

An important concept introduced with these models is that of a verified
DiffSet: Any DiffSet uploaded to MozReview by the hg server will have
a verification entry created for it (This operation will require the
special permission introduced). This will allow us to prevent
unauthorized diffs from being published, guaranteeing any diff shown
on MozReview reflects the commit backing it in hg.

Use of the VerifiedDiffSet, as well as the other models, will come in
following changes.

Comment 7

[Steven MacLeod [:smacleod]]

Attachment: Part 03: mozreview: grant the mozreview user the verify_diffset permission

Comment 8

[Steven MacLeod [:smacleod]]

Attachment: Part 06: mozreview: verify DiffSets during batch creation

DiffSetVerifications are created for each DiffSet that is uploaded
as part of batch review request creation. Actually checking these
verifications will come in a future change.

Additionally, providing a user with the verify_diffset permission is now
required in order to save a DiffSetVerification. While it wasn't
strictly necessary to add this, it means you're forced to think about
permissions whenever adding a place DiffSets get verified.

Comment 9

[Steven MacLeod [:smacleod]]

Attachment: Part 07: mozreview: create a signal_handlers.py

To prepare for moving a number of signal handlers from rbbz to MozReview
create a signal_handlers.py where they will live. The first signal to
be part of this file comes from MozReview itself and has been moved
from extension.py (with no changes to functionality).

I did take this chance to rename the signal handler and add a docstring
to explain its functionality. I also cleaned up the import styling for
both files and removed some unused imports.

Comment 10

[Steven MacLeod [:smacleod]]

Attachment: Part 08: mozreview: move on_review_request_publishing into MozReview from rbbz

This commit mostly just moves code from rbbz into MozReview, centered
around what was needed to move on_review_request_publishing(). Several
new files were created to hold various utility functions which were
part of rbbz's extenion.py but the code should mostly just be a copy.

A few of the functions relating to extra data, and accessing it, were
changed from the implementations present in rbbz to those in
mozreview.extra_data, but functionality should remain the same.

Comment 11

[Mauro Doglio [:mdoglio]]

Comment on attachment 8714043 [details] [diff] [review]
Part 06: mozreview: verify DiffSets during batch creation

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

::: pylib/mozreview/mozreview/commits/models.py
@@ +37,5 @@
> +        to save to prevent accidental verifications of a DiffSet in any
> +        code added in the future.
> +        """
> +        if authorized_user is None:
> +            raise ValueError('A DiffsetVerification may not be saved without '

I think raising a django.core.exceptions.ValidationError would be more specific.

@@ +41,5 @@
> +            raise ValueError('A DiffsetVerification may not be saved without '
> +                             'providing a User object with the verify_diffset '
> +                             'permission.')
> +        elif not authorized_user.has_perm('mozreview.verify_diffset'):
> +            raise ValueError('Provided User does not have permission to '

same here

Comment 12

[Steven MacLeod [:smacleod]]

Attachment: Part 06: mozreview: verify DiffSets during batch creation

DiffSetVerifications are created for each DiffSet that is uploaded
as part of batch review request creation. Actually checking these
verifications will come in a future change.

Additionally, providing a user with the verify_diffset permission is now
required in order to save a DiffSetVerification. While it wasn't
strictly necessary to add this, it means you're forced to think about
permissions whenever adding a place DiffSets get verified.

Comment 13

[Mark Côté [:mcote]]

Comment on attachment 8714045 [details] [diff] [review]
Part 08: mozreview: move on_review_request_publishing into MozReview from rbbz

+DRAFTED_EXTRA_DATA_KEYS = (
+    COMMIT_ID_KEY,
+    'p2rb.first_public_ancestor',
+    IDENTIFIER_KEY,
+)

Just curious--why did you not turn the second one into a _KEY variable like the others?

+        review_request.extra_data['p2rb.unpublished_rids'] = '[]'
+        review_request.extra_data['p2rb.discard_on_publish_rids'] = '[]'

Use the variables from mozreview.extra_data?

Comment 14

[Gregory Szorc [:gps]]

Comment on attachment 8714041 [details] [diff] [review]
Part 02: mozreview: Add models to hold commit information

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

I would rather we not tackle the establishment of dedicated models for commit tracking in this bug. Can we do it as a follow-up? Perhaps we can lock down the review request / draft POST APIs instead and require the batch submit API (which can only be used by the privileged user).

Comment 15

[Steven MacLeod [:smacleod]]

(In reply to Gregory Szorc [:gps] from comment #14)
> Comment on attachment 8714041 [details] [diff] [review]
> Part 02: mozreview: Add models to hold commit information
> 
> Review of attachment 8714041 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I would rather we not tackle the establishment of dedicated models for
> commit tracking in this bug. Can we do it as a follow-up? Perhaps we can
> lock down the review request / draft POST APIs instead and require the batch
> submit API (which can only be used by the privileged user).

Locking down those APIs is probably possible (I mean, we are in python) but designing and implementing that is going to be incredibly hacky.

I've got it working with all the tests passing where DiffSets are locked down and we only allow publishing ones that have come from the hg server user. That still leaves us open to being tricked by having the extra data manipulated to show other verified diffs / trick autoland to land a different diff. We could always deploy the first part of the fix and finish locking things down but then the security cat is out of the bag.

Comment 16

[Steven MacLeod [:smacleod]]

(In reply to Mark Côté [:mcote] from comment #13)
> Comment on attachment 8714045 [details] [diff] [review]
> Part 08: mozreview: move on_review_request_publishing into MozReview from
> rbbz
> 
> +DRAFTED_EXTRA_DATA_KEYS = (
> +    COMMIT_ID_KEY,
> +    'p2rb.first_public_ancestor',
> +    IDENTIFIER_KEY,
> +)
> 
> Just curious--why did you not turn the second one into a _KEY variable like
> the others?
> 
> +        review_request.extra_data['p2rb.unpublished_rids'] = '[]'
> +        review_request.extra_data['p2rb.discard_on_publish_rids'] = '[]'
> 
> Use the variables from mozreview.extra_data?

I was trying to keep changes to existing working code as minimal as possible, so I'm going to just leave these as is for now and clean things up later (a lot of the extra data stuff will just disappear anyways)

Comment 17

[Steven MacLeod [:smacleod]]

Attachment: Part 09: mozreview: prevent publishing of unverified DiffSets

We now prevent any publish operation on a review request if it contains
a DiffSet without a corresponding DiffSetVerification. This prevents any
DiffSet which didn't come from the review server from being published.

This should fix the most obvious security issue of a fake DiffSet being
uploaded to represent the underlying commit. We still need to move away
from extra_data completely in order to prevent other issues.

Comment 18

[Steven MacLeod [:smacleod]]

:gps and I just discussed this a bit more in IRC. Something we thought up as a simple fix for extra data problems would be to:
 - Create a single model with a ForeignKey to a ReviewRequest
   - Give this model a JSONField
 - Migrate the current extra_data into our models JSONField
 - Make any sensitive information stored there go through a protected API (such as the batch creation one)
 - Update all our code to use this extra data (should be easier than using the new models).

I really do want us to move everything to the models I proposed, but if we can get away with implementing this solution quickly now it's probably a better idea, and then we can move to the proper schema at a leisurely pace.

Comment 19

[Dan Minor [:dminor]]

Comment on attachment 8714041 [details] [diff] [review]
Part 02: mozreview: Add models to hold commit information

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

Looks good. Feel free to ignore my naming suggestions in the interests of expediency :)

::: pylib/mozreview/mozreview/commits/models.py
@@ +17,5 @@
> +# backwards from a built-in model. For context, see
> +# https://reviews.reviewboard.org/r/6224/
> +
> +
> +class DiffSetVerification(models.Model):

Sorry I missed this earlier, but maybe we should use the word 'trusted' rather than 'verification'. We're not really verifying the diffset, we're trusting it because it comes from the hg server.

@@ +55,5 @@
> +        ReviewRequest,
> +        null=True,
> +        db_index=True,
> +        help_text=_('The review request representing this commit.'))
> +

nit: remove extra blank line.

@@ +108,5 @@
> +    class Meta:
> +        app_label = 'mozreview'
> +
> +
> +class Identifier(models.Model):

Maybe call this ReviewIdentifier?

@@ +139,5 @@
> +    diffset_verification = models.OneToOneField(
> +        DiffSetVerification,
> +        help_text=_('The Verification for the DiffSet representing this '
> +                    'set of commits squashed together.'))
> +

nit: please remove extra blank line

Comment 20

[Steven MacLeod [:smacleod]]

(In reply to Dan Minor [:dminor] from comment #19)
> Comment on attachment 8714041 [details] [diff] [review]
> Part 02: mozreview: Add models to hold commit information
> 
> Review of attachment 8714041 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good. Feel free to ignore my naming suggestions in the interests of
> expediency :)
> 
> ::: pylib/mozreview/mozreview/commits/models.py
> @@ +17,5 @@
> > +# backwards from a built-in model. For context, see
> > +# https://reviews.reviewboard.org/r/6224/
> > +
> > +
> > +class DiffSetVerification(models.Model):
> 
> Sorry I missed this earlier, but maybe we should use the word 'trusted'
> rather than 'verification'. We're not really verifying the diffset, we're
> trusting it because it comes from the hg server.

What we've verified is that the DiffSet has come from the hg server. I think this represents what's going on decently well so I'm going to leave the name as is "in the interests of expediency" :)


> @@ +55,5 @@
> > +        ReviewRequest,
> > +        null=True,
> > +        db_index=True,
> > +        help_text=_('The review request representing this commit.'))
> > +
> 
> nit: remove extra blank line.
> 
> @@ +139,5 @@
> > +    diffset_verification = models.OneToOneField(
> > +        DiffSetVerification,
> > +        help_text=_('The Verification for the DiffSet representing this '
> > +                    'set of commits squashed together.'))
> > +
> 
> nit: please remove extra blank line

These are here to mark when relationship fields end and data fields stored on the model start. I'm going to leave them.


> @@ +108,5 @@
> > +    class Meta:
> > +        app_label = 'mozreview'
> > +
> > +
> > +class Identifier(models.Model):
> 
> Maybe call this ReviewIdentifier?

This is getting nuked for the security change so we can revisit the naming later.

Comment 21

[Steven MacLeod [:smacleod]]

Attachment: Part 02: mozreview: Add model to track DiffSets from the hg server

An important concept introduced with this model is that of a verified
DiffSet: Any DiffSet uploaded to MozReview by the hg server will have
a verification entry created for it (This operation will require the
special permission introduced). This will allow us to prevent
unauthorized diffs from being published, guaranteeing any diff shown
on MozReview reflects the commit backing it in hg.

Use of VerifiedDiffSet will come in following changes.

Comment 22

[Dan Minor [:dminor]]

(In reply to Steven MacLeod [:smacleod] from comment #20)
> (In reply to Dan Minor [:dminor] from comment #19)
> > Comment on attachment 8714041 [details] [diff] [review]
> > Part 02: mozreview: Add models to hold commit information
> > 
> > Review of attachment 8714041 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Looks good. Feel free to ignore my naming suggestions in the interests of
> > expediency :)
> > 
> > ::: pylib/mozreview/mozreview/commits/models.py
> > @@ +17,5 @@
> > > +# backwards from a built-in model. For context, see
> > > +# https://reviews.reviewboard.org/r/6224/
> > > +
> > > +
> > > +class DiffSetVerification(models.Model):
> > 
> > Sorry I missed this earlier, but maybe we should use the word 'trusted'
> > rather than 'verification'. We're not really verifying the diffset, we're
> > trusting it because it comes from the hg server.
> 
> What we've verified is that the DiffSet has come from the hg server. I think
> this represents what's going on decently well so I'm going to leave the name
> as is "in the interests of expediency" :)
>

Yes, but there's nothing in the name that indicates that is what you've verified :) But like I said, I don't feel that strongly about this.

Comment 23

[Steven MacLeod [:smacleod]]

Attachment: Part 10: mozreview: Add model to replace extra_data use

We need a quick and dirty solution for storing our extra_data which the
review request author can't modify. CommitData acts as a simple
JSON store tied to the review request where we can transition our
data. This should make migration very simple as we can directly
copy the JSON to our new table.

Comment 24

[Mauro Doglio [:mdoglio]]

Comment on attachment 8714335 [details] [diff] [review]
Part 10: mozreview: Add model to replace extra_data use

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

lgtm

Comment 25

[Steven MacLeod [:smacleod]]

Attachment: Part 09: mozreview: prevent publishing of unverified DiffSets

We now prevent any publish operation on a review request if it contains
a DiffSet without a corresponding DiffSetVerification. This prevents any
DiffSet which didn't come from the review server from being published.

This should fix the most obvious security issue of a fake DiffSet being
uploaded to represent the underlying commit. We still need to move away
from extra_data completely in order to prevent other issues.

Comment 26

[Gregory Szorc [:gps]]

Comment on attachment 8714095 [details] [diff] [review]
Part 06: mozreview: verify DiffSets during batch creation

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

::: pylib/mozreview/mozreview/resources/batch_review_request.py
@@ +215,5 @@
>          """Create or update a review request series."""
>          if not request.user.has_perm('mozreview.verify_diffset'):
>              return PERMISSION_DENIED
>  
> +        privileged_user = request.user

This isn't necessarily the "privileged" user: it is the "request user." Please change the variable names.

@@ +294,5 @@
>  
>          try:
>              with transaction.atomic():
> +                (
> +                    squashed_rr,

Split this into multiple statements:

res = self._process_submission(...)
squashed_rr, node_to_rid, ... = res

@@ +376,5 @@
>              update_diffset_history(squashed_rr, diffset)
>              diffset.save()
>  
> +            DiffSetVerification(diffset=diffset).save(
> +                authorized_user=privileged_user, force_insert=True)

What does force_insert do?

Comment 27

[Gregory Szorc [:gps]]

Comment on attachment 8714044 [details] [diff] [review]
Part 07: mozreview: create a signal_handlers.py

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

Apparently this patch has Windows line endings.

Comment 28

[Gregory Szorc [:gps]]

Comment on attachment 8714395 [details] [diff] [review]
Part 09: mozreview: prevent publishing of unverified DiffSets

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

::: hgext/reviewboard/tests/test-diffset-verification.t
@@ +17,5 @@
> +
> +  $ echo foo1 > foo
> +  $ hg commit -m 'Bug 1 - Foo 1'
> +  $ hg push
> +  pushing to ssh://*:$HGPORT6/test-repo (glob)

No need for (glob) anymore.

::: testing/vcttesting/reviewboard/mach_commands.py
@@ +321,5 @@
> +    @CommandArgument(
> +        '--base-commit',
> +        help='Base commit id to apply diff to')
> +    def upload_diff(self, rrid, base_commit=None):
> +        import sys

Move this import to top of file.

Comment 29

[Steven MacLeod [:smacleod]]

Attachment: Part 06: mozreview: verify DiffSets during batch creation

DiffSetVerifications are created for each DiffSet that is uploaded
as part of batch review request creation. Actually checking these
verifications will come in a future change.

Additionally, providing a user with the verify_diffset permission is now
required in order to save a DiffSetVerification. While it wasn't
strictly necessary to add this, it means you're forced to think about
permissions whenever adding a place DiffSets get verified.

(In reply to Gregory Szorc [:gps] from comment #26)
> Comment on attachment 8714095 [details] [diff] [review]
> Part 06: mozreview: verify DiffSets during batch creation
> 
> Review of attachment 8714095 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: pylib/mozreview/mozreview/resources/batch_review_request.py
> @@ +215,5 @@
> >          """Create or update a review request series."""
> >          if not request.user.has_perm('mozreview.verify_diffset'):
> >              return PERMISSION_DENIED
> >  
> > +        privileged_user = request.user
> 
> This isn't necessarily the "privileged" user: it is the "request user."
> Please change the variable names.

It *is* because of the `has_perm(...)` check directly above it. I've added
a comment explaining the "privileged" naming. I think calling this the request_user could confuse things if you're deep in the resources code trying to reason about what permissions the request has, or what user things should be done as.

> @@ +376,5 @@
> >              update_diffset_history(squashed_rr, diffset)
> >              diffset.save()
> >  
> > +            DiffSetVerification(diffset=diffset).save(
> > +                authorized_user=privileged_user, force_insert=True)
> 
> What does force_insert do?

I've added a comment to explain the use here.

Comment 30

[Steven MacLeod [:smacleod]]

Attachment: Part 11: mozreview: cleanup extra_data key use

We were using string literals all over mozreview to reference extra_data
keys, making it harder to be confident about where a particular key was
in use. All of the important extra_data key usage has been updated to
use constants defined in extra_data.py

Comment 31

[Steven MacLeod [:smacleod]]

Attachment: Part 12: mozreview: print CommitData during tests

We will need to print the data from our CommitData model for review
requests in order to test any use of it. We now print this data when
dumping a review request.

Comment 32

[Steven MacLeod [:smacleod]]

Attachment: Part 13: mozreview: move on_draft_pre_delete into MozReview from rbbz

This commit mostly just moves code from rbbz into MozReview, centered
around what was needed to move on_draft_pre_delete().

A few of the functions relating to extra data, and accessing it, were
changed from the implementations present in rbbz to those in
mozreview.extra_data, but functionality should remain the same.

Comment 33

[Steven MacLeod [:smacleod]]

Attachment: Part 14: mozreview: copy commit extra_data to draft_extra_data on draft creation

In order to mimic the semantics of the built-in extra data for
ReviewRequest and ReviewRequestDraft we need to copy
CommitData.extra_data to CommitData.draft_extra_data when a draft is
created.

Handling clearing the data isn't necessary as we will always gate access
to draft_extra_data by checking if the draft exists first. Any stale
data will be overwritten when a draft is next created for the
ReviewRequest.

Comment 34

[Steven MacLeod [:smacleod]]

Attachment: Part 14: mozreview: copy commit extra_data to draft_extra_data on draft creation

In order to mimic the semantics of the built-in extra data for
ReviewRequest and ReviewRequestDraft we need to copy
CommitData.extra_data to CommitData.draft_extra_data when a draft is
created.

Handling clearing the data isn't necessary as we will always gate access
to draft_extra_data by checking if the draft exists first. Any stale
data will be overwritten when a draft is next created for the
ReviewRequest.

Comment 35

[Gregory Szorc [:gps]]

Comment on attachment 8715030 [details] [diff] [review]
Part 12: mozreview: print CommitData during tests

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

::: pylib/mozreview/mozreview/resources/commit_data.py
@@ +42,5 @@
> +        if not rr.is_accessible_by(request.user):
> +            return self.get_no_access_error(request, *args, **kwargs)
> +
> +        rr_draft = rr.get_draft(user=request.user)
> +        data, created = CommitData.objects.get_or_create(review_request=rr)

Why are you using get_or_create() here? Shouldn't we return a 404 or an empty object if the commit data doesn't exist? I just worry about side-effects of creating the CommitData on GET.

Comment 36

[Steven MacLeod [:smacleod]]

(In reply to Gregory Szorc [:gps] from comment #35)
> Comment on attachment 8715030 [details] [diff] [review]
> Part 12: mozreview: print CommitData during tests
> 
> Review of attachment 8715030 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: pylib/mozreview/mozreview/resources/commit_data.py
> @@ +42,5 @@
> > +        if not rr.is_accessible_by(request.user):
> > +            return self.get_no_access_error(request, *args, **kwargs)
> > +
> > +        rr_draft = rr.get_draft(user=request.user)
> > +        data, created = CommitData.objects.get_or_create(review_request=rr)
> 
> Why are you using get_or_create() here? Shouldn't we return a 404 or an
> empty object if the commit data doesn't exist? I just worry about
> side-effects of creating the CommitData on GET.

If a review request ever exists, we should be able to request CommitData for it. Rather than create it somewhere listening to creation signals for review requests etc. I'm just going to get_or_create() it anywhere I want to access it. When the review request itself doesn't exist here we 404. Make sense?

Comment 37

[Gregory Szorc [:gps]]

Yes.

Comment 38

[Mauro Doglio [:mdoglio]]

Comment on attachment 8714903 [details] [diff] [review]
Part 11: mozreview: cleanup extra_data key use

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

::: pylib/mozreview/mozreview/extra_data.py
@@ +10,5 @@
>  from reviewboard.reviews.models import ReviewRequest
>  
>  MOZREVIEW_KEY = 'p2rb'
>  
> +AUTOLAND_REQUEST_KEY = MOZREVIEW_KEY + '.'

this key is missing something

Comment 39

[Mauro Doglio [:mdoglio]]

Comment on attachment 8715030 [details] [diff] [review]
Part 12: mozreview: print CommitData during tests

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

::: pylib/mozreview/mozreview/resources/commit_data.py
@@ +20,5 @@
> +)
> +
> +
> +class CommitDataResource(WebAPIResource):
> +    """Provides read-only access to extra_date.

typo

Comment 40

[Steven MacLeod [:smacleod]]

Attachment: Part 15: mozreview: copy draft_extra_data to extra_data for drafted CommitData

We'll need a similar mechanism for having certain extra_data keys be moved from
draft_extra_data to extra_data as we used for the built-in extra_data. This is
particularly important because using a ReviewRequestField backed by our
CommitData won't take care of this for us - we'll need to do this for any key
which supports a draft value as well as the published value.

Comment 41

[Steven MacLeod [:smacleod]]

Attachment: Part 16: mozreview: move on_review_request_reopened into MozReview from rbbz

This commit mostly just moves code from rbbz into MozReview, centered
around what was needed to move on_review_request_reopened().

A few of the functions relating to extra data, and accessing it, were
changed from the implementations present in rbbz to those in
mozreview.extra_data, but functionality should remain the same.

Comment 42

[Mauro Doglio [:mdoglio]]

Attachment: quickfix_extra_data_migration.py

Comment 43

[Steven MacLeod [:smacleod]]

Attachment: Part 17: mozreview: move review_request_closed signal handlers into MozReview from rbbz

Comment 44

[Steven MacLeod [:smacleod]]

Attachment: Part 18: mozreview: remove direct extra_data key usage from rbbz

To make moving the extra_data storage location easier we remove the
final direct usage of extra_data KEY constants from rbbz. We should
now be able to completely switch the storage location of the extra data
without touching rbbz.

Comment 45

[Steven MacLeod [:smacleod]]

Attachment: Part 19: mozreview: move IDENTIFIER_KEY to CommitData from extra_data

Comment 46

[Gregory Szorc [:gps]]

Comment on attachment 8715829 [details] [diff] [review]
Part 15: mozreview: copy draft_extra_data to extra_data for drafted CommitData

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

::: pylib/mozreview/mozreview/extra_data.py
@@ +30,5 @@
>  )
>  
> +# CommitData fields which should be automatically copied from
> +# draft_extra_data to extra_data when a review request is published.
> +DRAFTED_COMMIT_DATA_KEYS = (

Feels weird that this is empty. I guess you'll address this in a subsequent patch.

Comment 47

[Steven MacLeod [:smacleod]]

Attachment: Part 20: mozreview: move FIRST_PUBLIC_ANCESTOR_KEY to CommitData from extra_data

Comment 48

[Gregory Szorc [:gps]]

Comment on attachment 8715944 [details] [diff] [review]
Part 20: mozreview: move FIRST_PUBLIC_ANCESTOR_KEY to CommitData from extra_data

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

Oh no - you are going to create rebase conflicts for all my patches.

Comment 49

[Steven MacLeod [:smacleod]]

Attachment: Part 21: mozreview: move COMMIT_ID_KEY to CommitData from extra_data

Since COMMIT_ID_KEY was the last DRAFTED_EXTRA_DATA_KEY, remove the code
for copying the drafted keys for the built-in extra data. Additionally,
update some of the extra_data.py helper methods to take in a
commit_data object for reuse.

Comment 50

[Steven MacLeod [:smacleod]]

Comment on attachment 8715857 [details] [diff] [review]
quickfix_extra_data_migration.py

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

I don't think we should stick this file in the top level of the mozreview extension directory - this is a one of migration that we'll never really think about once it's in production. Maybe it should go in the migrations directory? or somewhere else?

Also, it would be good to give it either comments or a more descriptive name with the bug number maybe to make tracking down why we did this easier (I guess it's pretty obvious from the code, but who knows if CommitData will even exist in the future.)

::: pylib/mozreview/mozreview/quickfix_extra_data_migration.py
@@ +14,5 @@
> +
> +    CommitData.objects.get_or_create(
> +        review_request=review_request,
> +        defaults={
> +            'extra_data': json.dumps(review_request.extra_data),

Does it work properly to dump the string here? usually the class expects a dictionary which will then be serialized to json itself. Are we going to end up with the wrong thing stored? I think instead of doing a json.dumps, maybe we should do a copy.deepcopy()?

Comment 51

[Steven MacLeod [:smacleod]]

Attachment: Part 22: mozreview: move BASE_COMMIT_KEY to CommitData from extra_data

Comment 52

[Steven MacLeod [:smacleod]]

Attachment: Part 23: mozreview: move SQUASHED_KEY to CommitData from extra_data

Comment 53

[Steven MacLeod [:smacleod]]

Attachment: Part 24: mozreview: move COMMITS_KEY to CommitData from extra_data

Comment 54

[Steven MacLeod [:smacleod]]

Attachment: Part 25: mozreview: move DISCARD_ON_PUBLISH_KEY and UNPUBLISHED_KEY to CommitData from extra_data

Comment 55

[Steven MacLeod [:smacleod]]

Attachment: Part 26: mozreview: use standard is_pushed function in isPush template filter

Comment 56

[Steven MacLeod [:smacleod]]

Attachment: Part 27: mozreview: move MOZREVIEW_KEY to CommitData from extra_data

MOZREVIEW_KEY was the final piece of data that needed to be moved into
the new CommitData model. Now all of the data backing a pushed review
request can only be modified by the hg server.

Comment 57

[Steven MacLeod [:smacleod]]

Attachment: Part01-27 Bundle

These patches should take care of moving all the data we need to and updating the code to access the new location. All that is left is to finalize the migration strategy and schedule some downtime to preform the deployment.

That being said, I would love to have some more testing of the new code (especially front-end stuff). It would be awesome if you guys could pull down this bundle and do a little manual testing as well.

Comment 58

[Mark Côté [:mcote]]

Hm interesting, that cleared my NI.

I'll do some testing tomorrow morning.  Resetting my NI for now.

Comment 59

[Steven MacLeod [:smacleod]]

Attachment: Part01-27 Bundle v2

Rebased the bundle and dealt with the conflicts. The tests *should* pass, but I haven't run them because I'm going to sleep.

Comment 60

[Mauro Doglio [:mdoglio]]

Attachment: mozreview: add migration script for review request extra_data

Comment 61

[Steven MacLeod [:smacleod]]

Attachment: Part01-27 Bundle v3

Just updating the one test I introduced to have "stopped 9 containers" instead of "stopped 10 containers" - the tests should pass with this bundle.

Comment 62

[Mauro Doglio [:mdoglio]]

Comment on attachment 8715857 [details] [diff] [review]
quickfix_extra_data_migration.py

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

::: pylib/mozreview/mozreview/quickfix_extra_data_migration.py
@@ +14,5 @@
> +
> +    CommitData.objects.get_or_create(
> +        review_request=review_request,
> +        defaults={
> +            'extra_data': json.dumps(review_request.extra_data),

using copy.deepcopy() raises a `TypeError: expected string or buffer` so I figured it was expecting a json string. Looking at the values stored in the mozreview_commitdata table it seems correct.

Comment 63

[Steven MacLeod [:smacleod]]

Comment on attachment 8716295 [details] [diff] [review]
mozreview: add migration script for review request extra_data

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

The one thing this is missing is directions on how to properly run it in production etc. other than that LGTM

Comment 64

[Mauro Doglio [:mdoglio]]

Comment on attachment 8715911 [details] [diff] [review]
Part 19: mozreview: move IDENTIFIER_KEY to CommitData from extra_data

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

Feel free to drop my comment, we can eventually change this later.

::: pylib/mozreview/mozreview/extra_data.py
@@ +52,5 @@
> +    """
> +    is_draft = isinstance(review_request_details, ReviewRequestDraft)
> +
> +    if commit_data is None and is_draft:
> +        commit_data = CommitData.objects.get_or_create(

It feels strange that we use get_or_create here. I thought CommitData objects were create either by a migration or on review request creation. It's kind of unexpected for example that calling `is_pushed` may create a row in the database.

Comment 65

[Steven MacLeod [:smacleod]]

https://hg.mozilla.org/hgcustom/version-control-tools/pushloghtml?changeset=d24bbd4c7303

Comment 66

[Steven MacLeod [:smacleod]]

Landed :mdog's migration script:
https://hg.mozilla.org/hgcustom/version-control-tools/rev/7a58b51b386c

Comment 67

[Steven MacLeod [:smacleod]]

This has now been deployed to production and looks to be working.

Comment 68

[Dan Minor [:dminor]]

I didn't get to running the tests until after the changes were landed. These changes may have regressed Autoland integration, I'll investigate further today.