[PATCH 1/2] dpp: Add State and Role properties to dbus API

[PATCH 1/2] dpp: Add State and Role properties to dbus API

[Jesse Lentz]

Allow front-end applications to see DPP state (presence, authenticating,
or configuring) and role (enrollee or configurator) via dbus.
---
 src/dpp.c | 110 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 109 insertions(+), 1 deletion(-)

diff --git a/src/dpp.c b/src/dpp.c
index 0102dc58..93e4ca57 100644
--- a/src/dpp.c
+++ b/src/dpp.c
@@ -149,6 +149,58 @@ struct dpp_sm {
 	bool roc_started : 1;
 };
 
+static bool dpp_get_state(struct l_dbus *dbus,
+				struct l_dbus_message *message,
+				struct l_dbus_message_builder *builder,
+				void *user_data)
+{
+	struct dpp_sm *dpp = user_data;
+	const char *state;
+
+	switch (dpp->state) {
+	    case DPP_STATE_PRESENCE:
+		state = "presence";
+		break;
+	    case DPP_STATE_AUTHENTICATING:
+		state = "authenticating";
+		break;
+	    case DPP_STATE_CONFIGURING:
+		state = "configuring";
+		break;
+	    default:
+		return false;
+	}
+
+	l_dbus_message_builder_append_basic(builder, 's', state);
+	return true;
+}
+
+static bool dpp_get_role(struct l_dbus *dbus,
+				struct l_dbus_message *message,
+				struct l_dbus_message_builder *builder,
+				void *user_data)
+{
+	struct dpp_sm *dpp = user_data;
+	const char *role;
+
+	if (dpp->state == DPP_STATE_NOTHING)
+	    return false;
+
+	switch (dpp->role) {
+	    case DPP_CAPABILITY_ENROLLEE:
+		role = "enrollee";
+		break;
+	    case DPP_CAPABILITY_CONFIGURATOR:
+		role = "configurator";
+		break;
+	    default:
+		return false;
+	}
+
+	l_dbus_message_builder_append_basic(builder, 's', role);
+	return true;
+}
+
 static void *dpp_serialize_iovec(struct iovec *iov, size_t iov_len,
 				size_t *out_len)
 {
@@ -253,6 +305,15 @@ static void dpp_reset(struct dpp_sm *dpp)
 	dpp->frame_retry = 0;
 	dpp->frame_cookie = 0;
 
+	l_dbus_property_changed(dbus_get_bus(),
+				netdev_get_path(dpp->netdev),
+				IWD_DPP_INTERFACE,
+				"State");
+	l_dbus_property_changed(dbus_get_bus(),
+				netdev_get_path(dpp->netdev),
+				IWD_DPP_INTERFACE,
+				"Role");
+
 	explicit_bzero(dpp->r_nonce, dpp->nonce_len);
 	explicit_bzero(dpp->i_nonce, dpp->nonce_len);
 	explicit_bzero(dpp->e_nonce, dpp->nonce_len);
@@ -494,6 +555,11 @@ static void dpp_configuration_start(struct dpp_sm *dpp, const uint8_t *addr)
 
 	dpp->state = DPP_STATE_CONFIGURING;
 
+	l_dbus_property_changed(dbus_get_bus(),
+				netdev_get_path(dpp->netdev),
+				IWD_DPP_INTERFACE,
+				"State");
+
 	dpp_send_frame(dpp, iov, 2, dpp->current_freq);
 }
 
@@ -994,6 +1060,11 @@ static void dpp_handle_config_request_frame(const struct mmpdu_header *frame,
 
 	dpp->state = DPP_STATE_CONFIGURING;
 
+	l_dbus_property_changed(dbus_get_bus(),
+				netdev_get_path(dpp->netdev),
+				IWD_DPP_INTERFACE,
+				"State");
+
 	dpp_send_config_response(dpp, DPP_STATUS_OK);
 
 	return;
@@ -1286,6 +1357,11 @@ static void authenticate_confirm(struct dpp_sm *dpp, const uint8_t *from,
 auth_confirm_failed:
 	dpp->state = DPP_STATE_PRESENCE;
 	dpp_free_auth_data(dpp);
+
+	l_dbus_property_changed(dbus_get_bus(),
+				netdev_get_path(dpp->netdev),
+				IWD_DPP_INTERFACE,
+				"State");
 }
 
 static void dpp_auth_request_failed(struct dpp_sm *dpp,
@@ -1750,6 +1826,11 @@ static void authenticate_request(struct dpp_sm *dpp, const uint8_t *from,
 	dpp->state = DPP_STATE_AUTHENTICATING;
 	dpp_reset_protocol_timer(dpp);
 
+	l_dbus_property_changed(dbus_get_bus(),
+				netdev_get_path(dpp->netdev),
+				IWD_DPP_INTERFACE,
+				"State");
+
 	/* Don't send if the frequency is changing */
 	if (!dpp->new_freq)
 		send_authenticate_response(dpp);
@@ -1757,7 +1838,6 @@ static void authenticate_request(struct dpp_sm *dpp, const uint8_t *from,
 	return;
 
 auth_request_failed:
-	dpp->state = DPP_STATE_PRESENCE;
 	dpp_free_auth_data(dpp);
 }
 
@@ -2040,6 +2120,11 @@ static void dpp_handle_presence_announcement(struct dpp_sm *dpp,
 
 	dpp->state = DPP_STATE_AUTHENTICATING;
 
+	l_dbus_property_changed(dbus_get_bus(),
+				netdev_get_path(dpp->netdev),
+				IWD_DPP_INTERFACE,
+				"State");
+
 	if (!dpp_send_authenticate_request(dpp))
 		return;
 
@@ -2416,6 +2501,15 @@ static struct l_dbus_message *dpp_dbus_start_enrollee(struct l_dbus *dbus,
 	dpp->state = DPP_STATE_PRESENCE;
 	dpp->role = DPP_CAPABILITY_ENROLLEE;
 
+	l_dbus_property_changed(dbus_get_bus(),
+				netdev_get_path(dpp->netdev),
+				IWD_DPP_INTERFACE,
+				"State");
+	l_dbus_property_changed(dbus_get_bus(),
+				netdev_get_path(dpp->netdev),
+				IWD_DPP_INTERFACE,
+				"Role");
+
 	l_ecdh_generate_key_pair(dpp->curve, &dpp->proto_private,
 					&dpp->own_proto_public);
 
@@ -2560,6 +2654,15 @@ static struct l_dbus_message *dpp_start_configurator_common(
 						network_get_ssid(network),
 						hs->akm_suite);
 
+	l_dbus_property_changed(dbus_get_bus(),
+				netdev_get_path(dpp->netdev),
+				IWD_DPP_INTERFACE,
+				"State");
+	l_dbus_property_changed(dbus_get_bus(),
+				netdev_get_path(dpp->netdev),
+				IWD_DPP_INTERFACE,
+				"Role");
+
 	scan_periodic_stop(dpp->wdev_id);
 
 	l_debug("DPP Start Configurator: %s", dpp->uri);
@@ -2606,6 +2709,11 @@ static void dpp_setup_interface(struct l_dbus_interface *interface)
 				dpp_dbus_configure_enrollee, "", "s", "uri");
 	l_dbus_interface_method(interface, "Stop", 0,
 				dpp_dbus_stop, "", "");
+
+	l_dbus_interface_property(interface, "State", 0, "s",
+				  dpp_get_state, NULL);
+	l_dbus_interface_property(interface, "Role", 0, "s",
+				  dpp_get_role, NULL);
 }
 
 static void dpp_destroy_interface(void *user_data)
-- 
2.36.1

[PATCH 2/2] doc: Add State and Role properties to DPP API

[Jesse Lentz]

Document the State and Role properties of the DeviceProvisioning API.
---
 doc/device-provisioning-api.txt | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/doc/device-provisioning-api.txt b/doc/device-provisioning-api.txt
index 0aba2557..c1d706ec 100644
--- a/doc/device-provisioning-api.txt
+++ b/doc/device-provisioning-api.txt
@@ -56,3 +56,13 @@ Methods		string StartEnrollee()
 						net.connman.iwd.NotConfigured
 						net.connman.iwd.NotSupported
 						net.connman.iwd.Busy
+
+Properties	string State [readonly, optional]
+
+			Reflects the DPP state. Possible values are "presence",
+			"authenticating", and "configuring".
+
+		string Role [readonly, optional]
+
+			Reflects the DPP role. Possible values are "enrollee"
+			and "configurator"
-- 
2.36.1

Re: [PATCH 2/2] doc: Add State and Role properties to DPP API

[James Prestwood]

Hi Jesse,

On Wed, 2022-06-22 at 15:41 -0400, Jesse Lentz wrote:
> Document the State and Role properties of the DeviceProvisioning API.
> ---
>  doc/device-provisioning-api.txt | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/doc/device-provisioning-api.txt b/doc/device-
> provisioning-api.txt
> index 0aba2557..c1d706ec 100644
> --- a/doc/device-provisioning-api.txt
> +++ b/doc/device-provisioning-api.txt
> @@ -56,3 +56,13 @@ Methods              string StartEnrollee()
>                                                 net.connman.iwd.NotCo
> nfigured
>                                                 net.connman.iwd.NotSu
> pported
>                                                 net.connman.iwd.Busy
> +
> +Properties     string State [readonly, optional]
> +
> +                       Reflects the DPP state. Possible values are
> "presence",
> +                       "authenticating", and "configuring".

I think a "stopped" state would also be needed.

> +
> +               string Role [readonly, optional]
> +
> +                       Reflects the DPP role. Possible values are
> "enrollee"
> +                       and "configurator"

I don't think you actually need this one. The role should be implied by
however you started DPP:

StartEnrollee() -> enrollee
StartConfigurator() -> configurator
ConfigureEnrollee() -> configurator

Thanks,
James

Re: [PATCH 2/2] doc: Add State and Role properties to DPP API

[Jesse Lentz]

Hi James,

Thanks for your feedback.

> > +Properties     string State [readonly, optional]
> > +
> > +                       Reflects the DPP state. Possible values are
> > "presence",
> > +                       "authenticating", and "configuring".
>
> I think a "stopped" state would also be needed.

As a user of the API, I can infer that DPP is inactive if "State" is
undefined. A non-optional State property with an explicit "inactive"
value should provide equivalent information; would you prefer the latter
approach?

> > +               string Role [readonly, optional]
> > +
> > +                       Reflects the DPP role. Possible values are
> > "enrollee"
> > +                       and "configurator"
>
> I don't think you actually need this one. The role should be implied by
> however you started DPP:
>
> StartEnrollee() -> enrollee
> StartConfigurator() -> configurator
> ConfigureEnrollee() -> configurator

One use case that I'm trying to address is when a user launches the
front-end app while DPP is already active. There are two ways that I can
think of to handle this:

1) The front-end stops and re-starts DPP if the user requests it. This
only requires iwd to provide State, so that the front-end can see if a
previous invocation must be stopped.

2) iwd provides sufficient information via properties to restore the
previous invocation: State, Role, and URI.

I originally had option 2 in mind, but if DPP invocations are intended
to be transient, then perhaps option 1 would be more appropriate. Let me
know what your thoughts are, and I'll send a revised patch.

Thanks.

Sincerely,
Jesse

Re: [PATCH 2/2] doc: Add State and Role properties to DPP API

[James Prestwood]

Hi Jesse,

On Thu, 2022-06-23 at 12:30 -0400, Jesse Lentz wrote:
> Hi James,
> 
> Thanks for your feedback.
> 
> > > +Properties     string State [readonly, optional]
> > > +
> > > +                       Reflects the DPP state. Possible values
> > > are
> > > "presence",
> > > +                       "authenticating", and "configuring".
> > 
> > I think a "stopped" state would also be needed.
> 
> As a user of the API, I can infer that DPP is inactive if "State" is
> undefined. A non-optional State property with an explicit "inactive"
> value should provide equivalent information; would you prefer the
> latter
> approach?

Yeah, I think having an explicit value is best. I think it makes it
easier to handle for the consumer of the API.

> 
> > > +               string Role [readonly, optional]
> > > +
> > > +                       Reflects the DPP role. Possible values
> > > are
> > > "enrollee"
> > > +                       and "configurator"
> > 
> > I don't think you actually need this one. The role should be
> > implied by
> > however you started DPP:
> > 
> > StartEnrollee() -> enrollee
> > StartConfigurator() -> configurator
> > ConfigureEnrollee() -> configurator
> 
> One use case that I'm trying to address is when a user launches the
> front-end app while DPP is already active. There are two ways that I
> can
> think of to handle this:

Ah, ok this is fine then. Having the property would be the simplest way
to handle this case.

> 
> 1) The front-end stops and re-starts DPP if the user requests it.
> This
> only requires iwd to provide State, so that the front-end can see if
> a
> previous invocation must be stopped.
> 
> 2) iwd provides sufficient information via properties to restore the
> previous invocation: State, Role, and URI.
> 
> I originally had option 2 in mind, but if DPP invocations are
> intended
> to be transient, then perhaps option 1 would be more appropriate. Let
> me
> know what your thoughts are, and I'll send a revised patch.

Out of curiosity do you need these properties purely for displaying to
the user? If so I wonder if they shoud go on a new interface, e.g.
DeviceProvisioningDiagnostics.

We have a similar concept with AccessPointDiagnostics and
StationDiagnostics. These interfaces hold extra information that isn't
really needed to use IWD generally, but more debugging or displaying
information in a UI. These seem to fit more into that category than
needed for DPP in general.

> 
> Thanks.
> 
> Sincerely,
> Jesse

Re: [PATCH 2/2] doc: Add State and Role properties to DPP API

[Jesse Lentz]

James,

> Yeah, I think having an explicit value is best. I think it makes it
> easier to handle for the consumer of the API.

Sounds good.

> > 1) The front-end stops and re-starts DPP if the user requests it.
> > This
> > only requires iwd to provide State, so that the front-end can see if
> > a
> > previous invocation must be stopped.
> > 
> > 2) iwd provides sufficient information via properties to restore the
> > previous invocation: State, Role, and URI.
> > 
> > I originally had option 2 in mind, but if DPP invocations are
> > intended
> > to be transient, then perhaps option 1 would be more appropriate. Let
> > me
> > know what your thoughts are, and I'll send a revised patch.
>
> Out of curiosity do you need these properties purely for displaying to
> the user? If so I wonder if they shoud go on a new interface, e.g.
> DeviceProvisioningDiagnostics.
>
> We have a similar concept with AccessPointDiagnostics and
> StationDiagnostics. These interfaces hold extra information that isn't
> really needed to use IWD generally, but more debugging or displaying
> information in a UI. These seem to fit more into that category than
> needed for DPP in general.

The uses that I have in mind are:
1) Notify the user when a device is being configured (by monitoring
   State)
2) Determine whether to show the user a "Start" button or a "Stop"
   button.
3) Display a QR code and a label showing the user whether credentials
   are being given or received.

The benefit of properties over method return values for (2) and (3) is
that the front-end app gets to be stateless, so that the user could quit
and re-start the app and still see the QR code.

Thanks, and let me know what your thoughts are.

Sincerely,
Jesse

Re: [PATCH 2/2] doc: Add State and Role properties to DPP API

[Denis Kenzior]

Hi Jesse,

On 6/23/22 11:30, Jesse Lentz wrote:
> Hi James,
> 
> Thanks for your feedback.
> 
>>> +Properties     string State [readonly, optional]
>>> +
>>> +                       Reflects the DPP state. Possible values are
>>> "presence",
>>> +                       "authenticating", and "configuring".
>>
>> I think a "stopped" state would also be needed.
> 
> As a user of the API, I can infer that DPP is inactive if "State" is
> undefined. A non-optional State property with an explicit "inactive"
> value should provide equivalent information; would you prefer the latter
> approach?

Inferring the state is fine if you can do this reliably.  One problem with 
optional properties is that once they transition from 'defined' -> 'undefined' 
there is no PropertiesChanged signal that is sent (or rather you have to look 
into the invalidated_properties array of that signal).

We have been designing our APIs with the assumption that many bindings wouldn't 
be using this invalidated_properties reliably and added another point of reference.

For example, Station.ConnectedNetwork is invalidated when Station.State goes 
into the 'disconnected' state and thus not explicitly 'changed'.  But the DBus 
client can infer that Station.ConnectedNetwork is 'undefined' at that point.

But maybe bindings have improved over the years and we should just rely on 
invalidated_properties being taken into account properly?  If so, that would be 
a good argument to make 'State' optional.

Regards,
-Denis

Re: [PATCH 2/2] doc: Add State and Role properties to DPP API

[Jesse Lentz]

Hi Denis,

> Inferring the state is fine if you can do this reliably.  One problem with 
> optional properties is that once they transition from 'defined' -> 'undefined' 
> there is no PropertiesChanged signal that is sent (or rather you have to look 
> into the invalidated_properties array of that signal).
>
> We have been designing our APIs with the assumption that many bindings wouldn't 
> be using this invalidated_properties reliably and added another point of reference.
>
> For example, Station.ConnectedNetwork is invalidated when Station.State goes 
> into the 'disconnected' state and thus not explicitly 'changed'.  But the DBus 
> client can infer that Station.ConnectedNetwork is 'undefined' at that point.
>
> But maybe bindings have improved over the years and we should just rely on 
> invalidated_properties being taken into account properly?  If so, that would be 
> a good argument to make 'State' optional.

Hmm, I wasn't aware of that. I can only comment on the GLib binding,
which works reliably as far as I can tell - it supplies both changed and
invalidated properties to the user-registered callback, which gets
called in response to both changed and to invalidated properties.

But if other bindings handle it incorrectly, then I suppose there's no
compelling reason to make it an optional property.

Thanks.

Sincerely,
Jesse

Re: [PATCH 2/2] doc: Add State and Role properties to DPP API

[Denis Kenzior]

Hi Jesse,

On 6/23/22 14:07, Jesse Lentz wrote:
> Hi Denis,
> 
>> Inferring the state is fine if you can do this reliably.  One problem with
>> optional properties is that once they transition from 'defined' -> 'undefined'
>> there is no PropertiesChanged signal that is sent (or rather you have to look
>> into the invalidated_properties array of that signal).
>>
>> We have been designing our APIs with the assumption that many bindings wouldn't
>> be using this invalidated_properties reliably and added another point of reference.
>>
>> For example, Station.ConnectedNetwork is invalidated when Station.State goes
>> into the 'disconnected' state and thus not explicitly 'changed'.  But the DBus
>> client can infer that Station.ConnectedNetwork is 'undefined' at that point.
>>
>> But maybe bindings have improved over the years and we should just rely on
>> invalidated_properties being taken into account properly?  If so, that would be
>> a good argument to make 'State' optional.
> 
> Hmm, I wasn't aware of that. I can only comment on the GLib binding,
> which works reliably as far as I can tell - it supplies both changed and
> invalidated properties to the user-registered callback, which gets
> called in response to both changed and to invalidated properties.
> 

Ok, good to know that this works well.

> But if other bindings handle it incorrectly, then I suppose there's no
> compelling reason to make it an optional property.

Maybe I shouldn't blame the bindings as much as the documentation.  This wasn't 
a widely explained feature at the time we started, and this project is getting 
pretty old.

But yes, maybe it is easier to just make this property mandatory while all 
others can be made optional.

Regards,
-Denis