All the diffset in a review request should be verified

RESOLVED FIXED

Status

MozReview
General
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mdoglio, Assigned: smacleod)

Tracking

(Blocks: 1 bug)

Details

Attachments

(29 attachments, 10 obsolete attachments)

1.53 KB, patch
smacleod
: review+
gps
: review+
Details | Diff | Splinter Review
5.90 KB, patch
smacleod
: review+
gps
: review+
Details | Diff | Splinter Review
9.74 KB, patch
smacleod
: review+
gps
: review+
Details | Diff | Splinter Review
1.81 KB, patch
dminor
: review+
gps
: review+
Details | Diff | Splinter Review
9.45 KB, patch
mdoglio
: review+
gps
: review+
Details | Diff | Splinter Review
36.03 KB, patch
mdoglio
: review+
mcote
: review+
Details | Diff | Splinter Review
3.12 KB, patch
smacleod
: review+
gps
: review+
Details | Diff | Splinter Review
2.48 KB, patch
mdoglio
: review+
Details | Diff | Splinter Review
7.57 KB, patch
gps
: review+
Details | Diff | Splinter Review
13.10 KB, patch
gps
: review+
Details | Diff | Splinter Review
28.70 KB, patch
mcote
: review+
mdoglio
: review+
gps
: review+
Details | Diff | Splinter Review
43.87 KB, patch
gps
: review+
mdoglio
: review+
Details | Diff | Splinter Review
10.58 KB, patch
mdoglio
: review+
gps
: review+
Details | Diff | Splinter Review
4.15 KB, patch
mdoglio
: review+
mcote
: review+
gps
: review+
Details | Diff | Splinter Review
3.18 KB, patch
mdoglio
: review+
gps
: review+
Details | Diff | Splinter Review
9.65 KB, patch
mcote
: review+
Details | Diff | Splinter Review
11.04 KB, patch
mcote
: review+
Details | Diff | Splinter Review
3.89 KB, patch
mcote
: review+
Details | Diff | Splinter Review
56.95 KB, patch
mdoglio
: review+
gps
: review+
Details | Diff | Splinter Review
52.63 KB, patch
gps
: review+
Details | Diff | Splinter Review
43.40 KB, patch
gps
: review+
Details | Diff | Splinter Review
29.77 KB, patch
gps
: review+
Details | Diff | Splinter Review
54.05 KB, patch
gps
: review+
Details | Diff | Splinter Review
49.80 KB, patch
gps
: review+
Details | Diff | Splinter Review
42.77 KB, patch
gps
: review+
Details | Diff | Splinter Review
2.64 KB, patch
gps
: review+
Details | Diff | Splinter Review
48.09 KB, patch
mcote
: review+
mdoglio
: review+
Details | Diff | Splinter Review
1.49 KB, patch
smacleod
: review+
Details | Diff | Splinter Review
41.54 KB, application/octet-stream
Details
(Reporter)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
Created attachment 8713992 [details] [diff] [review]
Part 01: mozreview: switch import style
Attachment #8713992 - Flags: review+
(Assignee)

Comment 2

2 years ago
Created attachment 8713993 [details] [diff] [review]
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.
Attachment #8713993 - Flags: review?(dminor)
(Assignee)

Comment 3

2 years ago
Created attachment 8713994 [details] [diff] [review]
Part 03: mozreview: grant the mozreview user the verify_diffset permission
Attachment #8713994 - Flags: review?(dminor)
(Assignee)

Comment 4

2 years ago
Created attachment 8713995 [details] [diff] [review]
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.
Attachment #8713995 - Flags: review+
(Assignee)

Comment 5

2 years ago
Created attachment 8713996 [details] [diff] [review]
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.
Attachment #8713996 - Flags: review+
(Assignee)

Comment 6

2 years ago
Created attachment 8714041 [details] [diff] [review]
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.
Attachment #8713993 - Attachment is obsolete: true
Attachment #8713993 - Flags: review?(dminor)
Attachment #8714041 - Flags: review?(dminor)
(Assignee)

Comment 7

2 years ago
Created attachment 8714042 [details] [diff] [review]
Part 03: mozreview: grant the mozreview user the verify_diffset permission
Attachment #8713994 - Attachment is obsolete: true
Attachment #8713994 - Flags: review?(dminor)
Attachment #8714042 - Flags: review?(dminor)
(Assignee)

Comment 8

2 years ago
Created attachment 8714043 [details] [diff] [review]
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.
Attachment #8714043 - Flags: review?(mdoglio)
Attachment #8714043 - Flags: review?(gps)
(Assignee)

Comment 9

2 years ago
Created attachment 8714044 [details] [diff] [review]
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.
Attachment #8714044 - Flags: review?(mdoglio)
(Assignee)

Updated

2 years ago
Attachment #8714044 - Attachment is patch: true
(Assignee)

Comment 10

2 years ago
Created attachment 8714045 [details] [diff] [review]
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.
Attachment #8714045 - Flags: review?(mdoglio)
Attachment #8714045 - Flags: review?(mcote)
(Assignee)

Updated

2 years ago
Status: NEW → ASSIGNED
(Reporter)

Comment 11

2 years ago
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
Attachment #8714043 - Flags: review?(mdoglio) → review+
(Reporter)

Updated

2 years ago
Attachment #8714044 - Flags: review?(mdoglio) → review+
(Reporter)

Updated

2 years ago
Attachment #8714045 - Flags: review?(mdoglio) → review+
(Assignee)

Updated

2 years ago
Blocks: 1244536
(Assignee)

Comment 12

2 years ago
Created attachment 8714095 [details] [diff] [review]
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.
Attachment #8714043 - Attachment is obsolete: true
Attachment #8714043 - Flags: review?(gps)
Attachment #8714095 - Flags: review?(gps)
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?
Attachment #8714045 - Flags: review?(mcote) → review+
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).
(Assignee)

Comment 15

2 years ago
(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.
(Assignee)

Comment 16

2 years ago
(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)
(Assignee)

Comment 17

2 years ago
Created attachment 8714119 [details] [diff] [review]
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.
Attachment #8714119 - Flags: review?(mdoglio)
Attachment #8714119 - Flags: review?(gps)
(Assignee)

Comment 18

2 years ago
: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

2 years ago
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
Attachment #8714041 - Flags: review?(dminor) → review+

Updated

2 years ago
Attachment #8714042 - Flags: review?(dminor) → review+
(Assignee)

Comment 20

2 years ago
(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.
(Assignee)

Comment 21

2 years ago
Created attachment 8714324 [details] [diff] [review]
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.
Attachment #8714041 - Attachment is obsolete: true
Attachment #8714324 - Flags: review+

Comment 22

2 years ago
(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.
(Reporter)

Updated

2 years ago
Attachment #8714119 - Flags: review?(mdoglio) → review+
(Assignee)

Comment 23

2 years ago
Created attachment 8714335 [details] [diff] [review]
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.
Attachment #8714335 - Flags: review?(mdoglio)
(Assignee)

Updated

2 years ago
Attachment #8714335 - Attachment is patch: true
(Reporter)

Comment 24

2 years ago
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
Attachment #8714335 - Flags: review?(mdoglio) → review+
(Assignee)

Comment 25

2 years ago
Created attachment 8714395 [details] [diff] [review]
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.
Attachment #8714119 - Attachment is obsolete: true
Attachment #8714119 - Flags: review?(gps)
Attachment #8714395 - Flags: review?(gps)

Updated

2 years ago
Attachment #8713992 - Flags: review+

Updated

2 years ago
Attachment #8713995 - Flags: review+

Updated

2 years ago
Attachment #8714324 - Flags: review+

Updated

2 years ago
Attachment #8714042 - Flags: review+

Updated

2 years ago
Attachment #8713996 - Flags: review+
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?
Attachment #8714095 - Flags: review?(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.
Attachment #8714044 - Flags: review+
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.
Attachment #8714395 - Flags: review?(gps) → review+
(Assignee)

Comment 29

2 years ago
Created attachment 8714833 [details] [diff] [review]
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.
Attachment #8714095 - Attachment is obsolete: true
Attachment #8714833 - Flags: review?(gps)

Updated

2 years ago
Attachment #8714833 - Flags: review?(gps) → review+
(Assignee)

Comment 30

2 years ago
Created attachment 8714903 [details] [diff] [review]
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
Attachment #8714903 - Flags: review?(mdoglio)
Attachment #8714903 - Flags: review?(mcote)
(Assignee)

Comment 31

2 years ago
Created attachment 8715030 [details] [diff] [review]
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.
Attachment #8715030 - Flags: review?(mdoglio)
Attachment #8715030 - Flags: review?(gps)
(Assignee)

Comment 32

2 years ago
Created attachment 8715044 [details] [diff] [review]
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.
Attachment #8715044 - Flags: review?(mdoglio)
(Assignee)

Comment 33

2 years ago
Created attachment 8715068 [details] [diff] [review]
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.
Attachment #8715068 - Flags: review?(mdoglio)
Attachment #8715068 - Flags: review?(mcote)
(Assignee)

Comment 34

2 years ago
Created attachment 8715077 [details] [diff] [review]
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.
Attachment #8715068 - Attachment is obsolete: true
Attachment #8715068 - Flags: review?(mdoglio)
Attachment #8715068 - Flags: review?(mcote)
Attachment #8715077 - Flags: review?(mdoglio)
Attachment #8715077 - Flags: review?(mcote)
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.
(Assignee)

Comment 36

2 years ago
(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?
Flags: needinfo?(gps)
Yes.
Flags: needinfo?(gps)

Updated

2 years ago
Attachment #8714903 - Flags: review+

Updated

2 years ago
Attachment #8715030 - Flags: review?(gps) → review+

Updated

2 years ago
Attachment #8715044 - Flags: review+

Updated

2 years ago
Attachment #8715077 - Flags: review+
(Reporter)

Comment 38

2 years ago
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
Attachment #8714903 - Flags: review?(mdoglio) → review+
(Reporter)

Comment 39

2 years ago
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
Attachment #8715030 - Flags: review?(mdoglio) → review+
(Reporter)

Updated

2 years ago
Attachment #8715044 - Flags: review?(mdoglio) → review+
(Reporter)

Updated

2 years ago
Attachment #8715077 - Flags: review?(mdoglio) → review+

Updated

2 years ago
Attachment #8714903 - Flags: review?(mcote) → review+

Updated

2 years ago
Attachment #8715077 - Flags: review?(mcote) → review+
(Assignee)

Comment 40

2 years ago
Created attachment 8715829 [details] [diff] [review]
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.
Attachment #8715829 - Flags: review?(mdoglio)
Attachment #8715829 - Flags: review?(gps)
(Assignee)

Comment 41

2 years ago
Created attachment 8715856 [details] [diff] [review]
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.
Attachment #8715856 - Flags: review?(mcote)
(Reporter)

Comment 42

2 years ago
Created attachment 8715857 [details] [diff] [review]
quickfix_extra_data_migration.py
Attachment #8715857 - Flags: review?(smacleod)
(Assignee)

Updated

2 years ago
Attachment #8715856 - Attachment is patch: true
(Assignee)

Comment 43

2 years ago
Created attachment 8715871 [details] [diff] [review]
Part 17: mozreview: move review_request_closed signal handlers into MozReview from rbbz
Attachment #8715871 - Flags: review?(mcote)
(Assignee)

Comment 44

2 years ago
Created attachment 8715879 [details] [diff] [review]
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.
Attachment #8715879 - Flags: review?(mcote)

Updated

2 years ago
Attachment #8715856 - Flags: review?(mcote) → review+
(Assignee)

Comment 45

2 years ago
Created attachment 8715911 [details] [diff] [review]
Part 19: mozreview: move IDENTIFIER_KEY to CommitData from extra_data
Attachment #8715911 - Flags: review?(mdoglio)
Attachment #8715911 - Flags: review?(gps)

Updated

2 years ago
Attachment #8715871 - Flags: review?(mcote) → review+

Updated

2 years ago
Attachment #8715879 - Flags: review?(mcote) → review+
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.
Attachment #8715829 - Flags: review?(gps) → review+

Updated

2 years ago
Attachment #8715911 - Flags: review?(gps) → review+
(Assignee)

Comment 47

2 years ago
Created attachment 8715944 [details] [diff] [review]
Part 20: mozreview: move FIRST_PUBLIC_ANCESTOR_KEY to CommitData from extra_data
Attachment #8715944 - Flags: review?(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.
Attachment #8715944 - Flags: review?(gps) → review+
(Assignee)

Comment 49

2 years ago
Created attachment 8716000 [details] [diff] [review]
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.
Attachment #8716000 - Flags: review?(gps)

Updated

2 years ago
Attachment #8716000 - Flags: review?(gps) → review+
(Assignee)

Comment 50

2 years ago
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()?
Attachment #8715857 - Flags: review?(smacleod) → feedback+
(Assignee)

Comment 51

2 years ago
Created attachment 8716043 [details] [diff] [review]
Part 22: mozreview: move BASE_COMMIT_KEY to CommitData from extra_data
Attachment #8716043 - Flags: review?(gps)
(Assignee)

Comment 52

2 years ago
Created attachment 8716071 [details] [diff] [review]
Part 23: mozreview: move SQUASHED_KEY to CommitData from extra_data
Attachment #8716071 - Flags: review?(gps)

Updated

2 years ago
Attachment #8716043 - Flags: review?(gps) → review+

Updated

2 years ago
Attachment #8716071 - Flags: review?(gps) → review+
(Assignee)

Comment 53

2 years ago
Created attachment 8716099 [details] [diff] [review]
Part 24: mozreview: move COMMITS_KEY to CommitData from extra_data
Attachment #8716099 - Flags: review?(gps)
(Assignee)

Comment 54

2 years ago
Created attachment 8716123 [details] [diff] [review]
Part 25: mozreview: move DISCARD_ON_PUBLISH_KEY and UNPUBLISHED_KEY to CommitData from extra_data
Attachment #8716123 - Flags: review?(gps)
(Assignee)

Comment 55

2 years ago
Created attachment 8716125 [details] [diff] [review]
Part 26: mozreview: use standard is_pushed function in isPush template filter
Attachment #8716125 - Flags: review?(gps)

Updated

2 years ago
Attachment #8716123 - Flags: review?(gps) → review+

Updated

2 years ago
Attachment #8716099 - Flags: review?(gps) → review+

Updated

2 years ago
Attachment #8716125 - Flags: review?(gps) → review+
(Assignee)

Comment 56

2 years ago
Created attachment 8716139 [details] [diff] [review]
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.
Attachment #8716139 - Flags: review?(mdoglio)
Attachment #8716139 - Flags: review?(mcote)
(Assignee)

Comment 57

2 years ago
Created attachment 8716140 [details]
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.
Flags: needinfo?(mdoglio)
Flags: needinfo?(mcote)
Flags: needinfo?(dminor)

Updated

2 years ago
Flags: needinfo?(mcote)
Attachment #8716139 - Flags: review?(mcote) → review+
Hm interesting, that cleared my NI.

I'll do some testing tomorrow morning.  Resetting my NI for now.
Flags: needinfo?(mcote)
(Assignee)

Comment 59

2 years ago
Created attachment 8716178 [details]
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.
Attachment #8716140 - Attachment is obsolete: true
(Reporter)

Comment 60

2 years ago
Created attachment 8716295 [details] [diff] [review]
mozreview: add migration script for review request extra_data
Attachment #8715857 - Attachment is obsolete: true
Attachment #8716295 - Flags: review?(smacleod)
(Assignee)

Comment 61

2 years ago
Created attachment 8716298 [details]
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.
Attachment #8716178 - Attachment is obsolete: true
(Reporter)

Comment 62

2 years ago
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.
(Assignee)

Comment 63

2 years ago
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
Attachment #8716295 - Flags: review?(smacleod) → review+
(Reporter)

Updated

2 years ago
Attachment #8715829 - Flags: review?(mdoglio) → review+
(Reporter)

Comment 64

2 years ago
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.
Attachment #8715911 - Flags: review?(mdoglio) → review+
(Reporter)

Updated

2 years ago
Attachment #8716139 - Flags: review?(mdoglio) → review+
(Assignee)

Comment 65

2 years ago
https://hg.mozilla.org/hgcustom/version-control-tools/pushloghtml?changeset=d24bbd4c7303
(Assignee)

Comment 66

2 years ago
Landed :mdog's migration script:
https://hg.mozilla.org/hgcustom/version-control-tools/rev/7a58b51b386c
(Assignee)

Comment 67

2 years ago
This has now been deployed to production and looks to be working.
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED

Updated

2 years ago
Blocks: 1244835

Updated

2 years ago
Group: bugzilla-security, webtools-security
Flags: needinfo?(mcote)

Updated

2 years ago
Blocks: 1246308
(Reporter)

Updated

2 years ago
Flags: needinfo?(mdoglio)

Comment 68

2 years ago
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.
Flags: needinfo?(dminor)
(Assignee)

Updated

2 years ago
No longer blocks: 1244536
Product: Developer Services → MozReview
You need to log in before you can comment on or make changes to this bug.