Closed Bug 927497 Opened 8 years ago Closed 8 years ago

'Add me to CC list' feature of bug creation screen gives token error

Categories

(Bugzilla :: WebService, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 5.0

People

(Reporter: gerv, Assigned: dkl)

References

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

Steps to reproduce:

* Visit https://landfill.bugzilla.org/bugzilla-tip/enter_bug.cgi?product=FoodReplicator
* Enter "Foo bar baz bug"
* Click "Add me to CC list" on any of the potential duplicates which come up

Expected: added to CC list of bug

Actual: "You submitted changes to process_bug.cgi with an invalid token, which may indicate that someone tried to abuse you, for instance by making you click on a URL which redirected you here without your consent.

Are you sure you want to commit these changes?"

This problem is also visible on BMO.

Gerv
I can only reproduce in trunk. 4.4.1 and older are all fine.
Keywords: regression
Target Milestone: --- → Bugzilla 5.0
Version: unspecified → 4.5
I found the problem. Bug.possible_updates no longer returns update_token, which explains the error message (missing token).
Assignee: create-and-change → webservice
Component: Creating/Changing Bugs → WebService
This is a regression due to bug 569177. update_token has been moved out of _bug_to_hash(), which was called by possible_duplicates(), and moved into get(), which makes it unaccessible to possible_duplicates(). dkl: why this move?
Assignee: webservice → dkl
Status: NEW → ASSIGNED
Depends on: 569177
(In reply to Frédéric Buclin from comment #3)
> dkl: why this move?

as the token is different each time, you can't use it when calculating the etag of a bug.
possible_duplicates() needs to inject update tokens into the response.
Working on this now.

dkl
Attached patch 927497_1.patch (obsolete) — Splinter Review
Attachment #818689 - Flags: review?(glob)
Comment on attachment 818689 [details] [diff] [review]
927497_1.patch

>=== modified file 'js/bug.js'

>-                include_fields : [ "id", "summary", "status", "resolution",
>-                                   "update_token" ]
>+                include_fields : [ "id", "summary", "status",
>+                                   "resolution", "last_change_time" ]

Are you saying that in order to get a token, you have to pass last_change_time instead of update_token? This is not documented.



>=== modified file 'process_bug.cgi'

>-my $delta_ts = $cgi->param('delta_ts') || '';
>+my $delta_ts = $cgi->param('delta_ts') || $first_bug->delta_ts;
> 
> if ($delta_ts) {

Now that $delta_ts is always set, this makes the |if ($delta_ts)| check useless. We will always enter this block.
Comment on attachment 818689 [details] [diff] [review]
927497_1.patch

Going to revise the way I am doing this as discussed with LpSolit on IRC. We should not require for clients to include 'last_change_time' and not 'update_token' for the token to work properly. I will change it to automatically include last_change_time if the user wants update_token and then remove before returning.

Also need to figure out how to do include delta_ts as recent changes in process_bug.cgi have also broken the "Add to cc list" function as we now required delta_ts to be passed as a cgi param. Which previously the URL generated by possible duplicates did not include it.

dkl
Attachment #818689 - Flags: review?(glob)
Attached patch 927497_2.patch (obsolete) — Splinter Review
Attachment #818689 - Attachment is obsolete: true
Attachment #819225 - Flags: review?(LpSolit)
Comment on attachment 819225 [details] [diff] [review]
927497_2.patch

>=== modified file 'js/bug.js'

>                 include_fields : [ "id", "summary", "status", "resolution",
>+                                   "last_change_time", "update_token" ]

In comment 8, you said we didn't want the caller to be forced to pass "last_change_time", but it's still listed in include_fields... Is that intentional?
(In reply to Frédéric Buclin from comment #10)
> In comment 8, you said we didn't want the caller to be forced to pass
> "last_change_time", but it's still listed in include_fields... Is that
> intentional?

It is. Until we update process_bug.cgi to no longer need cgi->param('delta_ts') to validate tokens
then I also need to pass in delta_ts as part of the URL when adding a user to the cc list. So 
I need to request that last_change_time be include in the fields returned. 

dkl
Depends on: 930013
Attached patch 927497_3.patchSplinter Review
When the change in 930013 lands it will no longer be necessary for the possible duplicates code to pass the delta_ts value when adding to the cc list. So I have simplified the patch for this bug that requires the patch in 930013 to work.

dkl
Attachment #819225 - Attachment is obsolete: true
Attachment #819225 - Flags: review?(LpSolit)
Attachment #821290 - Flags: review?(LpSolit)
Comment on attachment 821290 [details] [diff] [review]
927497_3.patch

r=LpSolit
Attachment #821290 - Flags: review?(LpSolit) → review+
Flags: approval?
Flags: approval? → approval+
Committing to: bzr+ssh://dlawrence%40mozilla.com@bzr.mozilla.org/bugzilla/trunk
modified Bugzilla/WebService/Bug.pm
Committed revision 8793.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.