Hi Luiz,
śr., 2 maj 2018 o 13:33 Luiz Augusto von Dentz <luiz.dentz@xxxxxxxxx>
napisał(a):
> Hi Grzegorz,
> On Wed, May 2, 2018 at 1:09 PM Grzegorz Kolodziejczyk <
> grzegorz.kolodziejczyk@xxxxxxxxxxx> wrote:
> > This patch adds gatt-server possibility to request authorization from
> > application if needed and previously wasn't authorized. Authorization is
> > requested by sending write request with no data to application.
> > ---
> > doc/gatt-api.txt | 8 ++++++
> > src/gatt-database.c | 6 +++++
> > src/shared/gatt-server.c | 64
> +++++++++++++++++++++++++++++++++++++++++-------
> > 3 files changed, 69 insertions(+), 9 deletions(-)
> > diff --git a/doc/gatt-api.txt b/doc/gatt-api.txt
> > index 0f1cc9029..b9bdc9352 100644
> > --- a/doc/gatt-api.txt
> > +++ b/doc/gatt-api.txt
> > @@ -251,6 +251,14 @@ Properties string UUID [read-only]
> > "secure-read" (Server only)
> > "secure-write" (Server only)
> Documentation changes should be split in a different patch.
> > + boolean Authorized [read-only, optional] (Server only)
> > +
> > + True, if read/write operation was previously
> authorized
> > + and attribute requires authorization. If no
> Authorized
> > + property is available along with characteristic
> or this
> > + property is set to true, then daemon don't send
> empty
> > + write request for authorization.
> I guess we are missing the authorization flag, otherwise the daemon cannot
> tell if authorization is required on not, or we assume it is always
> required if Authorized is set to false? In that case, we should make it
If there is no authorization request then there is no property on client
(chrc_get_authorized_exists checks for prop existence).
There are 3 ways of handling this property:
1) No property - means that writing behaves as before without issuing
authorization request part, only gatt database asks for property existence.
2) Property set to False - means that there is authorization request for
preparing write this property, once authorized it sets to True
3) Property set to True - means that write was previously authorized and
any continous (offset > 0) write can be issued. If write will be issued
with offset = 0, new authorization request will be required.
> clearer in the documentation. Perhaps we should make the property state if
> the authorization is required like boolean Authorization, if it is not
> present it would be assumed to be false meaning authorization is not
> required. We should also state how it affects the calls stating that a
> prepare write would cause WriteValue to be called with empty data if
> authorization is required, and perhaps we should attempt to call
WriteValue
> just once per procedure which means the authorization would be valid until
> execute, or we change the property to be string that can assume the
values:
> always, once, never. Btw, I don't think we can support Authorization with
Every time write request will be issued with offset = 0 - application will
be asked for authorization - that was my assumption.
> AcquireWrite since there is no reply to writes on the pipe, so we might as
> well document that those properties cannot be used together.
Right, I'll update it in next version.
> > Characteristic Descriptors hierarchy
> > ====================================
> > diff --git a/src/gatt-database.c b/src/gatt-database.c
> > index 0ac5b75b0..419db909c 100644
> > --- a/src/gatt-database.c
> > +++ b/src/gatt-database.c
> > @@ -2614,6 +2614,12 @@ static void chrc_write_cb(struct
gatt_db_attribute
> *attrib,
> > goto fail;
> > }
> > + if (offset && g_dbus_proxy_get_property(chrc->proxy,
"Authorized",
> > + &iter))
{
> > + gatt_db_attribute_write_result(attrib, id, 0);
> > + return;
> > + }
> What is the logic behind checking the offset? Is this check here because
> the prepare writes are now passed up the stack? Afaik the offset can be
set
> to 0 even with prepare write which means we should check the opcode
instead
> which might be required for execute as well otherwise nothing that has
> offset set will be ever written to the application.
> > if (chrc->write_io) {
> > if (pipe_io_send(chrc->write_io, value, len) < 0) {
> > error("Unable to write: %s", strerror(errno));
> > diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
> > index 4b554f665..cdade76f8 100644
> > --- a/src/shared/gatt-server.c
> > +++ b/src/shared/gatt-server.c
> > @@ -1208,6 +1208,45 @@ static bool store_prep_data(struct bt_gatt_server
> *server,
> > return prep_data_new(server, handle, offset, length, value);
> > }
> > +struct prep_write_complete_data {
> > + void *pdu;
> > + uint16_t length;
> > + struct bt_gatt_server *server;
> > +};
> > +
> > +static void prep_write_complete_cb(struct gatt_db_attribute *attr, int
> err,
> > + void
> *user_data)
> > +{
> > + struct prep_write_complete_data *pwcd = user_data;
> > + uint16_t handle = 0;
> > + uint16_t offset;
> > +
> > + handle = get_le16(pwcd->pdu);
> > +
> > + if (err) {
> > + bt_att_send_error_rsp(pwcd->server->att,
> > + BT_ATT_OP_PREP_WRITE_REQ,
handle,
> err);
> > + free(pwcd->pdu);
> > + free(pwcd);
> > +
> > + return;
> > + }
> > +
> > + offset = get_le16(pwcd->pdu + 2);
> > +
> > + if (!store_prep_data(pwcd->server, handle, offset, pwcd->length
-
> 4,
> > + &((uint8_t *)
> pwcd->pdu)[4]))
> > + bt_att_send_error_rsp(pwcd->server->att,
> > + BT_ATT_OP_PREP_WRITE_RSP,
handle,
> > +
> BT_ATT_ERROR_INSUFFICIENT_RESOURCES);
> > +
> > + bt_att_send(pwcd->server->att, BT_ATT_OP_PREP_WRITE_RSP,
> pwcd->pdu,
> > + pwcd->length, NULL,
NULL,
> NULL);
> > +
> > + free(pwcd->pdu);
> > + free(pwcd);
> > +}
> > +
> > static void prep_write_cb(uint8_t opcode, const void *pdu,
> > uint16_t length, void
*user_data)
> > {
> > @@ -1215,7 +1254,8 @@ static void prep_write_cb(uint8_t opcode, const
> void *pdu,
> > uint16_t handle = 0;
> > uint16_t offset;
> > struct gatt_db_attribute *attr;
> > - uint8_t ecode;
> > + struct prep_write_complete_data *pwcd;
> > + uint8_t ecode, status;
> > if (length < 4) {
> > ecode = BT_ATT_ERROR_INVALID_PDU;
> > @@ -1245,15 +1285,21 @@ static void prep_write_cb(uint8_t opcode, const
> void *pdu,
> > if (ecode)
> > goto error;
> > - if (!store_prep_data(server, handle, offset, length - 4,
> > - &((uint8_t *) pdu)[4]))
{
> > - ecode = BT_ATT_ERROR_INSUFFICIENT_RESOURCES;
> > - goto error;
> > - }
> > + pwcd = new0(struct prep_write_complete_data, 1);
> > + pwcd->pdu = malloc(length);
> > + memcpy(pwcd->pdu, pdu, length);
> > + pwcd->length = length;
> > + pwcd->server = server;
> > - bt_att_send(server->att, BT_ATT_OP_PREP_WRITE_RSP, pdu, length,
> NULL,
> > - NULL,
> NULL);
> > - return;
> > + status = gatt_db_attribute_write(attr, offset, NULL, 0,
> > +
BT_ATT_OP_PREP_WRITE_REQ,
> > + server->att,
> > + prep_write_complete_cb,
> pwcd);
> > + if (status)
> > + return;
> > +
> > + ecode = BT_ATT_ERROR_UNLIKELY;
> > error:
> > bt_att_send_error_rsp(server->att, opcode, handle, ecode);
> > --
> > 2.13.6
> > --
> > To unsubscribe from this list: send the line "unsubscribe
> linux-bluetooth" in
> > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> --
> Luiz Augusto von Dentz
Grzegorz Kołodziejczyk
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
