1286905 - Support deleting an XMPP account on the server

Status: NEW

(Chat Core :: XMPP, defect)

Description

[Abdelrahman Ahmed [:abdelrahman]]

Implement section 3.2 (Entity Cancels an Existing Registration) in XEP-0077: In-Band Registration.

Comment 1

[Abdelrahman Ahmed [:abdelrahman]]

I suggest adding a check box in the deletePrompt that says "Delete account on server too." and when it's checked, we execute implementation of section 3.2?

Comment 2

[aleth [:aleth]]

That sounds reasonable, but you need to find a way to only add that checkbox when the prpl supports it.

Comment 3

[Abdelrahman Ahmed [:abdelrahman]]

(In reply to aleth [:aleth] from comment #2)
> That sounds reasonable, but you need to find a way to only add that checkbox
> when the prpl supports it.

Add a boolean |canDeleteOnServer| to the interface |prplIProtocol| [1] and set false as a default value, so we can check that from account account.imAccount.canDeleteOnServer

I noticed that prompts can have only one checkbox [2], so I think we can pop up two prompts (the first will be shown if the prpl supports delete on server and when the user confirms that, the second will not be shown, otherwise do normal checks for promptOnDelete pref [3]).

[1] https://dxr.mozilla.org/comm-central/source/chat/components/public/prplIProtocol.idl#86
[2] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIPromptService#confirmEx()
[3] https://dxr.mozilla.org/comm-central/source/im/content/accounts.js#297

Comment 4

[aleth [:aleth]]

OK. The question about deleting on the server should probably be shown after the "are you sure you want to delete" check though.

Comment 5

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: v1 - WIP

Can I add a general method (void deleteOnServer();) for all protocols in |prplIAccount|[1]?

[1] https://dxr.mozilla.org/comm-central/source/chat/components/public/imIAccount.idl#78

Comment 6

[aleth [:aleth]]

> Can I add a general method (void deleteOnServer();) for all protocols in
> |prplIAccount|[1]?

Adding an optional boolean parameter to deleteAccount() etc might be better. There is a potential issue with timing, however. Deleting the account on the server is likely to be an async process. You'll have to investigate how this best fits with removing the account at the IB end.

Regarding the idl changes, flo may have some useful input.

Comment 7

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: v2 - WIP

(In reply to aleth [:aleth] from comment #6)
> > Can I add a general method (void deleteOnServer();) for all protocols in
> > |prplIAccount|[1]?
> 
> Adding an optional boolean parameter to deleteAccount() etc might be better.
> There is a potential issue with timing, however. Deleting the account on the
> server is likely to be an async process. You'll have to investigate how this
> best fits with removing the account at the IB end.
> 
> Regarding the idl changes, flo may have some useful input.

I added optional boolean parameter to deleteAccount() to indicate delete on server or not.

I added another boolean to |prplIAccount| to be used as 
1 - a check when the account gets disconnected
2 - in observer "account-connected" in imAccounts to perform deleting of account when it's connectd

Comment 8

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: v1 - XMPP WIP

Comment 9

[aleth [:aleth]]

Comment on attachment 8783998 [details] [diff] [review]
v2 - WIP

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

This looks like it could work, but it's unfortunate that the code is spread out over so many files. That makes it hard to figure out what happens e.g. if the connection fails for some reason or if the account deletion fails. What does happen? 

Have you considered as an alternative that remove() gets an optional parameter remove(deleteOnServer)? I think when remove is called the account is already disconnected. So if deleteOnServer is set, then remove() in the prpl would have to trigger connecting the account and then doing the deletion (as the final "connection" step). That way all the state (the boolean flags) would be inside the prpl and not have to be exposed via an interface. Would something like that work?

::: chat/components/public/prplIProtocol.idl
@@ +86,5 @@
>    readonly attribute boolean usePurpleProxy;
>  
> +  // Indicates if the protocol plugin can cancel registration of
> +  // accounts on server.
> +  readonly attribute boolean deleteAccountOnServer;

Is that the same as deleteOnServer?

Comment 10

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: v3 - WIP

(In reply to aleth [:aleth] from comment #9)
> This looks like it could work, but it's unfortunate that the code is spread
> out over so many files. That makes it hard to figure out what happens e.g.
> if the connection fails for some reason or if the account deletion fails.
> What does happen? 

If the connection fails, the account will not be deleted in IB.
If the account deletion fails, the account will be deleted in IB and the error could be shown to the user as notification or ignored.

> Have you considered as an alternative that remove() gets an optional
> parameter remove(deleteOnServer)? I think when remove is called the account
> is already disconnected. So if deleteOnServer is set, then remove() in the
> prpl would have to trigger connecting the account and then doing the
> deletion (as the final "connection" step). That way all the state (the
> boolean flags) would be inside the prpl and not have to be exposed via an
> interface. Would something like that work?

This seems better. I applied that in this patch

> ::: chat/components/public/prplIProtocol.idl
> @@ +86,5 @@
> >    readonly attribute boolean usePurpleProxy;
> >  
> > +  // Indicates if the protocol plugin can cancel registration of
> > +  // accounts on server.
> > +  readonly attribute boolean deleteAccountOnServer;
> 
> Is that the same as deleteOnServer?

Yes, but this used to show the prompt of deleting on server when the protocol of the account supports that.

Comment 11

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: v2 - XMPP WIP

Comment 12

[aleth [:aleth]]

Comment on attachment 8786029 [details] [diff] [review]
v2 - XMPP WIP

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

This looks cleaner, yes. 

It assumes disconnect() is synchronous for XMPP - is it?

What's the sequence now? Is unInit() called at all somewhere during this process?

::: chat/protocols/xmpp/xmpp.jsm
@@ +1336,5 @@
>                        this.getString("connection_security"), this._jid,
>                        this.imAccount.password, this);
>    },
>  
> +  remove: function(aDeleteOnServer) {

Better check this.disconnected to be safe, and this.ERROR() if not.

@@ +1696,5 @@
>    /* XMPPSession events */
>  
>    /* Called when the XMPP session is started */
>    onConnection: function() {
> +    if (this._deleteOnConnection) {

Add this.reportConnecting calls to show in the account manager what's going on.

Or is the account already removed from the account manager at this point? When exactly does that happen?

@@ +1703,5 @@
> +                                    Stanza.node("query", Stanza.NS.register,
> +                                    null, removeStanza));
> +      this.sendStanza(cancelResponse, aStanza => {
> +        // Handle result and error checking...
> +        this.imAccount.observe(this, "account-deleted", null);

what happens then?

Comment 13

[Abdelrahman Ahmed [:abdelrahman]]

(In reply to aleth [:aleth] from comment #12)
> It assumes disconnect() is synchronous for XMPP - is it?

Yes. (it should, as if we didn't get a response from server (e.g. delay, etc.), the socket will be closed)

> What's the sequence now? Is unInit() called at all somewhere during this
> process?

Let's describe behaviour with this case assuming deleteAccount is true:
After the prompt messages, |deleteAccount| is called. 
it calls |remove| after getting accout object then returns.
|remove| disconnects the account calls |remove| of prplIAccount object then returns (unInit is not called).
|remove| of prplIAccount triggers a connection and sets private flag |_deleteOnConnection| to remove account on server when it's connected.
After finishing deleting stuff, we report to imAccounts that account is deleted using "account-deleted", so it calls |deleteAccount| with false value for |deleteAccount| to let deleted from IB with the normal way.

Comment 14

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: v3 - XMPP WIP

Comment 15

[aleth [:aleth]]

Comment on attachment 8786028 [details] [diff] [review]
v3 - WIP

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

::: chat/components/src/imAccounts.js
@@ +648,5 @@
>      }
>      if (this.connected || this.connecting)
>        this.disconnect();
>      if (this.prplAccount)
> +      this.prplAccount.remove(aDeleteOnServer);

How can the connection work if we've removed the password from the password manager already?

Comment 16

[aleth [:aleth]]

Comment on attachment 8786515 [details] [diff] [review]
v3 - XMPP WIP

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

::: chat/protocols/xmpp/xmpp.jsm
@@ +1708,5 @@
> +      let cancelResponse = Stanza.iq("set", null, null,
> +                                    Stanza.node("query", Stanza.NS.register,
> +                                    null, removeStanza));
> +      this.sendStanza(cancelResponse, aStanza => {
> +        // Handle result and error checking...

It would be good to see some more details here to be able to understand if the rest of the code flow works.

Comment 17

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: v1 - protocol independent changes

(In reply to aleth [:aleth] from comment #15)
> Comment on attachment 8786028 [details] [diff] [review]
> v3 - WIP
> 
> Review of attachment 8786028 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: chat/components/src/imAccounts.js
> @@ +648,5 @@
> >      }
> >      if (this.connected || this.connecting)
> >        this.disconnect();
> >      if (this.prplAccount)
> > +      this.prplAccount.remove(aDeleteOnServer);
> 
> How can the connection work if we've removed the password from the password
> manager already?

You are right. fixed in this patch.

Comment 18

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: v1 - XMPP changes

Comment 19

[aleth [:aleth]]

Comment on attachment 8792205 [details] [diff] [review]
v1 - protocol independent changes

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

::: chat/components/public/imIAccount.idl
@@ +116,4 @@
>     */
>    void requestBuddyInfo(in AUTF8String aBuddyName);
>  
> +  readonly attribute boolean showDeleteOnServerPrompt;

Why is this needed? If it's just to not ask about deletion on the server after a failure, that doesn't seem necessary, also the user may insist on trying again.

::: chat/components/public/imIAccountsService.idl
@@ +42,5 @@
>    /* will fire the event account-added */
>    imIAccount createAccount(in AUTF8String aName, in AUTF8String aPrpl);
>  
> +  /* will fire the event account-removed if the account is removed
> +     aDeleteOnServer is false by default.

If you want that parameter to be optional, you have to mark it optional too.

::: chat/components/public/prplIProtocol.idl
@@ +85,5 @@
>    // account, or uses the Mozilla proxy settings for all accounts.
>    readonly attribute boolean usePurpleProxy;
>  
> +  // Indicates if the protocol plugin can cancel registration of
> +  // accounts on server.

on the server

@@ +86,5 @@
>    readonly attribute boolean usePurpleProxy;
>  
> +  // Indicates if the protocol plugin can cancel registration of
> +  // accounts on server.
> +  readonly attribute boolean deleteAccountOnServer;

canDeleteAccountOnServer would be clearer.

::: chat/components/src/imAccounts.js
@@ +298,4 @@
>          return;
>        }
>      }
> +    else if (aTopic == "account-deleted") {

account-deleted-on-server to avoid confusion with account-removed

::: im/content/accounts.js
@@ +318,5 @@
>  
> +
> +    let account = Services.accounts.getAccountById(selectedItem.id);
> +    if (!account)
> +      throw Cr.NS_ERROR_INVALID_ARG;

Has this happened to you in testing?

@@ +321,5 @@
> +    if (!account)
> +      throw Cr.NS_ERROR_INVALID_ARG;
> +
> +    var deleteOnServer = false;
> +    var showDeleteOnServerPrompt = false;

Use let in new code.

::: im/locales/en-US/chrome/instantbird/accounts.properties
@@ +25,4 @@
>  account.deletePrompt.message=Are you sure you want to delete this account?
>  account.deletePrompt.checkbox=Do &not ask next time
>  account.deletePrompt.button=&Delete
> +account.deleteOnServerPrompt.title=Delete Account on server?

Delete Account On The Server?

@@ +25,5 @@
>  account.deletePrompt.message=Are you sure you want to delete this account?
>  account.deletePrompt.checkbox=Do &not ask next time
>  account.deletePrompt.button=&Delete
> +account.deleteOnServerPrompt.title=Delete Account on server?
> +account.deleteOnServerPrompt.message=Do you want to delete this account on server too?

This could be a bit more explicit to help remove confusion, like:

Would you like to delete the account on the server too? (If you choose 'no', you will remain registered on the server, and only the Instantbird account will be removed.)

Comment 20

[aleth [:aleth]]

Comment on attachment 8792207 [details] [diff] [review]
v1 - XMPP changes

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

::: chat/protocols/xmpp/xmpp.js
@@ +52,4 @@
>      port: {get label() { return _("options.connectPort"); }, default: 5222}
>    },
>    get chatHasTopic() { return true; },
> +  get deleteAccountOnServer() { return true; },

Is this not dependent on some service discovery thing?

::: chat/protocols/xmpp/xmpp.jsm
@@ +1352,5 @@
> +
> +    if (aDeleteOnServer) {
> +      this._deleteOnConnection = true;
> +      this.connect();
> +    }

Move this above the various close() and roster deletions, because if the delete on the server fails, you want to be back in the same state as before the user clicked "delete".

@@ +1709,5 @@
> +      this.reportConnecting(_("connection.deleteAccountOnServer"));
> +      let removeStanza = Stanza.node("remove", null,  null, null);
> +      let cancelResponse = Stanza.iq("set", null, null,
> +                                    Stanza.node("query", Stanza.NS.register,
> +                                    null, removeStanza));

nit: indent

@@ +1712,5 @@
> +                                    Stanza.node("query", Stanza.NS.register,
> +                                    null, removeStanza));
> +      this.sendStanza(cancelResponse, aStanza => {
> +        if (aStanza.attributes["type"] == "result") {
> +          this.imAccount.observe(this, "account-deleted", null);

Would be nice to add a this.reportConnecting call here saying something like "Account successfully cancelled on the server."

@@ +1713,5 @@
> +                                    null, removeStanza));
> +      this.sendStanza(cancelResponse, aStanza => {
> +        if (aStanza.attributes["type"] == "result") {
> +          this.imAccount.observe(this, "account-deleted", null);
> +          return true;

Don't you need to disconnect here, or does the spec guarantee that the server will do it?

Comment 21

[aleth [:aleth]]

You also need to add matching changes for Thunderbird and purplexpcom.

Comment 22

[aleth [:aleth]]

This looks like it is almost complete. It would be nice to land it in the 52 cycle (i.e. before 2016-11-07), as 52 will be the next Thunderbird ESR release once it has gone through aurora/beta. (The same is true for in-band registration. Don't hesitate to ping your reviewers who have been very slow on this...)

Comment 23

[Patrick Cloke [:clokep]]

Resetting assignee due to lack of activity.