433776 - Can't add additional extension author

Status: VERIFIED FIXED

(addons.mozilla.org Graveyard :: Developer Pages, defect)

Description

[Justin Dolske [:Dolske]]

If I go to my Developer Tools --> Edit Addon --> Add Author, entering an email address and submitting just results in "There are errors in this form. Please correct them and resubmit." being printed on the page.

Comment 1

[Fred Wenzel [:wenzel]]

That sounds like another "hidden form element" bug. Meh! Thanks for filing this, Justin.

Chris, do you mind taking a look at it?

Comment 2

[Chris Pollett]

Attachment: Adds sessionCheck hidden variable to AJAX form in question

The above patch should fix the problem. Basically, since this form was sent via Ajax it wasn't noticed when the initial checkCSRF stuff was done. Adding the sessionCheck variable solves the problem

Comment 3

[Fred Wenzel [:wenzel]]

Comment on attachment 321012 [details] [diff] [review]
Adds sessionCheck hidden variable to AJAX form in question

Not sure what exactly is causing the problem but when I click the button, the forum just submits and says it saved the changes to the add-on instead of actually adding an author.

On Safari, it just sits there, grays out the email field, and does not react further. (But it doesn't submit the form either).

The reason I am r-ing though is that I noticed when you look at the HTML code of the developer edit page, two hidden elements are in there, both of which have the id "sessionCheck". ids have to be unique.

Did this patch work for you as expected?

Comment 4

[Justin Scott [:fligtar]]

The add author form only checks for the existence of an e-mail address in the db, so you can just exclude it from CSRF checking.

Comment 5

[Chris Pollett]

Attachment: fixes id issue and other stuff as described below

Good catch on the multiple id thing on the last patch. The current patch works, tested on test database. Besides the CSRF problem it fixes two other things.
One is the JS code as it was would die under IE6 because IE 6 does not like getDocumentById where the variable and id are the same. The second issue is that the edit.thtml code has a form tag within another form tag. This will validate on the w3c validator but is not valid html according to the spec. Fligtar's point that you don't need to do CSRF checking is valid. If you want I could switch the request to be get instead in which case checking won't be done.
In any case, doing the checking doesn't hurt anything.

Comment 6

[Chris Pollett]

On an aside note the removeAuthor code gives me JS errors in the scriptaculous code

Comment 7

[Justin Scott [:fligtar]]

The second form is there so that pressing enter from the author email box will add the author. (I can't find the bug for it)

comment #6 is bug 433523

Comment 8

[Chris Pollett]

Attachment: alternate patch. No change to edit.thtml, use get rather than post

Okay. Attached is a patch which follows fligtar's suggestion of using get.
It addresses the IE6 issue mentioned above as well but leaves the nested form for another day.

Comment 9

[Justin Scott [:fligtar]]

Comment on attachment 321132 [details] [diff] [review]
alternate patch. No change to edit.thtml, use get rather than post

Works for me.

Comment 10

[Chris Pollett]

Checked in the Fligtar variant.
r13209.

Comment 11

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using https://addons.mozilla.org/en-US/developers/addon/edit/7431/authors#author-table

Comment 12

[Basil Hashem [:baz]]

This is not working for me on production AMO. I can't add additional authors - the setting is not sticking. Targeting 4.0.2

Comment 13

[Justin Scott [:fligtar]]

That is bug 451706.

Comment 14

[Stephen Donner [:stephend] Not actively reading bugmail]

Re-verifying, using https://addons.mozilla.org/en-US/developers/addon/edit/7431/authors.