Bug 30460

This blocks https://bugzilla.gnome.org/show_bug.cgi?id=630896

One problem with this: adding TpTLSCertificate requires us to have an idea of its life-cycle.

When a TLSCertificate is accepted or rejected, does it disappear from the bus (i.e. it should be invalidated), or does it remain present?

(Currently, the answer in Gabble is: it stays.)

When a TLSCertificate appears, will it always be associated with a Channel?

(Currently, the answer in Gabble is: yes for ServerTLSAuthentication; no idea for end-to-end encryption.)

When a TLSCertificate's Channel (if any) closes, should it be invalidated?

(Currently, the answer in Gabble is: yes.)

Branch so far, compiles, no tests, might not work :-)

(In reply to comment #2)
> When a TLSCertificate is accepted or rejected, does it disappear from the bus
> (i.e. it should be invalidated), or does it remain present?

I went for: remains present.

> When a TLSCertificate appears, will it always be associated with a Channel?
> 
> (Currently, the answer in Gabble is: yes for ServerTLSAuthentication; no idea
> for end-to-end encryption.)

I went for: not necessarily, but it will always have either a Channel or a Connection.

> When a TLSCertificate's Channel (if any) closes, should it be invalidated?

Yes.

What's blocking this? Just the tests?

(In reply to comment #4)
> What's blocking this? Just the tests?

Tests, review, preferably a real application using it. You're welcome to pick it up if you want.

On it.

I rebased your branch on top of master, fix a few issues and added test:
http://cgit.collabora.com/git/user/cassidy/telepathy-glib/log/?h=tls-30460

I'm going to port Empathy to it but my patches can already be reviewed.

(In reply to comment #7)
> I'm going to port Empathy to it but my patches can already be reviewed.

The port was pretty straight foward except for the test which doesn't have a TpConnection or TpChannel to pass to the constructor. What about making the parent optional?

(I realise I probably wrote a lot of the code I'm criticizing here. In my defence, it was over a year ago!)

+typedef struct {
...
+    GHashTable *details;
+} SignalledRejection;

Could this be a GVariant of type G_VARIANT_TYPE_VARDICT, converted from D-Bus with _tp_asv_to_vardict()?

(I would like new extensibility points to be GVariant-based if possible, so we don't have to keep adding parallel API.)

+  GPtrArray *cert_data;

I don't know what type we should use for this in the glorious GIO-compatible future, but I feel that GPtrArray<GArray<guchar>> is probably not it.

Perhaps GList<GByteArray>, or GList<GBytes> with a GLib 2.32 dependency?

Using a GList would mean we can't make it a property.

If in doubt, make it not be a property, and we can give it a GList<GByteArray> getter now, and a GList<GBytes> getter later?

It's a pity we can't use a GList<GTlsCertificate>, but GTlsCertificate is X.509-only and only supports PEM (ASCII-armoured), whereas we support either PGP or X.509 in their respective binary formats.

+void
+tp_tls_certificate_add_rejection (TpTLSCertificate *self,
+    TpTLSCertificateRejectReason reason,
+    const gchar *dbus_error,
+    GHashTable *details)

Again, details should be a GVariant of type G_VARIANT_TYPE_VARDICT, or NULL (to be treated as an empty dict). tp_account_update_parameters_vardict_async() has logic you can steal - it'd probably be worth adding a _tp_asv_from_vardict().

+ * tp_tls_certificate_get_nth_rejection:

Perhaps we'd be better off returning a GList<TpTLSCertificateRejection> where TpTCR is a small object with accessors?

For g-i, it could have a method something like

    gboolean tp_tls_certificate_rejection_raise_error (TpTCR *, GError **);

which g-i bindings would turn into something usable like this:

    try:
        tcr.raise_error()
    except Exception, e:
        # ...

(In reply to comment #8)
> The port was pretty straight foward except for the test which doesn't have a
> TpConnection or TpChannel to pass to the constructor. What about making the
> parent optional?

Doesn't this mean the "parent dies => cert invalidated" logic is never tested?

(In reply to comment #10)
> (In reply to comment #8)
> > The port was pretty straight foward except for the test which doesn't have a
> > TpConnection or TpChannel to pass to the constructor. What about making the
> > parent optional?
> 
> Doesn't this mean the "parent dies => cert invalidated" logic is never tested?

I forgot to test it in tp-glib; I just added a test for that.

(In reply to comment #9)
> +  GPtrArray *cert_data;
> 
> I don't know what type we should use for this in the glorious GIO-compatible
> future, but I feel that GPtrArray<GArray<guchar>> is probably not it.
> 
> Perhaps GList<GByteArray>, or GList<GBytes> with a GLib 2.32 dependency?
> 
> Using a GList would mean we can't make it a property.
> 
> If in doubt, make it not be a property, and we can give it a GList<GByteArray>
> getter now, and a GList<GBytes> getter later?

Why not use GPtrArray<GByteArray> or GPtrArray<GBytes> so it can be a property?
As I already said in bug #39189 I don't list GList * as a public container.

I don't think we should refrain us from depending on GLib 2.32 btw.

(In reply to comment #9)
> (I realise I probably wrote a lot of the code I'm criticizing here. In my
> defence, it was over a year ago!)
> 
> +typedef struct {
> ...
> +    GHashTable *details;
> +} SignalledRejection;
> 
> Could this be a GVariant of type G_VARIANT_TYPE_VARDICT, converted from D-Bus
> with _tp_asv_to_vardict()?
> 
> (I would like new extensibility points to be GVariant-based if possible, so we
> don't have to keep adding parallel API.)

done.

> +void
> +tp_tls_certificate_add_rejection (TpTLSCertificate *self,
> +    TpTLSCertificateRejectReason reason,
> +    const gchar *dbus_error,
> +    GHashTable *details)
> 
> Again, details should be a GVariant of type G_VARIANT_TYPE_VARDICT, or NULL (to
> be treated as an empty dict). tp_account_update_parameters_vardict_async() has
> logic you can steal - it'd probably be worth adding a _tp_asv_from_vardict().

done.

> + * tp_tls_certificate_get_nth_rejection:
> 
> Perhaps we'd be better off returning a GList<TpTLSCertificateRejection> where
> TpTCR is a small object with accessors?
> 
> For g-i, it could have a method something like
> 
>     gboolean tp_tls_certificate_rejection_raise_error (TpTCR *, GError **);
> 
> which g-i bindings would turn into something usable like this:
> 
>     try:
>         tcr.raise_error()
>     except Exception, e:
>         # ...

Sounds like a good idea. I've done that.

The branch has been updated. I think I fixed all your comments except Comment 12.

(In reply to comment #12)
> (In reply to comment #9)
> > +  GPtrArray *cert_data;
> > 
> > I don't know what type we should use for this in the glorious GIO-compatible
> > future, but I feel that GPtrArray<GArray<guchar>> is probably not it.
> > 
> > Perhaps GList<GByteArray>, or GList<GBytes> with a GLib 2.32 dependency?
> > 
> > Using a GList would mean we can't make it a property.
> > 
> > If in doubt, make it not be a property, and we can give it a GList<GByteArray>
> > getter now, and a GList<GBytes> getter later?
> 
> Why not use GPtrArray<GByteArray> or GPtrArray<GBytes> so it can be a property?
> As I already said in bug #39189 I don't list GList * as a public container.

I switched to GptrArray<GBytes>. I can switch to GList if you really want to.

(In reply to comment #14)

> I switched to GptrArray<GBytes>. I can switch to GList if you really want to.

Top commit is now the GList port.

I'd like to move the inclusion of tp_cli_* to a separate header (or even just only include them in the .c) so people don't start depending on them.

> +  GError *error /* badger */ ;

We should probably just get rid of that grep, it's more trouble than it's worth. :-(

> +      NUM_TP_TLS_CERTIFICATE_REJECT_REASONS,

I'd prefer TP_NUM_ ideally.

> +      default:
> +        /* what does it mean? we just don't know */
> +        self->priv->state = state;
> +        g_object_notify ((GObject *) self, "state");

Doesn't state have a range-limited type? If so, this is invalid. :-(

We can either insist that the state is in range (and treat out-of-range values as invalidation with TP_DBUS_ERROR_INCONSISTENT), or remove the range-limit on the GObject property and let API users deal with it.

I'd be inclined to say the three states (pending, accepted, rejected) are exhaustive, so an out-of-range value is always a CM bug. We should put that in the spec really.

The rest looks OK.

(In reply to comment #15)
> Top commit is now the GList port.

Since we don't really have any consensus on whether to go to GList or not, please drop that commit. Sorry.

(In reply to comment #16)
> I'd like to move the inclusion of tp_cli_* to a separate header (or even just
> only include them in the .c) so people don't start depending on them.

done.

> > +  GError *error /* badger */ ;
> 
> We should probably just get rid of that grep, it's more trouble than it's
> worth. :-(

done.

> > +      NUM_TP_TLS_CERTIFICATE_REJECT_REASONS,
> 
> I'd prefer TP_NUM_ ideally.

done.

> > +      default:
> > +        /* what does it mean? we just don't know */
> > +        self->priv->state = state;
> > +        g_object_notify ((GObject *) self, "state");
> 
> Doesn't state have a range-limited type? If so, this is invalid. :-(
> 
> We can either insist that the state is in range (and treat out-of-range values
> as invalidation with TP_DBUS_ERROR_INCONSISTENT), or remove the range-limit on
> the GObject property and let API users deal with it.
> 
> I'd be inclined to say the three states (pending, accepted, rejected) are
> exhaustive, so an out-of-range value is always a CM bug. We should put that in
> the spec really.

fixed.

(In reply to comment #17)
> (In reply to comment #15)
> > Top commit is now the GList port.
> 
> Since we don't really have any consensus on whether to go to GList or not,
> please drop that commit. Sorry.

No problem; removed.

I rebased the branch on top of master and added 2 extra commits.

> +<SUBSECTION>
> +tp_cli_authentication_tls_certificate_call_accept
> +tp_cli_authentication_tls_certificate_call_reject
> +tp_cli_authentication_tls_certificate_callback_for_accept
> +tp_cli_authentication_tls_certificate_callback_for_reject
> +tp_cli_authentication_tls_certificate_connect_to_accepted
> +tp_cli_authentication_tls_certificate_connect_to_rejected
> +tp_cli_authentication_tls_certificate_signal_callback_accepted
> +tp_cli_authentication_tls_certificate_signal_callback_rejected

Please undocument these. (Now there's something you don't hear every day... :-)

Changing it to <SUBSECTION Private> would do nicely.

> + * tp_tls_certificate_rejection_raise_error:

should also document:

Returns: %FALSE

> +_TP_AVAILABLE_IN_0_20
> +struct _TpTLSCertificateRejectionClass {

Not portable. In MSVC++, those macros expand to __declspec(deprecated) or something, which can only be applied to extern symbols (functions/variables), not to types.

If we really need it, we could add _TP_GNUC_AVAILABLE_IN_0_20 which only does anything in gcc but can appear in more places - but we'd have to define it ourselves, using G_UNAVAILABLE in gcc >= 4.5 and no expansion otherwise.

Or, we could just drop it. If you can't call any function that returns one of these objects then in practice that'll be enough.

+      g_variant_builder_init (&builder, G_VARIANT_TYPE ("a{sv}"));
+
+      rej = _tp_tls_certificate_rejection_new (error,
+          TP_TLS_CERTIFICATE_REJECT_REASON_UNKNOWN, TP_ERROR_STR_CERT_INVALID,
+          g_variant_builder_end (&builder));

g_variant_builder_end returns a new floating ref, and I was worried that _tp_tls_certificate_rejection_new doesn't seem to sink the floating reference... but g_object_new() probably uses a GValue, which sinks floating variants automatically.

This seems a bit subtle, it might be better if _tp_tls_certificate_rejection_new explicitly did:

g_variant_ref_sink (details);
ret = g_object_new (..., details, ...);
g_variant_unref (details);
return ret;

I think _tp_tls_certificate_rejection_new should certainly document that it sinks floating references. (Assuming it does...)

(In reply to comment #20)
> > +<SUBSECTION>
> > +tp_cli_authentication_tls_certificate_call_accept
> > +tp_cli_authentication_tls_certificate_call_reject
> > +tp_cli_authentication_tls_certificate_callback_for_accept
> > +tp_cli_authentication_tls_certificate_callback_for_reject
> > +tp_cli_authentication_tls_certificate_connect_to_accepted
> > +tp_cli_authentication_tls_certificate_connect_to_rejected
> > +tp_cli_authentication_tls_certificate_signal_callback_accepted
> > +tp_cli_authentication_tls_certificate_signal_callback_rejected
> 
> Please undocument these. (Now there's something you don't hear every day... :-)
> 
> Changing it to <SUBSECTION Private> would do nicely.

done.

> > + * tp_tls_certificate_rejection_raise_error:
> 
> should also document:
> 
> Returns: %FALSE

added (thanks gtk-doc for not catching this).

> > +_TP_AVAILABLE_IN_0_20
> > +struct _TpTLSCertificateRejectionClass {
> 
> Not portable. In MSVC++, those macros expand to __declspec(deprecated) or
> something, which can only be applied to extern symbols (functions/variables),
> not to types.
> 
> If we really need it, we could add _TP_GNUC_AVAILABLE_IN_0_20 which only does
> anything in gcc but can appear in more places - but we'd have to define it
> ourselves, using G_UNAVAILABLE in gcc >= 4.5 and no expansion otherwise.
> 
> Or, we could just drop it. If you can't call any function that returns one of
> these objects then in practice that'll be enough.

fixed (ameneded).

> +      g_variant_builder_init (&builder, G_VARIANT_TYPE ("a{sv}"));
> +
> +      rej = _tp_tls_certificate_rejection_new (error,
> +          TP_TLS_CERTIFICATE_REJECT_REASON_UNKNOWN, TP_ERROR_STR_CERT_INVALID,
> +          g_variant_builder_end (&builder));
> 
> g_variant_builder_end returns a new floating ref, and I was worried that
> _tp_tls_certificate_rejection_new doesn't seem to sink the floating
> reference... but g_object_new() probably uses a GValue, which sinks floating
> variants automatically.
> 
> This seems a bit subtle, it might be better if
> _tp_tls_certificate_rejection_new explicitly did:
> 
> g_variant_ref_sink (details);
> ret = g_object_new (..., details, ...);
> g_variant_unref (details);
> return ret;
> 
> I think _tp_tls_certificate_rejection_new should certainly document that it
> sinks floating references. (Assuming it does...)

done.

Looks good, let's merge it and go for 0.19.

Merged to master for 0.19.0