*** Original post on bio 1883 at 2013-02-27 22:14:00 UTC *** A server that I have an account on doesn't have a web-interface to change the account password but only supports "in-band password changes", which seems to mean that the client handles all of it and only sends a request to change the password to the server. It would be nice to have this from my point of view (which means: I'm stuck with my old password until we either support it or I abuse another client to change it). The XEP in questions seems to be 0077: http://xmpp.org/extensions/xep-0077.html

*** Original post on bio 1883 at 2013-02-27 22:15:44 UTC *** I forgot: additionally this XEP seems to define support for registering accounts on the server from within the client.

*** Original post on bio 1883 at 2013-02-27 23:48:33 UTC *** From http://log.bezut.info/instantbird/130227/#m206: 23:44:16 - flo-retina: Mic: any idea of how the UI could look? [...] 00:39:45 - Mic: I've no clear idea for the password change. 00:40:21 - Mic: The registration UI could be a special message (or even a new binding?) on newly created accounts: "An account with this name was not found on the server. Do you want to [create it now] or [edit the name of the account]." (<- the latter would be for the case that there's been a typo in the account name. 00:41:57 - flo-retina: that could work :) 00:41:59 - Mic: Even though it wouldn't be clear that it is possible to create account at the moment one clicks "New account" then. 00:42:15 - flo-retina: assuming the server doesn't reply the same thing if the password is wrong or if the account name doesn't exist 00:42:47 - Mic: Yes, we'd need to know if the password is wrong or the account is non-existent + that the server supports this feature.

I am working on implementing XEP-0077 as part of Tor Messenger. (The plan is to merge the changes upstream with Instantbird.) Before I submit the patches and finalize the code, one of the points for discussion is the UI for the registration dialog. Since modal dialogs are a big no, I was wondering what kind of a UI for the registration form would be desired? Should we extend the account wizard dialog itself (if a user has requested a new XMPP account) and not create a new dialog? (To see the account registration form/dialog, start Pidgin, add an XMPP account, enter "jabber.ccc.de" as the domain and check "Create this new account on this server".)

(In reply to Sukhbir Singh from comment #4) Hi Sukhe, do you have any WIPs for this you could share?

In think he said he's clean it up before submitting, but see: https://gitweb.torproject.org/tor-messenger-build.git/tree/projects/instantbird/xmpp-inband-registration.patch

(In reply to arlolra from comment #6) > In think he said he's clean it up before submitting, but see: > https://gitweb.torproject.org/tor-messenger-build.git/tree/projects/ > instantbird/xmpp-inband-registration.patch Awesome, thanks! We'll just wait for him to upload the patch then.

Hi. I am attaching the patches for in-band XMPP registration. A few points of discussions about them, since while they work, they are far from complete. I will be happy to make changes to get them accepted into Instantbird. - This is not a complete implementation of XEP-0077. In-band works but that's not the only thing XEP-0077 talks about. - The implementation of data forms (described by XEP-0004) which in-band uses is also far from complete. The code works but it may break for cases not covered by the common servers but covered by XEP-0004. So the tl;dr is that we wrote this to work with Tor Messenger and to cover the commonly used XMPP servers that support in-band (like jabber.ccc.de, jabber.otr.im). This is not a complete implementation but I would like to finish it. (Since this is the first time I am submitting a patch to Instantbird, feedback is welcome.)

Comment on attachment 8689081 [details] [diff] [review] XMPP in-band registration support Review of attachment 8689081 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for upstreaming this! It would be nice if this could land in the 45 cycle (ie. the next TB ESR release). String freeze for that is in around three weeks. sukhe: abdelrhman knows the XMPP code well, will you be working together on this? ::: chat/protocols/xmpp/xmpp-session.jsm @@ +357,5 @@ > + let register = Stanza.iq("get", null, null, > + Stanza.node("query", Stanza.NS.register)); > + this.sendStanza(register); > + this.onXmppStanza = this.stanzaListeners.startRegister; > + return; What happens if this fails? ::: im/content/accountWizard.js @@ +115,5 @@ > + if (this.proto.id == "prpl-jabber") { > + document.getElementById("registerXMPP").hidden = false; > + } else { > + document.getElementById("registerXMPP").hidden = true; > + } Theres already a mechanism for prpl-specific code in *ProtoSpecificBox(). Can we do something similar here and/or maybe even reuse things? I can see why you wouldn't want this checkbox under "advanced options" though. @@ +422,5 @@ > if (this.alias) > acc.alias = this.alias; > //FIXME: newMailNotification > > + acc.setBool("register", document.getElementById("registerXMPP").checked); Does this needs API changes. ::: im/content/accountWizard.xul @@ +64,5 @@ > <separator/> > <vbox id="userNameBox"/> > <separator/> > <description id="duplicateAccount" hidden="true">&accountUsernameDuplicate.label;</description> > + <checkbox id="registerXMPP" label="&registerXMPP.label;" hidden="true" /> If this is added for all protocols then it should not have an XMPP-specific id etc. and the string (which might be protocol dependent) would be set via JS. ::: im/content/jar.mn @@ +61,5 @@ > * content/instantbird/viewlog.xul > content/instantbird/viewlog.js > content/instantbird/viewlog.css > +* content/instantbird/xmppRegister.xul > + content/instantbird/xmppRegister.js Nit: note indent in jar.mn files are probably tabs.

(In reply to aleth [:aleth] from comment #9) > Thanks for upstreaming this! > > It would be nice if this could land in the 45 cycle (ie. the next TB ESR > release). String freeze for that is in around three weeks. > > sukhe: abdelrhman knows the XMPP code well, will you be working together on > this? Hi, and yes, abdelrhman will be working on fixing the remaining issues since he knows the XMPP code better than I do. I am attaching the XUL and JS file which I forgot. We will now resume the discussion.

(In reply to Abdelrhman Ahmed [:abdelrhman] from comment #13) > Created attachment 8696247 [details] [diff] [review] > support XEP-0077 Could you please attach a patch that folds in Sukhbir's changes? (hg qfold can do this for you.) That would be much easier to review. We have Sukhbir's original patches here on BMO for future reference.

Comment on attachment 8696247 [details] [diff] [review] support XEP-0077 Review of attachment 8696247 [details] [diff] [review]: ----------------------------------------------------------------- ::: chat/protocols/xmpp/xmpp-session.jsm @@ +328,5 @@ > + > + Services.ww.openWindow(null, "chrome://instantbird/content/xmppRegister.xul", > + null, > + "centerscreen,chrome,modal,minimizable=no,resizable=no", > + args); prpls are not allowed to access frontend UI directly. We should discuss what to do instead. ::: im/content/jar.mn @@ +61,5 @@ > * content/instantbird/viewlog.xul > content/instantbird/viewlog.js > content/instantbird/viewlog.css > +* content/instantbird/xmppRegister.xul > + content/instantbird/xmppRegister.js Note jar.mn files use tabs for the indent.

(In reply to aleth [:aleth] from comment #16) > > + Services.ww.openWindow(null, "chrome://instantbird/content/xmppRegister.xul", > > + null, > > + "centerscreen,chrome,modal,minimizable=no,resizable=no", > > + args); > > prpls are not allowed to access frontend UI directly. We should discuss what > to do instead. The simplest way to do this is to look at the browser-request notification as an example (which is what twitter oauth uses to show its registration form) and do something similar, i.e. instead of calling Services.ww.openWindow from the prpl, you send a notification along with an object containing the data the dialog needs. The frontend listens for the notification and calls Services.ww.openWindow. Note the dialog XUL/js files should be in chat/content not im/content (or Thunderbird will never get them). It would be nicer if the dialog was be one of the account wizard steps instead, but adding support for that seems even more complicated, and this patch is already complicated enough.

Comment on attachment 8696264 [details] [diff] [review] support XEP-0077 Review of attachment 8696264 [details] [diff] [review]: ----------------------------------------------------------------- ::: chat/components/public/prplIPref.idl @@ +18,5 @@ > > readonly attribute AUTF8String name; > readonly attribute AUTF8String label; > + > + // Used to specify the preferred page in which option will be shown. For the comment: what are the allowed values? There should be a default if it's not set/empty string. @@ +19,5 @@ > readonly attribute AUTF8String name; > readonly attribute AUTF8String label; > + > + // Used to specify the preferred page in which option will be shown. > + readonly attribute AUTF8String pageId; Might as well just call this "page" ::: chat/locales/en-US/xmpp.properties @@ +14,5 @@ > connection.gettingResource=Getting resource > connection.downloadingRoster=Downloading contact list > +connection.registering=Registering new account > +connection.gettingRegistration=Getting registration form > +connection.onRegistrationSuccess=Account registered Account registered successfully @@ +35,5 @@ > connection.error.notSendingPasswordInClear=The server only supports authentication by sending the password in cleartext > connection.error.authenticationFailure=Authentication failure > connection.error.notAuthorized=Not authorized (Did you enter the wrong password?) > connection.error.failedToGetAResource=Failed to get a resource > +connection.error.noRegistrationSupport=The server does not support in-band registration This server... @@ +36,5 @@ > connection.error.authenticationFailure=Authentication failure > connection.error.notAuthorized=Not authorized (Did you enter the wrong password?) > connection.error.failedToGetAResource=Failed to get a resource > +connection.error.noRegistrationSupport=The server does not support in-band registration > +connection.error.registrationCancel=Registration canceled Registration canceled by the user @@ +38,5 @@ > connection.error.failedToGetAResource=Failed to get a resource > +connection.error.noRegistrationSupport=The server does not support in-band registration > +connection.error.registrationCancel=Registration canceled > +connection.error.registrationConflict=Username is not available > +connection.error.registrationNotAcceptable=Some required information are not provided Some required information was not provided @@ +39,5 @@ > +connection.error.noRegistrationSupport=The server does not support in-band registration > +connection.error.registrationCancel=Registration canceled > +connection.error.registrationConflict=Username is not available > +connection.error.registrationNotAcceptable=Some required information are not provided > +connection.error.unknownError=Unknown error connection.error.unknownRegistrationError=Unknown error on account registration ::: chat/protocols/xmpp/xmpp-session.jsm @@ +323,5 @@ > + fields["username"] = {required: true, value: this._jid.node}; > + fields["password"] = {required: true, value: this._password}; > + > + let args = {query: query, fields: fields, cancelled: false}; > + args.wrappedJSObject = args; Horrible hack due to not separating UI from backend properly. @@ +328,5 @@ > + > + Services.ww.openWindow(null, "chrome://instantbird/content/xmppRegister.xul", > + null, > + "centerscreen,chrome,modal,minimizable=no,resizable=no", > + args); No modal dialogs!! @@ +334,5 @@ > + let x = query.getElement(["x"]); > + if (x && x.uri == Stanza.NS.xoob) { > + // XEP-0077 (5): Redirection. > + // Host redirect users to another medium (e.g. a website) registration. > + return; What happens then? @@ +345,5 @@ > + } > + > + // XEP-0077 (3.1): Entity Registers with a Host. > + let fieldStanza; > + if (x) { If you intend to support forms, please follow http://xmpp.org/extensions/xep-0077.html#precedence (also for the NS.xoob case) Please check the namespace on x. ::: im/content/accountWizard.js @@ +148,5 @@ > defaultVal)); > } > this.userNameBoxes[0].focus(); > this.userNameProto = proto; > + this.addOptions(vbox, "accountusername"); "username" is probably enough ("account" is kind of obvious). @@ +281,2 @@ > for (let opt in this.getProtoOptions()) { > + let pageId = opt.pageId ? opt.pageId : "accountadvanced"; maybe "advancedOptions" instead of "accountadvanced"

Comment on attachment 8696264 [details] [diff] [review] support XEP-0077 Review of attachment 8696264 [details] [diff] [review]: ----------------------------------------------------------------- ::: chat/protocols/xmpp/xmpp-session.jsm @@ +301,5 @@ > + this.WARN(`Unhandled IQ ${type} stanza.`); > + }, > + startRegister: function(aStanza) { > + // Some servers do not support in-band registration. In that case, > + // complain and quit the registration process. Shouldn't you be checking http://xmpp.org/extensions/xep-0077.html#streamfeature at some earlier point? @@ +370,5 @@ > + let registerResponse = Stanza.iq("set", null, this._domain, > + Stanza.node("query", Stanza.NS.register, > + null, fieldStanza)); > + logString = "<iq .../> (Stanza containing password to register account " + > + this._jid.jid + " not logged)"; Ensure you don't send the password over an unencrypted connection if the user hasn't allowed this. see this._connectionSecurity, this._encrypted

Let's leave password change for a followup.

Comment on attachment 8697796 [details] [diff] [review] support XEP-0077 Review of attachment 8697796 [details] [diff] [review]: ----------------------------------------------------------------- Great progress! ::: chat/components/public/prplIPref.idl @@ +5,5 @@ > #include "nsISupports.idl" > #include "nsISimpleEnumerator.idl" > > /* > * This is a proxy for libpurple PurpleAccountOption Let's expand the comment a bit: "Used to specify protocol-specific account options and for the fields of a dialogRequest." @@ +18,5 @@ > > + // Used to specify type as a label only. > + const short typeLabel = 5; > + > + // Used to specify type as an image. Yes, but how? which field is the image passed in? ;) @@ +30,5 @@ > + // If it's not set, it will have default value advancedOptions. > + readonly attribute AUTF8String page; > + > + // If it's not set, it will be determined according to the type of the default > + // value Good idea to add a comment. Let's also explain what it is used for and what it can be set to. @@ +37,3 @@ > readonly attribute boolean masked; > + readonly attribute boolean required; > + readonly attribute boolean isReadonly; readonly? What's a readonly option? If the only readonly option is typeLabel, then you don't need a bool for that, it's obvious. ::: chat/components/public/prplIRequest.idl @@ +20,5 @@ > void cancelled(); > void loaded(in nsIDOMWindow aWindow, > in nsIWebProgress aWebProgress); > }; > /* This interface is for use in the dialog-request notification, to let protocol plugins open a dialog window similar to an account wizard options page. This is an unfortunate necessity for protocols like XMPP that support in-band authentication. */ @@ +29,5 @@ > + // returns an enumerator of prplIPref > + nsISimpleEnumerator getFields(); > + > + void cancelled(); > + void saved(in nsISimpleEnumerator aFields); Let's call this accepted() to match the dialog buttons. Comment: aFields is an enumerator of...? ::: chat/content/dialogRequest.js @@ +1,4 @@ > +const { interfaces: Ci, utils: Cu, classes: Cc } = Components; > + > +Cu.import("resource:///modules/imXPCOMUtils.jsm"); > +Cu.import("resource:///modules/jsProtoHelper.jsm"); Not needed? @@ +2,5 @@ > + > +Cu.import("resource:///modules/imXPCOMUtils.jsm"); > +Cu.import("resource:///modules/jsProtoHelper.jsm"); > + > +let dialog = { var @@ +5,5 @@ > + > +let dialog = { > + _fields: [], > + > + createElement:function(aType, aID, aValue) { I suspect this would be more readable if you inline this where the arguments are not needed, and make it more specific when you are setting up a particular type of row, e.g. like createTextbox and createMenulist in accountWizard.js @@ +14,5 @@ > + element.setAttribute("value", aValue); > + return element; > + }, > + > + createRow:function() { Inline this @@ +16,5 @@ > + }, > + > + createRow:function() { > + let row = document.createElement("row"); > + row.setAttribute("align", "baseline"); What does this do? @@ +33,5 @@ > + rows.appendChild(row); > + return; > + } > + > + let label = aElement.labelValue + (aElement.required ? " *" : null); This is not a great way to tell the user that the element is required. @@ +61,5 @@ > + let request = window.arguments[0]; > + request.QueryInterface(Components.interfaces.prplIRequestDialog); > + > + if (request.title) > + document.title = request.title; Where does this end up? The titlebar? @@ +64,5 @@ > + if (request.title) > + document.title = request.title; > + > + let fields = request.getFields(); > + while (fields.hasMoreElements()) { Probably will end up very similar to populateProtoSpecificBox in accountWizard.js ;) @@ +79,5 @@ > + } > + else if (field.type == Ci.prplIPref.typeImage) { > + options.imageSrc = field.getString(); > + } > + this.addRow(options); This is only called once, from here, so inline it. Then you also don't need all that options stuff. @@ +83,5 @@ > + this.addRow(options); > + } > + > + if (this.checkAcceptEnable()) > + document.documentElement.getButton("accept").click(); ? @@ +87,5 @@ > + document.documentElement.getButton("accept").click(); > + }, > + > + onSave: function() { > + let retFields = [] Hmm. @@ +111,5 @@ > + if (field.required && !document.getElementById(field.id).value) { > + isCompleted = false; > + break; > + } > + document.documentElement.getButton("accept").disabled = !isCompleted; Does window.getButton work? ::: chat/content/dialogRequest.xul @@ +7,5 @@ > +]> > + > +<dialog > + xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" > + id="registerDialog" dialogRequest @@ +8,5 @@ > + > +<dialog > + xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" > + id="registerDialog" > + title = "&brandShortName;" Nit: spaces @@ +11,5 @@ > + id="registerDialog" > + title = "&brandShortName;" > + onload="dialog.onLoad()" > + buttons="accept,cancel" > + ondialogaccept="return dialog.onSave()" dialog.onAccept() @@ +13,5 @@ > + onload="dialog.onLoad()" > + buttons="accept,cancel" > + ondialogaccept="return dialog.onSave()" > + ondialogcancel="dialog.onCancel()"> > + <script type="application/javascript" src="chrome://chat/content/dialogRequest.js" /> Does the dialog look better/more consistent if you include the CSS from accountWizard.css that styles the groupbox etc? @@ +14,5 @@ > + buttons="accept,cancel" > + ondialogaccept="return dialog.onSave()" > + ondialogcancel="dialog.onCancel()"> > + <script type="application/javascript" src="chrome://chat/content/dialogRequest.js" /> > + Needs a keyset like the one in browserrequest.xul @@ +15,5 @@ > + ondialogaccept="return dialog.onSave()" > + ondialogcancel="dialog.onCancel()"> > + <script type="application/javascript" src="chrome://chat/content/dialogRequest.js" /> > + > + <groupbox id="register-groupbox" flex="1"> dialogGroupbox @@ +18,5 @@ > + > + <groupbox id="register-groupbox" flex="1"> > + <grid flex="1"> > + <columns> > + <column flex="1" /> I think what you want here is <column id="label-column" flex="1"/> <column id="value-column" flex="1"/> @@ +20,5 @@ > + <grid flex="1"> > + <columns> > + <column flex="1" /> > + </columns> > + <rows id="register-rows" /> dialogRows ::: chat/modules/jsProtoHelper.jsm @@ +777,1 @@ > this.type = Ci.prplIPref["type" + type]; Check if this makes sense for new types that are not prefs. Do we need the type field for those at all? @@ +789,5 @@ > + > + if ("page" in aOption && aOption.page) { > + const kPages = ["username", "password", "advancedOptions"]; > + let condition = kPages.find(p => p == aOption.page); > + if (!condition) Use kPages.some() ::: chat/protocols/xmpp/xmpp-session.jsm @@ +273,5 @@ > this.onXmppStanza = this.stanzaListeners.startAuth; > this.onXmppStanza(aStanza); > }, > + onRegisterResponse: function(aStanza) { > + this.disconnect(); Not needed. onError also calls disconnect, but with the correct error message. @@ +287,5 @@ > + break; > + default: > + errorMsg = _("connection.error.unknownRegistrationError"); > + } > + this.onError(null, errorMsg); Is null really the right error code here? I'm not sure. @@ +293,5 @@ > + } > + > + let type = aStanza.attributes["type"]; > + if (type == "result") { > + this._account.reportConnecting(_("connection.onRegistrationSuccess")); This looks like it should be a disconnect message? At this point the account is not connecting so you should not call this, at best it won't work, at worst it will confuse something as account-connecting notifications are sent twice. @@ +298,5 @@ > + this._account.prefs.setBoolPref("register", false); > + this._account.connect(); > + } > + else > + this.WARN(`Unhandled IQ ${type} stanza.`); and then what happens? What state is the account and the connection in? @@ +337,5 @@ > + purplePrefs.push(new purplePref("password", {label: "password", > + default: session._password, > + masked: true, > + required: true, > + isReadonly: true})); What's the point of showing these if they can't be changed? Don't show the password again (it's masked anyway). Show the username as the full account name in bold at the top of the dialog (you can use a <description> element for this), e.g. "XMPP account registration for someguy@jabberserver.com", a bit like the page headers in the account wizard. @@ +339,5 @@ > + masked: true, > + required: true, > + isReadonly: true})); > + > + let x = query.getElement(["x"]); Namespace?! @@ +340,5 @@ > + required: true, > + isReadonly: true})); > + > + let x = query.getElement(["x"]); > + if (x) { Better to move this enumerator-building code outside _dialogRequest so this stanza parsing runs before any dialog is opened. Easier if something goes wrong. @@ +376,5 @@ > + fields.add(element.attributes["var"]); > + let isRequired = element.getElement(["required"]) ? true : false; > + > + // Some forms have an OCR field. In that case, show the OCR image > + // and provide input for the same. Do we have an example server that uses this? @@ +429,5 @@ > + session._connectionSecurity != "allow_unencrypted_plain_auth") { > + session.onError(Ci.prplIAccount.ERROR_AUTHENTICATION_IMPOSSIBLE, > + _("connection.error.notSendingPasswordInClear")); > + return; > + } In this case, you don't have to open a dialog at all. In fact, you can probably move this check to startAuth(). @@ +432,5 @@ > + return; > + } > + > + // XEP-0077 (3.1): Entity Registers with a Host. > + let x = query.getElement(["x"]); Haven't we checked this already earlier? Maybe an isDataForm boolean or something like that would be useful. @@ +466,5 @@ > + session.onXmppStanza = session.stanzaListeners.onRegisterResponse; > + }, > + QueryInterface: XPCOMUtils.generateQI([Ci.prplIRequestDialog]) > + }; > + Services.obs.notifyObservers(_dialogRequest, "dialog-request", null); No need to open a dialog if we already have all the info to fill in the form (e.g. if only username/password are asked for). ::: im/modules/ibCore.jsm @@ +290,5 @@ > null, "chrome", aSubject); > return; > } > > + if (aTopic == "dialog-request") { You also have to add this to the corresponding file in mail/ @@ +291,5 @@ > return; > } > > + if (aTopic == "dialog-request") { > + var features = "chrome,modal,titlebar,centerscreen"; This can be a const, if you don't inline it. Please, not modal. Or do you see any reason for it to be modal?

Comment on attachment 8697796 [details] [diff] [review] support XEP-0077 Review of attachment 8697796 [details] [diff] [review]: ----------------------------------------------------------------- ::: im/content/accountWizard.js @@ +274,5 @@ > > + // Adds options to aElement for specific page if aPage is provided, otherwise > + // adds all existing options. > + // Returns true if any option added to aElement, otherwise returns false. > + addOptions: function(aElement, aPage) { Unfortunately there is some duplicated code :-( And so account.js needs similar changes. Both in im/ and in mail/. Even better if you can move the shared/duplicate code into helper functions in a separate .jsm file that can be included by accountWizard and account.js. And maybe dialogRequest too?

Servers known to use OCRs and data forms: sukhe provided these servers For OCR: jabber.ccc.de For data forms: jabber.otr.im dukgo.com jabber.calyxinstitute.org

Comment on attachment 8781123 [details] [diff] [review] v1 - support in-band registration Review of attachment 8781123 [details] [diff] [review]: ----------------------------------------------------------------- What's a good server to actually test the data forms part with? I tried dukgo.com and it worked, but without any visible data forms step. There is a bug when the new account is connected for the first time. The vcard-temp iq get request fails with <error><item-not-found>. IB responds by immediately sending iq get again. This goes on forever... ::: chat/protocols/xmpp/xmpp-session.jsm @@ +391,5 @@ > + errorMsg = _("connection.error.unknownRegistrationError"); > + } > + } > + else if (type == "result") { > + this._account.prefs.setBoolPref("register", false); Let's call this pref "registerOnNextConnect" to be clearer. @@ +393,5 @@ > + } > + else if (type == "result") { > + this._account.prefs.setBoolPref("register", false); > + errorReason = Ci.prplIAccount.NO_ERROR; > + errorMsg = _("connection.onRegistrationSuccess"); Aren't we actually connected at this point and can just continue normally setting up a stream etc? Do you have to disconnect? As this will disconnect, it looks like you're relying on the automatic reconnection mechanism here. Was that intentional? At least add a comment. @@ +427,5 @@ > + let usedDataForms = false; > + > + let parsedElements = new Map(); > + let registrationParser = aQuery => { > + // Exculde elements that their values are known (e.g. username). Nit: exclude elements with known values (e.g. username) @@ +429,5 @@ > + let parsedElements = new Map(); > + let registrationParser = aQuery => { > + // Exculde elements that their values are known (e.g. username). > + let exculdeKnownElements = aElements => { > + return aElements.filter(elm => { nit: element is more readable than elm @@ +574,5 @@ > + // feature. > + let canRegister = false; > + let registration = aStanza.getElement(["register"]); > + if (registration && registration.uri == Stanza.NS.register_feature) > + canRegister = true; You can shorten this to let canRegister = registration && registration.uri == Stanza.NS.register_feature; Move this check inside the next if clause, it's not really needed otherwise. @@ +581,5 @@ > + // in-band registration first (if the server supports it) and then > + // we authenticate. > + if (this._account.getBool("register")) { > + if (!canRegister) { > + this.onError(null, _("connection.error.noRegistrationSupport")); Why null? @@ +587,5 @@ > + } > + else if (!this._encrypted && > + this._connectionSecurity != "allow_unencrypted_plain_auth") { > + this.onError(Ci.prplIAccount.ERROR_AUTHENTICATION_IMPOSSIBLE, > + _("connection.error.notSendingPasswordInClear")); Good :-)

(In reply to aleth [:aleth] from comment #28) > Comment on attachment 8781123 [details] [diff] [review] > v1 - support in-band registration > > Review of attachment 8781123 [details] [diff] [review]: > ----------------------------------------------------------------- > > What's a good server to actually test the data forms part with? I tried > dukgo.com and it worked, but without any visible data forms step. Try jabber.ccc.de?

(In reply to Sukhbir Singh from comment #29) > Try jabber.ccc.de? Somewhat surprisingly, the connection fails as the cert is not accepted: "This certificate was signed using a signature algorithm that is disabled because it is not secure."

(In reply to aleth [:aleth] from comment #30) > (In reply to Sukhbir Singh from comment #29) > > Try jabber.ccc.de? > > Somewhat surprisingly, the connection fails as the cert is not accepted: > "This certificate was signed using a signature algorithm that is disabled > because it is not secure." With jabber.ccc.de, it's not surprising at all :) Yes, unfortunately, you have to add an exception. I also found another server using data forms, planetjabber.de (from https://list.jabber.at/).

(In reply to Sukhbir Singh from comment #31) > With jabber.ccc.de, it's not surprising at all :) Yes, unfortunately, you > have to add an exception. I also found another server using data forms, > planetjabber.de (from https://list.jabber.at/). Looks like these all require captcha support.

Comment on attachment 8781123 [details] [diff] [review] v1 - support in-band registration Review of attachment 8781123 [details] [diff] [review]: ----------------------------------------------------------------- ::: chat/protocols/xmpp/xmpp-session.jsm @@ +443,5 @@ > + return true; > + }); > + }; > + > + // Parses iq:register. Can you expand this comment? Looks like you are translating iq:register fields to data forms standard? @@ +468,5 @@ > + // Ignore elements of other features (e.g x:data form). > + continue; > + } > + else > + throw `Unhandled required element ${child.localName}`; If this happens, which onError call is triggered? What's the code flow? @@ +490,5 @@ > + > + // XEP-0077: The x:data form shall take precedence over the > + // iq:register fields. > + try { > + let parserdElements = parseElements(aQuery); What's parseElements? Looks like there's a typo in parserdElements, do you mean parsedElements? Better to give it a name saying what they are (e.g. dataFormsElements), not what they used to be @@ +493,5 @@ > + try { > + let parserdElements = parseElements(aQuery); > + if (!parserdElements.elements.length) > + return getIQRegisterElements(); > + usedDataForms = true; isDataFormsExchange might be clearer @@ +498,5 @@ > + return { > + elements: exculdeKnownElements(parserdElements.elements), > + showForm: false > + }; > + } catch(e) { Why the try-catch? Should it only be around parseElements? Are you looking for try...finally? You should probably log e somehow or the error message will be lost, unless this error is expected. @@ +512,5 @@ > + return; > + } > + > + // concat results. > + result.forEach((value, key) => parsedElements.set(key, value)); parsedElements is coming from where? Is this a new Map? @@ +514,5 @@ > + > + // concat results. > + result.forEach((value, key) => parsedElements.set(key, value)); > + > + let submittedInfo; Add a comment // Assemble response stanza. responseData may be clearer than submittedInfo as no info has been submitted yet. @@ +518,5 @@ > + let submittedInfo; > + if (usedDataForms) { > + let fieldNodes = []; > + query.getElement(["x"]).children.forEach(child => { > + let name = child.attributes["var"]; Is var optional in the spec or not? Add a comment @@ +539,5 @@ > + } > + else if (parsedElements.size) { > + submittedInfo = []; > + parsedElements.forEach((value, key) => > + submittedInfo.push(Stanza.node(key, null, null, value[0]))); submittedInfo = [...parsedElements].map(key, value => Stanza.node...) would be easier to read

(In reply to aleth [:aleth] from comment #28) > Aren't we actually connected at this point and can just continue normally > setting up a stream etc? Do you have to disconnect? > > As this will disconnect, it looks like you're relying on the automatic > reconnection mechanism here. Was that intentional? At least add a comment. Yes, I added a comment for that. (In reply to aleth [:aleth] from comment #33) > @@ +468,5 @@ > > + // Ignore elements of other features (e.g x:data form). > > + continue; > > + } > > + else > > + throw `Unhandled required element ${child.localName}`; > > If this happens, which onError call is triggered? What's the code flow? You are right, I added |onError| before it > @@ +498,5 @@ > > + return { > > + elements: exculdeKnownElements(parserdElements.elements), > > + showForm: false > > + }; > > + } catch(e) { > > Why the try-catch? Should it only be around parseElements? Are you looking > for try...finally? Yes, I did that as I thought it will be more readable in one block. > @@ +512,5 @@ > > + return; > > + } > > + > > + // concat results. > > + result.forEach((value, key) => parsedElements.set(key, value)); > > parsedElements is coming from where? Is this a new Map? No, it was for elements with known values. I changed its name to |knownElements|

(In reply to aleth [:aleth] from comment #28) > There is a bug when the new account is connected for the first time. The > vcard-temp iq get request fails with <error><item-not-found>. IB responds by > immediately sending iq get again. This goes on forever... I don't think this is fixed. > > + else if (type == "result") { > > + this._account.prefs.setBoolPref("register", false); > > + errorReason = Ci.prplIAccount.NO_ERROR; > > + errorMsg = _("connection.onRegistrationSuccess"); > > Aren't we actually connected at this point and can just continue normally > setting up a stream etc? Do you have to disconnect? You didn't answer my question. Is disconnecting here part of the spec? What goes wrong if you continue with startAuth() at this point?

Comment on attachment 8782059 [details] [diff] [review] v2 - support in-band registration Review of attachment 8782059 [details] [diff] [review]: ----------------------------------------------------------------- ::: chat/locales/en-US/xmpp.properties @@ +39,5 @@ > connection.error.failedToGetAResource=Failed to get a resource > connection.error.failedMaxResourceLimit=This account is connected from too many places at the same time. > connection.error.failedResourceNotValid=Resource is not valid. > connection.error.XMPPNotSupported=This server does not support XMPP > +connection.error.noRegistrationSupport=This server does not support in-band registration This server does not support in-band registration, so you have to register another way before connecting @@ +40,5 @@ > connection.error.failedMaxResourceLimit=This account is connected from too many places at the same time. > connection.error.failedResourceNotValid=Resource is not valid. > connection.error.XMPPNotSupported=This server does not support XMPP > +connection.error.noRegistrationSupport=This server does not support in-band registration > +connection.error.noRegistrationSupport.client=This client does not support features that are required for registration This client does not support the required kind of in-band registration, so you have to register another way before connecting @@ +44,5 @@ > +connection.error.noRegistrationSupport.client=This client does not support features that are required for registration > +connection.error.registrationCancel=Registration canceled by the user > +connection.error.registrationConflict=Username is not available > +connection.error.registrationNotAcceptable=Some required information was not provided > +connection.error.unknownRegistrationError=Unknown error on account registration Unknown error during account registration ::: chat/protocols/xmpp/xmpp-session.jsm @@ +433,5 @@ > + let knownElements = new Map(); > + > + let registrationParser = aQuery => { > + // Exclude elements with known values (e.g. username). > + let exculdeKnownElements = aElements => { nit: exculde -> exclude @@ +434,5 @@ > + > + let registrationParser = aQuery => { > + // Exclude elements with known values (e.g. username). > + let exculdeKnownElements = aElements => { > + return aElements.filter(element => { You can probably save a layer of brackets here aElements => aElements.filter(...) @@ +435,5 @@ > + let registrationParser = aQuery => { > + // Exclude elements with known values (e.g. username). > + let exculdeKnownElements = aElements => { > + return aElements.filter(element => { > + // Escape elements that do not have name property // Skip elements... @@ +460,5 @@ > + let usedDataForms = false; > + let iqElements = []; > + for (let child of aQuery.children) { > + if (child.localName == "instructions") > + iqElements.push({type: child.localName, value: child.innerText}); If you wanted to write this in a more functional way, you could do iqElements = aQuery.children.map(child => object or undefined).filter(remove undefined entries) @@ +510,5 @@ > + this.LOG(e); > + return getIQRegisterElements(); > + } > + if (!dataFormsElements.elements.length) > + return getIQRegisterElements(); The double getIQRegisterElements still feels wrong here. Does this do what you want: try { dataFormsElements = parseElements(aQuery); } catch(e) { this.LOG(e); // should this be this.WARN or this.ERROR or is it expected? If it's expected, add a comment saying when it happens } if (!dataFormsElements || !dataFormsElements.length) return getIQRegisterElements(); @@ +559,5 @@ > + responseData = > + Stanza.node("x", Stanza.NS.xdata, {"type": "submit"}, fieldNodes); > + } > + else if (result.size) { > + responseData = [...result].map(value => Use destructuring to make this more readable: .map([nodename, value] => Stanza.node(nodename, ...)

(In reply to aleth [:aleth] from comment #36) > > > + else if (type == "result") { > > > + this._account.prefs.setBoolPref("register", false); > > > + errorReason = Ci.prplIAccount.NO_ERROR; > > > + errorMsg = _("connection.onRegistrationSuccess"); > > > > Aren't we actually connected at this point and can just continue normally > > setting up a stream etc? Do you have to disconnect? > > You didn't answer my question. Is disconnecting here part of the spec? What > goes wrong if you continue with startAuth() at this point? No, it's not part of the spec and we should continue with startAuth() in this patch.

Comment on attachment 8782211 [details] [diff] [review] v3 - support in-band registration Review of attachment 8782211 [details] [diff] [review]: ----------------------------------------------------------------- This is looking pretty good, but for the final reviews it would help - to have the data forms bug reviewed (ping clokep and nhnt11) - to include CAPTCHAs (here or in a followup bug) because the servers I can find don't have nontrivial forms that aren't CAPTCHAs. ::: chat/protocols/xmpp/xmpp-session.jsm @@ +502,5 @@ > + // XEP-0077: The x:data form shall take precedence over the > + // iq:register fields. > + let dataFormsElements; > + try { > + dataFormsElements = parseElements(aQuery); I was looking around trying to find parseElements... it's more readable to call it XMPPDataForms.parseElements (i.e. have XMPPDataForms only export one object). @@ +513,5 @@ > + > + isDataFormsExchange = true; > + return { > + elements: excludeKnownElements(dataFormsElements.elements), > + showForm: false Why is showForm always false? @@ +517,5 @@ > + showForm: false > + }; > + }; > + > + XMPPDataForms(_("connection.registering"), this._account, query, result => { XMPPDataForms.handleForm(...) maybe? @@ +518,5 @@ > + }; > + }; > + > + XMPPDataForms(_("connection.registering"), this._account, query, result => { > + if (result === FORM_CANCELLED) { same here, XMPPDataForms.FORM_CANCELLED would be better @@ +557,5 @@ > + responseData = > + Stanza.node("x", Stanza.NS.xdata, {"type": "submit"}, fieldNodes); > + } > + else if (result.size) { > + responseData = [...result].map(([nodename, value]) => Add a comment why in this case only value[0] matters. ::: chat/protocols/xmpp/xmpp.jsm @@ +2600,1 @@ > this._sendVCard(); I'm not sure we should always send the vcard, what does the spec say about this? It makes sense to send the vCard if onUserVCard received an error stanza with item-not-found, but why does it make sense to send it in every other case?

Comment on attachment 8780511 [details] [diff] [review] v1 - protocol-independant changes Review of attachment 8780511 [details] [diff] [review]: ----------------------------------------------------------------- This needs matching changes in Thunderbird and purplexpcom. ::: chat/components/public/prplIPref.idl @@ +21,5 @@ > readonly attribute short type; > readonly attribute boolean masked; > > + // Used to specify the preferred page in which option will be shown and > + // allowed values are username, password and advancedOptions. I don't think this patch supports "passport" yet, so remove it. ::: chat/modules/jsProtoHelper.jsm @@ +777,5 @@ > + if ("page" in aOption) { > + const kPages = ["username", "password", "advancedOptions"]; > + let condition = kPages.some(p => p == aOption.page); > + if (!condition) > + throw "Invalid option page"; Include condition in the throw message.

I was attempting to test bug 1269331 by using these patches...but they seem to have bitrotted. I tried to resolve the conflict and I ended up not having an XMPP protocol (so I likely messed it up). Can you please un-bitrot these?

This is missing the libpurple part, so I can't actually test this.

I think you were able to test it according you comment in bug 1269331 Comment 20