Bug 30460

Summary: Generate client side API for Chan.T.ServerTLSConnection and TLSCertificate
Product: Telepathy Reporter: Guillaume Desmottes <guillaume.desmottes>
Component: tp-glibAssignee: Simon McVittie <smcv>
Status: RESOLVED FIXED QA Contact: Telepathy bugs list <telepathy-bugs>
Severity: enhancement    
Priority: medium Keywords: patch
Version: unspecified   
Hardware: Other   
OS: All   
URL: http://cgit.collabora.com/git/user/cassidy/telepathy-glib/log/?h=tls-30460
Whiteboard: review+
i915 platform: i915 features:

Description Guillaume Desmottes 2010-09-29 05:13:41 UTC
Currently we don't generate client side API for those because TLSCertificate requiers a new TpProxy subclass. We should add one (probably by stealing Empathy's one) and generate the API.
Comment 1 Guillaume Desmottes 2010-09-29 05:15:27 UTC
This blocks https://bugzilla.gnome.org/show_bug.cgi?id=630896
Comment 2 Simon McVittie 2010-11-26 06:04:36 UTC
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.)
Comment 3 Simon McVittie 2010-12-15 05:19:41 UTC
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.
Comment 4 Guillaume Desmottes 2012-04-11 02:41:04 UTC
What's blocking this? Just the tests?
Comment 5 Simon McVittie 2012-04-11 03:26:00 UTC
(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.
Comment 6 Guillaume Desmottes 2012-04-18 02:28:21 UTC
On it.
Comment 7 Guillaume Desmottes 2012-04-18 04:31:37 UTC
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.
Comment 8 Guillaume Desmottes 2012-04-18 06:49:18 UTC
(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?
Comment 9 Simon McVittie 2012-04-18 07:17:48 UTC
(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:
        # ...
Comment 10 Simon McVittie 2012-04-18 07:18:28 UTC
(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?
Comment 11 Guillaume Desmottes 2012-04-19 01:32:11 UTC
(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.
Comment 12 Guillaume Desmottes 2012-04-19 03:51:37 UTC
(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.
Comment 13 Guillaume Desmottes 2012-04-19 07:15:32 UTC
(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.
Comment 14 Guillaume Desmottes 2012-04-23 01:34:34 UTC
(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.
Comment 15 Guillaume Desmottes 2012-05-02 06:17:13 UTC
(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.
Comment 16 Simon McVittie 2012-05-02 07:12:33 UTC
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.
Comment 17 Simon McVittie 2012-05-02 07:13:15 UTC
(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.
Comment 18 Guillaume Desmottes 2012-05-03 01:21:38 UTC
(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.
Comment 19 Guillaume Desmottes 2012-05-03 05:35:38 UTC
I rebased the branch on top of master and added 2 extra commits.
Comment 20 Simon McVittie 2012-05-03 08:44:42 UTC
> +<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...)
Comment 21 Guillaume Desmottes 2012-05-04 00:45:11 UTC
(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.
Comment 22 Simon McVittie 2012-05-09 03:08:38 UTC
Looks good, let's merge it and go for 0.19.
Comment 23 Guillaume Desmottes 2012-05-09 03:16:10 UTC
Merged to master for 0.19.0

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.