1057029 - update certificate exception handling in chat to deal with bug 940506

Status: RESOLVED FIXED

(Chat Core :: General, defect)

Description

[Magnus Melin [:mkmelin]]

+++ This bug was initially created as a clone of Bug #1046328 +++

Bug 940506 removed the nsIRecentBadCerts interface and implementation, which means that the old way of adding certificate exceptions do not work. I thought there was a bug on this already, but I guess not. What needs to happen is everywhere Thunderbird opens the certificate exception override dialog, it needs to pass along the nsISSLStatus from the connection that failed. See for reference the patch in bug 1025332.

---

In bug 1046328 I'm fixing it for most of thunderbird, but I'll leave just a fixme comment for chat, as the status needed isn't (at least easily?) accessible there.

Comment 1

[Patrick Cloke [:clokep]]

Attachment: c-c part v1

Magnus, thanks for filing this.

Florian, I tested this using an IRC account to irc.oftc.net:6697.

I'm not really positive about the libpurple part and would appreciate feedback!

Comment 2

[Patrick Cloke [:clokep]]

Attachment: purple part v1

Comment 3

[Florian Quèze [:florian]]

Comment on attachment 8478744 [details] [diff] [review]
c-c part v1

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

r- for never clearing _sslStatus, but this looks good :-).

::: chat/components/public/imIAccount.idl
@@ +147,5 @@
>    /* When a certificate error occurs, the host/port that caused a
>     * SSL/certificate error when connecting to it. This is only valid when
>     * connectionErrorReason is one of ERROR_CERT_*. */
>    readonly attribute AUTF8String connectionTarget;
> +  /* When a certificate error occurs, the nsISSLStatus error of the socket. This

nit: Looks like the other attributes have an empty line before their comment.

::: chat/modules/jsProtoHelper.jsm
@@ +60,5 @@
>  
>      if (aIsSslError)
>        return Ci.prplIAccount.ERROR_ENCRYPTION_ERROR;
>  
> +    this._sslStatus = aSocket.sslStatus;

I wonder if I wouldn't prefer:
let sslStatus = this._sslStatus = aSocket.sslStatus;

I know the .sslStatus getter does almost nothing, but we are calling it several times for no real reason :-/.

@@ +87,5 @@
>      return Ci.prplIAccount.ERROR_CERT_OTHER_ERROR;
>    },
>    _connectionTarget: "",
>    get connectionTarget() this._connectionTarget,
> +  _sslStatus: null,

Should we clear _connectionTarget and _sslStatus in the reportConnecting method?

::: im/content/accounts.js
@@ +208,5 @@
>      }
>    },
>    addException: function am_addException() {
>      let account = this.accountList.selectedItem.account;
>      if (!account.disconnected || !account.prplAccount.connectionTarget)

Should we add a |let prplAccount = account.prplAccount;| here to avoid calling this getter 3 times?

Comment 4

[Patrick Cloke [:clokep]]

Attachment: c-c part v2

Comment 5

[Florian Quèze [:florian]]

Comment on attachment 8478985 [details] [diff] [review]
c-c part v2

Thanks!

Comment 6

[Patrick Cloke [:clokep]]

https://hg.mozilla.org/comm-central/rev/fa4b68528277
http://hg.mozilla.org/users/florian_queze.net/purple/rev/29ed2e68dde1