* [RFC 0/2] Add support for prepare writes authorization @ 2018-05-02 9:17 Grzegorz Kolodziejczyk 2018-05-02 9:17 ` [RFC 1/2] shared/gatt-server: Request authorization for prepare writes Grzegorz Kolodziejczyk 2018-05-02 9:17 ` [RFC 2/2] client: Add authorized property handling to characteristic attribute Grzegorz Kolodziejczyk 0 siblings, 2 replies; 10+ messages in thread From: Grzegorz Kolodziejczyk @ 2018-05-02 9:17 UTC (permalink / raw) To: linux-bluetooth This patch set adds support for prepare writes authorization handling. This case covers "GATT/SR/GAW/BI-11-C [Write Long Characteristic Value – Insufficient Authorization]" PTS test case. If authorization property doesn't exist or is set to true then daemon doesn't bother application database with authorization ask and behaves as before. This patch set is on top of: 72c0c310fd2940d0989af2371728e743894ffcbc (client: Define maximum attribute value length as initial value) Grzegorz Kolodziejczyk (2): shared/gatt-server: Request authorization for prepare writes. client: Add authorized property handling to characteristic attribute client/gatt.c | 46 ++++++++++++++++++++++++++++++++++ doc/gatt-api.txt | 8 ++++++ src/gatt-database.c | 6 +++++ src/shared/gatt-server.c | 64 +++++++++++++++++++++++++++++++++++++++++------- 4 files changed, 115 insertions(+), 9 deletions(-) -- 2.13.6 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [RFC 1/2] shared/gatt-server: Request authorization for prepare writes. 2018-05-02 9:17 [RFC 0/2] Add support for prepare writes authorization Grzegorz Kolodziejczyk @ 2018-05-02 9:17 ` Grzegorz Kolodziejczyk 2018-05-02 11:32 ` Luiz Augusto von Dentz 2018-05-02 9:17 ` [RFC 2/2] client: Add authorized property handling to characteristic attribute Grzegorz Kolodziejczyk 1 sibling, 1 reply; 10+ messages in thread From: Grzegorz Kolodziejczyk @ 2018-05-02 9:17 UTC (permalink / raw) To: linux-bluetooth 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) + 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. + 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; + } + 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [RFC 1/2] shared/gatt-server: Request authorization for prepare writes. 2018-05-02 9:17 ` [RFC 1/2] shared/gatt-server: Request authorization for prepare writes Grzegorz Kolodziejczyk @ 2018-05-02 11:32 ` Luiz Augusto von Dentz 2018-05-02 13:31 ` Grzegorz Kołodziejczyk 0 siblings, 1 reply; 10+ messages in thread From: Luiz Augusto von Dentz @ 2018-05-02 11:32 UTC (permalink / raw) To: Grzegorz Kolodziejczyk; +Cc: linux-bluetooth Hi Grzegorz, On Wed, May 2, 2018 at 1:09 PM Grzegorz Kolodziejczyk < grzegorz.kolodziejczyk@codecoup.pl> 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 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 AcquireWrite since there is no reply to writes on the pipe, so we might as well document that those properties cannot be used together. > 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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC 1/2] shared/gatt-server: Request authorization for prepare writes. 2018-05-02 11:32 ` Luiz Augusto von Dentz @ 2018-05-02 13:31 ` Grzegorz Kołodziejczyk 2018-05-03 9:54 ` Luiz Augusto von Dentz 0 siblings, 1 reply; 10+ messages in thread From: Grzegorz Kołodziejczyk @ 2018-05-02 13:31 UTC (permalink / raw) To: Luiz Augusto von Dentz; +Cc: linux-bluetooth Hi Luiz, =C5=9Br., 2 maj 2018 o 13:33 Luiz Augusto von Dentz <luiz.dentz@gmail.com> napisa=C5=82(a): > Hi Grzegorz, > On Wed, May 2, 2018 at 1:09 PM Grzegorz Kolodziejczyk < > grzegorz.kolodziejczyk@codecoup.pl> wrote: > > This patch adds gatt-server possibility to request authorization from > > application if needed and previously wasn't authorized. Authorization i= s > > 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 canno= t > 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 =3D 0, new authorization request will be required. > clearer in the documentation. Perhaps we should make the property state i= f > 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 unti= l > 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 =3D 0 - application wil= l be asked for authorization - that was my assumption. > AcquireWrite since there is no reply to writes on the pipe, so we might a= s > well document that those properties cannot be used together. Right, I'll update it in next version. > > Characteristic Descriptors hierarchy > > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > > 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_serve= r > *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 =3D user_data; > > + uint16_t handle =3D 0; > > + uint16_t offset; > > + > > + handle =3D 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 =3D 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 =3D 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 =3D 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 =3D BT_ATT_ERROR_INSUFFICIENT_RESOURCES; > > - goto error; > > - } > > + pwcd =3D new0(struct prep_write_complete_data, 1); > > + pwcd->pdu =3D malloc(length); > > + memcpy(pwcd->pdu, pdu, length); > > + pwcd->length =3D length; > > + pwcd->server =3D server; > > - bt_att_send(server->att, BT_ATT_OP_PREP_WRITE_RSP, pdu, length, > NULL, > > - NULL, > NULL); > > - return; > > + status =3D 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 =3D 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@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- > Luiz Augusto von Dentz Grzegorz Ko=C5=82odziejczyk ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC 1/2] shared/gatt-server: Request authorization for prepare writes. 2018-05-02 13:31 ` Grzegorz Kołodziejczyk @ 2018-05-03 9:54 ` Luiz Augusto von Dentz 0 siblings, 0 replies; 10+ messages in thread From: Luiz Augusto von Dentz @ 2018-05-03 9:54 UTC (permalink / raw) To: Grzegorz Kolodziejczyk; +Cc: linux-bluetooth Hi Grzegorz, On Wed, May 2, 2018 at 4:31 PM Grzegorz Ko=C5=82odziejczyk < grzegorz.kolodziejczyk@codecoup.pl> wrote: > Hi Luiz, > =C5=9Br., 2 maj 2018 o 13:33 Luiz Augusto von Dentz <luiz.dentz@gmail.com= > > napisa=C5=82(a): > > Hi Grzegorz, > > On Wed, May 2, 2018 at 1:09 PM Grzegorz Kolodziejczyk < > > grzegorz.kolodziejczyk@codecoup.pl> 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 =3D 0, new authorization request will be required. I don't quite follow why we want the property to transit states? Ins't it simpler to just check the reply of WriteValue and then just continue? > > 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 =3D 0 - application w= ill > 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. Did you actually read about the suggestion above, I guess that will get us the same result without the application having to change the value of the property, which btw I don't think works with concurrent devices since Authorized would mean it is authorized for every device while we should track the authorization per prepare write. > > > Characteristic Descriptors hierarchy > > > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > > > 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 becaus= e > > 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. You haven't ack or nack this comment. > > > 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 =3D user_data; > > > + uint16_t handle =3D 0; > > > + uint16_t offset; > > > + > > > + handle =3D 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 =3D 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 =3D 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 =3D 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 =3D BT_ATT_ERROR_INSUFFICIENT_RESOURCES; > > > - goto error; > > > - } > > > + pwcd =3D new0(struct prep_write_complete_data, 1); > > > + pwcd->pdu =3D malloc(length); > > > + memcpy(pwcd->pdu, pdu, length); > > > + pwcd->length =3D length; > > > + pwcd->server =3D server; > > > - bt_att_send(server->att, BT_ATT_OP_PREP_WRITE_RSP, pdu, length, > > NULL, > > > - NULL, > > NULL); > > > - return; > > > + status =3D 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 =3D 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@vger.kernel.org > > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- > > Luiz Augusto von Dentz > Grzegorz Ko=C5=82odziejczyk --=20 Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 10+ messages in thread
* [RFC 2/2] client: Add authorized property handling to characteristic attribute 2018-05-02 9:17 [RFC 0/2] Add support for prepare writes authorization Grzegorz Kolodziejczyk 2018-05-02 9:17 ` [RFC 1/2] shared/gatt-server: Request authorization for prepare writes Grzegorz Kolodziejczyk @ 2018-05-02 9:17 ` Grzegorz Kolodziejczyk 2018-05-02 10:42 ` Luiz Augusto von Dentz 1 sibling, 1 reply; 10+ messages in thread From: Grzegorz Kolodziejczyk @ 2018-05-02 9:17 UTC (permalink / raw) To: linux-bluetooth This patch adds handling of authorized property to bluetoothctl. --- client/gatt.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/client/gatt.c b/client/gatt.c index 6bf644c48..fffa86851 100644 --- a/client/gatt.c +++ b/client/gatt.c @@ -1432,6 +1432,30 @@ static gboolean chrc_notify_acquired_exists(const GDBusPropertyTable *property, return FALSE; } +static gboolean chrc_get_authorized(const GDBusPropertyTable *property, + DBusMessageIter *iter, void *data) +{ + struct chrc *chrc = data; + dbus_bool_t value; + + value = chrc->authorized ? TRUE : FALSE; + + dbus_message_iter_append_basic(iter, DBUS_TYPE_BOOLEAN, &value); + + return TRUE; +} + +static gboolean chrc_get_authorized_exists(const GDBusPropertyTable *property, + void *data) +{ + struct chrc *chrc = data; + + if (chrc->authorization_req) + return TRUE; + + return FALSE; +} + static const GDBusPropertyTable chrc_properties[] = { { "UUID", "s", chrc_get_uuid, NULL, NULL }, { "Service", "o", chrc_get_service, NULL, NULL }, @@ -1442,6 +1466,8 @@ static const GDBusPropertyTable chrc_properties[] = { chrc_write_acquired_exists }, { "NotifyAcquired", "b", chrc_get_notify_acquired, NULL, chrc_notify_acquired_exists }, + { "Authorized", "b", chrc_get_authorized, NULL, + chrc_get_authorized_exists }, { } }; @@ -1665,6 +1691,15 @@ static void authorize_write_response(const char *input, void *user_data) chrc->authorized = true; + /* Authorization check of prepare writes */ + if (!chrc->value_len) { + reply = g_dbus_create_reply(pending_message, DBUS_TYPE_INVALID); + g_dbus_send_message(aad->conn, reply); + g_free(aad); + + return; + } + errsv = parse_value_arg(&iter, &chrc->value, &chrc->value_len, chrc->max_val_len); if (errsv == -EINVAL) { @@ -1701,8 +1736,16 @@ static DBusMessage *chrc_write_value(DBusConnection *conn, DBusMessage *msg, DBusMessageIter iter; char *str; int errsv; + uint16_t offset = 0; dbus_message_iter_init(msg, &iter); + /* Get offset only (second parameter) */ + dbus_message_iter_next(&iter); + + parse_options(&iter, &offset, NULL, NULL, NULL); + + if (chrc->authorization_req && offset == 0) + chrc->authorized = false; if (chrc->authorization_req && !chrc->authorized) { struct authorize_attribute_data *aad; @@ -1722,6 +1765,9 @@ static DBusMessage *chrc_write_value(DBusConnection *conn, DBusMessage *msg, return NULL; } + /* Rewind to value parameter */ + dbus_message_iter_init(msg, &iter); + errsv = parse_value_arg(&iter, &chrc->value, &chrc->value_len, chrc->max_val_len); if (errsv == -EINVAL) { -- 2.13.6 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [RFC 2/2] client: Add authorized property handling to characteristic attribute 2018-05-02 9:17 ` [RFC 2/2] client: Add authorized property handling to characteristic attribute Grzegorz Kolodziejczyk @ 2018-05-02 10:42 ` Luiz Augusto von Dentz 2018-05-02 13:06 ` Grzegorz Kołodziejczyk 0 siblings, 1 reply; 10+ messages in thread From: Luiz Augusto von Dentz @ 2018-05-02 10:42 UTC (permalink / raw) To: Grzegorz Kolodziejczyk; +Cc: linux-bluetooth Hi Grzegorz, On Wed, May 2, 2018 at 1:09 PM Grzegorz Kolodziejczyk < grzegorz.kolodziejczyk@codecoup.pl> wrote: > This patch adds handling of authorized property to bluetoothctl. > --- > client/gatt.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 46 insertions(+) > diff --git a/client/gatt.c b/client/gatt.c > index 6bf644c48..fffa86851 100644 > --- a/client/gatt.c > +++ b/client/gatt.c > @@ -1432,6 +1432,30 @@ static gboolean chrc_notify_acquired_exists(const GDBusPropertyTable *property, > return FALSE; > } > +static gboolean chrc_get_authorized(const GDBusPropertyTable *property, > + DBusMessageIter *iter, void *data) > +{ > + struct chrc *chrc = data; > + dbus_bool_t value; > + > + value = chrc->authorized ? TRUE : FALSE; > + > + dbus_message_iter_append_basic(iter, DBUS_TYPE_BOOLEAN, &value); > + > + return TRUE; > +} > + > +static gboolean chrc_get_authorized_exists(const GDBusPropertyTable *property, > + void *data) > +{ > + struct chrc *chrc = data; > + > + if (chrc->authorization_req) > + return TRUE; > + > + return FALSE; > +} > + > static const GDBusPropertyTable chrc_properties[] = { > { "UUID", "s", chrc_get_uuid, NULL, NULL }, > { "Service", "o", chrc_get_service, NULL, NULL }, > @@ -1442,6 +1466,8 @@ static const GDBusPropertyTable chrc_properties[] = { > chrc_write_acquired_exists }, > { "NotifyAcquired", "b", chrc_get_notify_acquired, NULL, > chrc_notify_acquired_exists }, > + { "Authorized", "b", chrc_get_authorized, NULL, > + chrc_get_authorized_exists }, > { } > }; > @@ -1665,6 +1691,15 @@ static void authorize_write_response(const char *input, void *user_data) > chrc->authorized = true; > + /* Authorization check of prepare writes */ > + if (!chrc->value_len) { > + reply = g_dbus_create_reply(pending_message, DBUS_TYPE_INVALID); > + g_dbus_send_message(aad->conn, reply); > + g_free(aad); > + > + return; > + } > + > errsv = parse_value_arg(&iter, &chrc->value, &chrc->value_len, chrc->max_val_len); > if (errsv == -EINVAL) { > @@ -1701,8 +1736,16 @@ static DBusMessage *chrc_write_value(DBusConnection *conn, DBusMessage *msg, > DBusMessageIter iter; > char *str; > int errsv; > + uint16_t offset = 0; > dbus_message_iter_init(msg, &iter); > + /* Get offset only (second parameter) */ > + dbus_message_iter_next(&iter); > + > + parse_options(&iter, &offset, NULL, NULL, NULL); > + > + if (chrc->authorization_req && offset == 0) > + chrc->authorized = false; > if (chrc->authorization_req && !chrc->authorized) { > struct authorize_attribute_data *aad; > @@ -1722,6 +1765,9 @@ static DBusMessage *chrc_write_value(DBusConnection *conn, DBusMessage *msg, > return NULL; > } > + /* Rewind to value parameter */ > + dbus_message_iter_init(msg, &iter); I would prefer to make parse_value_arg parse the offset as well so we don't have to reinit the iter. > errsv = parse_value_arg(&iter, &chrc->value, &chrc->value_len, chrc->max_val_len); > if (errsv == -EINVAL) { > -- > 2.13.6 > -- > To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC 2/2] client: Add authorized property handling to characteristic attribute 2018-05-02 10:42 ` Luiz Augusto von Dentz @ 2018-05-02 13:06 ` Grzegorz Kołodziejczyk 2018-05-03 9:58 ` Luiz Augusto von Dentz 0 siblings, 1 reply; 10+ messages in thread From: Grzegorz Kołodziejczyk @ 2018-05-02 13:06 UTC (permalink / raw) To: Luiz Augusto von Dentz; +Cc: linux-bluetooth Hi Luiz, =C5=9Br., 2 maj 2018 o 12:42 Luiz Augusto von Dentz <luiz.dentz@gmail.com> napisa=C5=82(a): > Hi Grzegorz, > On Wed, May 2, 2018 at 1:09 PM Grzegorz Kolodziejczyk < > grzegorz.kolodziejczyk@codecoup.pl> wrote: > > This patch adds handling of authorized property to bluetoothctl. > > --- > > client/gatt.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 46 insertions(+) > > diff --git a/client/gatt.c b/client/gatt.c > > index 6bf644c48..fffa86851 100644 > > --- a/client/gatt.c > > +++ b/client/gatt.c > > @@ -1432,6 +1432,30 @@ static gboolean chrc_notify_acquired_exists(cons= t > GDBusPropertyTable *property, > > return FALSE; > > } > > +static gboolean chrc_get_authorized(const GDBusPropertyTable *property= , > > + DBusMessageIter *iter, void *data) > > +{ > > + struct chrc *chrc =3D data; > > + dbus_bool_t value; > > + > > + value =3D chrc->authorized ? TRUE : FALSE; > > + > > + dbus_message_iter_append_basic(iter, DBUS_TYPE_BOOLEAN, &value)= ; > > + > > + return TRUE; > > +} > > + > > +static gboolean chrc_get_authorized_exists(const GDBusPropertyTable > *property, > > + void > *data) > > +{ > > + struct chrc *chrc =3D data; > > + > > + if (chrc->authorization_req) > > + return TRUE; > > + > > + return FALSE; > > +} > > + > > static const GDBusPropertyTable chrc_properties[] =3D { > > { "UUID", "s", chrc_get_uuid, NULL, NULL }, > > { "Service", "o", chrc_get_service, NULL, NULL }, > > @@ -1442,6 +1466,8 @@ static const GDBusPropertyTable chrc_properties[] =3D > { > > chrc_write_acquired_exists }, > > { "NotifyAcquired", "b", chrc_get_notify_acquired, NULL, > > chrc_notify_acquired_exists }, > > + { "Authorized", "b", chrc_get_authorized, NULL, > > + > chrc_get_authorized_exists }, > > { } > > }; > > @@ -1665,6 +1691,15 @@ static void authorize_write_response(const char > *input, void *user_data) > > chrc->authorized =3D true; > > + /* Authorization check of prepare writes */ > > + if (!chrc->value_len) { > > + reply =3D g_dbus_create_reply(pending_message, > DBUS_TYPE_INVALID); > > + g_dbus_send_message(aad->conn, reply); > > + g_free(aad); > > + > > + return; > > + } > > + > > errsv =3D parse_value_arg(&iter, &chrc->value, &chrc->value_le= n, > chrc->max_val_len); > > if (errsv =3D=3D -EINVAL) { > > @@ -1701,8 +1736,16 @@ static DBusMessage > *chrc_write_value(DBusConnection *conn, DBusMessage *msg, > > DBusMessageIter iter; > > char *str; > > int errsv; > > + uint16_t offset =3D 0; > > dbus_message_iter_init(msg, &iter); > > + /* Get offset only (second parameter) */ > > + dbus_message_iter_next(&iter); > > + > > + parse_options(&iter, &offset, NULL, NULL, NULL); > > + > > + if (chrc->authorization_req && offset =3D=3D 0) > > + chrc->authorized =3D false; > > if (chrc->authorization_req && !chrc->authorized) { > > struct authorize_attribute_data *aad; > > @@ -1722,6 +1765,9 @@ static DBusMessage *chrc_write_value(DBusConnection > *conn, DBusMessage *msg, > > return NULL; > > } > > + /* Rewind to value parameter */ > > + dbus_message_iter_init(msg, &iter); > I would prefer to make parse_value_arg parse the offset as well so we don't > have to reinit the iter. Current version of bluetoothctl already parsses offset in parse_value_arg for writing value purposes (patch 1dd33d584ce1e4bd0f1d87e88663350a80f37f01)= . As the offset is second parameter for write method it's needed (afaik) to recurse over iterator to offset parameter then parse value (there is no need to parse value at beginnig). Currently this implementation assumes that every write request with offset =3D 0 is new write request which has t= o be authoriez/re-authorized. > > errsv =3D parse_value_arg(&iter, &chrc->value, &chrc->value_le= n, > chrc->max_val_len); > > if (errsv =3D=3D -EINVAL) { > > -- > > 2.13.6 > > -- > > To unsubscribe from this list: send the line "unsubscribe > linux-bluetooth" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- > Luiz Augusto von Dentz Grzegorz Ko=C5=82odziejczyk ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC 2/2] client: Add authorized property handling to characteristic attribute 2018-05-02 13:06 ` Grzegorz Kołodziejczyk @ 2018-05-03 9:58 ` Luiz Augusto von Dentz 2018-05-04 14:03 ` Grzegorz Kołodziejczyk 0 siblings, 1 reply; 10+ messages in thread From: Luiz Augusto von Dentz @ 2018-05-03 9:58 UTC (permalink / raw) To: Grzegorz Kolodziejczyk; +Cc: linux-bluetooth Hi Grzegorz, On Wed, May 2, 2018 at 4:06 PM Grzegorz Ko=C5=82odziejczyk < grzegorz.kolodziejczyk@codecoup.pl> wrote: > Hi Luiz, > =C5=9Br., 2 maj 2018 o 12:42 Luiz Augusto von Dentz <luiz.dentz@gmail.com= > > napisa=C5=82(a): > > Hi Grzegorz, > > On Wed, May 2, 2018 at 1:09 PM Grzegorz Kolodziejczyk < > > grzegorz.kolodziejczyk@codecoup.pl> wrote: > > > This patch adds handling of authorized property to bluetoothctl. > > > --- > > > client/gatt.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 46 insertions(+) > > > diff --git a/client/gatt.c b/client/gatt.c > > > index 6bf644c48..fffa86851 100644 > > > --- a/client/gatt.c > > > +++ b/client/gatt.c > > > @@ -1432,6 +1432,30 @@ static gboolean chrc_notify_acquired_exists(const > > GDBusPropertyTable *property, > > > return FALSE; > > > } > > > +static gboolean chrc_get_authorized(const GDBusPropertyTable *property, > > > + DBusMessageIter *iter, void > *data) > > > +{ > > > + struct chrc *chrc =3D data; > > > + dbus_bool_t value; > > > + > > > + value =3D chrc->authorized ? TRUE : FALSE; > > > + > > > + dbus_message_iter_append_basic(iter, DBUS_TYPE_BOOLEAN, &value); > > > + > > > + return TRUE; > > > +} > > > + > > > +static gboolean chrc_get_authorized_exists(const GDBusPropertyTable > > *property, > > > + void > > *data) > > > +{ > > > + struct chrc *chrc =3D data; > > > + > > > + if (chrc->authorization_req) > > > + return TRUE; > > > + > > > + return FALSE; > > > +} > > > + > > > static const GDBusPropertyTable chrc_properties[] =3D { > > > { "UUID", "s", chrc_get_uuid, NULL, NULL }, > > > { "Service", "o", chrc_get_service, NULL, NULL }, > > > @@ -1442,6 +1466,8 @@ static const GDBusPropertyTable chrc_properties[] > =3D > > { > > > chrc_write_acquired_exists }= , > > > { "NotifyAcquired", "b", chrc_get_notify_acquired, NULL, > > > chrc_notify_acquired_exists }, > > > + { "Authorized", "b", chrc_get_authorized, NULL, > > > + > > chrc_get_authorized_exists }, > > > { } > > > }; > > > @@ -1665,6 +1691,15 @@ static void authorize_write_response(const cha= r > > *input, void *user_data) > > > chrc->authorized =3D true; > > > + /* Authorization check of prepare writes */ > > > + if (!chrc->value_len) { > > > + reply =3D g_dbus_create_reply(pending_message, > > DBUS_TYPE_INVALID); > > > + g_dbus_send_message(aad->conn, reply); > > > + g_free(aad); > > > + > > > + return; > > > + } > > > + > > > errsv =3D parse_value_arg(&iter, &chrc->value, &chrc->value_len, > > chrc->max_val_len); > > > if (errsv =3D=3D -EINVAL) { > > > @@ -1701,8 +1736,16 @@ static DBusMessage > > *chrc_write_value(DBusConnection *conn, DBusMessage *msg, > > > DBusMessageIter iter; > > > char *str; > > > int errsv; > > > + uint16_t offset =3D 0; > > > dbus_message_iter_init(msg, &iter); > > > + /* Get offset only (second parameter) */ > > > + dbus_message_iter_next(&iter); > > > + > > > + parse_options(&iter, &offset, NULL, NULL, NULL); > > > + > > > + if (chrc->authorization_req && offset =3D=3D 0) > > > + chrc->authorized =3D false; > > > if (chrc->authorization_req && !chrc->authorized) { > > > struct authorize_attribute_data *aad; > > > @@ -1722,6 +1765,9 @@ static DBusMessage > *chrc_write_value(DBusConnection > > *conn, DBusMessage *msg, > > > return NULL; > > > } > > > + /* Rewind to value parameter */ > > > + dbus_message_iter_init(msg, &iter); > > I would prefer to make parse_value_arg parse the offset as well so we > don't > > have to reinit the iter. > Current version of bluetoothctl already parsses offset in parse_value_arg > for writing value purposes (patch 1dd33d584ce1e4bd0f1d87e88663350a80f37f01). > As the offset is second parameter for write method it's needed (afaik) to > recurse over iterator to offset parameter then parse value (there is no > need to parse value at beginnig). Currently this implementation assumes > that every write request with offset =3D 0 is new write request which has= to > be authoriez/re-authorized. That fact that parse_value_arg is already a strong indication that we should reuse it, we just need to add the offset as a output parameter, like it is done in parse_options, that way we will get both the value and the offset in the same call and don't have to reinit the iter. > > > errsv =3D parse_value_arg(&iter, &chrc->value, &chrc->value_len, > > chrc->max_val_len); > > > if (errsv =3D=3D -EINVAL) { > > > -- > > > 2.13.6 > > > -- > > > To unsubscribe from this list: send the line "unsubscribe > > linux-bluetooth" in > > > the body of a message to majordomo@vger.kernel.org > > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- > > Luiz Augusto von Dentz > Grzegorz Ko=C5=82odziejczyk --=20 Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC 2/2] client: Add authorized property handling to characteristic attribute 2018-05-03 9:58 ` Luiz Augusto von Dentz @ 2018-05-04 14:03 ` Grzegorz Kołodziejczyk 0 siblings, 0 replies; 10+ messages in thread From: Grzegorz Kołodziejczyk @ 2018-05-04 14:03 UTC (permalink / raw) To: Luiz Augusto von Dentz; +Cc: linux-bluetooth Hi Luiz, czw., 3 maj 2018 o 11:58 Luiz Augusto von Dentz <luiz.dentz@gmail.com> napisa=C5=82(a): > Hi Grzegorz, > On Wed, May 2, 2018 at 4:06 PM Grzegorz Ko=C5=82odziejczyk < > grzegorz.kolodziejczyk@codecoup.pl> wrote: > > Hi Luiz, > > =C5=9Br., 2 maj 2018 o 12:42 Luiz Augusto von Dentz <luiz.dentz@gmail.c= om> > > napisa=C5=82(a): > > > Hi Grzegorz, > > > On Wed, May 2, 2018 at 1:09 PM Grzegorz Kolodziejczyk < > > > grzegorz.kolodziejczyk@codecoup.pl> wrote: > > > > This patch adds handling of authorized property to bluetoothctl. > > > > --- > > > > client/gatt.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ > > > > 1 file changed, 46 insertions(+) > > > > diff --git a/client/gatt.c b/client/gatt.c > > > > index 6bf644c48..fffa86851 100644 > > > > --- a/client/gatt.c > > > > +++ b/client/gatt.c > > > > @@ -1432,6 +1432,30 @@ static gboolean > chrc_notify_acquired_exists(const > > > GDBusPropertyTable *property, > > > > return FALSE; > > > > } > > > > +static gboolean chrc_get_authorized(const GDBusPropertyTable > *property, > > > > + DBusMessageIter *iter, void > > *data) > > > > +{ > > > > + struct chrc *chrc =3D data; > > > > + dbus_bool_t value; > > > > + > > > > + value =3D chrc->authorized ? TRUE : FALSE; > > > > + > > > > + dbus_message_iter_append_basic(iter, DBUS_TYPE_BOOLEAN, > &value); > > > > + > > > > + return TRUE; > > > > +} > > > > + > > > > +static gboolean chrc_get_authorized_exists(const GDBusPropertyTabl= e > > > *property, > > > > + voi= d > > > *data) > > > > +{ > > > > + struct chrc *chrc =3D data; > > > > + > > > > + if (chrc->authorization_req) > > > > + return TRUE; > > > > + > > > > + return FALSE; > > > > +} > > > > + > > > > static const GDBusPropertyTable chrc_properties[] =3D { > > > > { "UUID", "s", chrc_get_uuid, NULL, NULL }, > > > > { "Service", "o", chrc_get_service, NULL, NULL }, > > > > @@ -1442,6 +1466,8 @@ static const GDBusPropertyTable > chrc_properties[] > > =3D > > > { > > > > chrc_write_acquired_exists }, > > > > { "NotifyAcquired", "b", chrc_get_notify_acquired, NULL, > > > > chrc_notify_acquired_exist= s > }, > > > > + { "Authorized", "b", chrc_get_authorized, NULL, > > > > + > > > chrc_get_authorized_exists }, > > > > { } > > > > }; > > > > @@ -1665,6 +1691,15 @@ static void authorize_write_response(const char > > > *input, void *user_data) > > > > chrc->authorized =3D true; > > > > + /* Authorization check of prepare writes */ > > > > + if (!chrc->value_len) { > > > > + reply =3D g_dbus_create_reply(pending_message, > > > DBUS_TYPE_INVALID); > > > > + g_dbus_send_message(aad->conn, reply); > > > > + g_free(aad); > > > > + > > > > + return; > > > > + } > > > > + > > > > errsv =3D parse_value_arg(&iter, &chrc->value, > &chrc->value_len, > > > chrc->max_val_len); > > > > if (errsv =3D=3D -EINVAL) { > > > > @@ -1701,8 +1736,16 @@ static DBusMessage > > > *chrc_write_value(DBusConnection *conn, DBusMessage *msg, > > > > DBusMessageIter iter; > > > > char *str; > > > > int errsv; > > > > + uint16_t offset =3D 0; > > > > dbus_message_iter_init(msg, &iter); > > > > + /* Get offset only (second parameter) */ > > > > + dbus_message_iter_next(&iter); > > > > + > > > > + parse_options(&iter, &offset, NULL, NULL, NULL); > > > > + > > > > + if (chrc->authorization_req && offset =3D=3D 0) > > > > + chrc->authorized =3D false; > > > > if (chrc->authorization_req && !chrc->authorized) { > > > > struct authorize_attribute_data *aad; > > > > @@ -1722,6 +1765,9 @@ static DBusMessage > > *chrc_write_value(DBusConnection > > > *conn, DBusMessage *msg, > > > > return NULL; > > > > } > > > > + /* Rewind to value parameter */ > > > > + dbus_message_iter_init(msg, &iter); > > > I would prefer to make parse_value_arg parse the offset as well so we > > don't > > > have to reinit the iter. > > Current version of bluetoothctl already parsses offset in parse_value_arg > > for writing value purposes (patch > 1dd33d584ce1e4bd0f1d87e88663350a80f37f01). > > As the offset is second parameter for write method it's needed (afaik) to > > recurse over iterator to offset parameter then parse value (there is no > > need to parse value at beginnig). Currently this implementation assumes > > that every write request with offset =3D 0 is new write request which h= as to > > be authoriez/re-authorized. > That fact that parse_value_arg is already a strong indication that we > should reuse it, we just need to add the offset as a output parameter, like > it is done in parse_options, that way we will get both the value and the > offset in the same call and don't have to reinit the iter. For now the parse_value_arg function is not only parsing dbus message and gets values, but also write attribute value. I guess the idea of requesting authorization for write value with offset = =3D 0 is not considered for now so this wouldn't be needed. > > > > errsv =3D parse_value_arg(&iter, &chrc->value, > &chrc->value_len, > > > chrc->max_val_len); > > > > if (errsv =3D=3D -EINVAL) { > > > > -- > > > > 2.13.6 > > > > -- > > > > To unsubscribe from this list: send the line "unsubscribe > > > linux-bluetooth" in > > > > the body of a message to majordomo@vger.kernel.org > > > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > -- > > > Luiz Augusto von Dentz > > Grzegorz Ko=C5=82odziejczyk > -- > Luiz Augusto von Dentz Grzegorz Ko=C5=82odziejczyk ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2018-05-04 14:03 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-05-02 9:17 [RFC 0/2] Add support for prepare writes authorization Grzegorz Kolodziejczyk 2018-05-02 9:17 ` [RFC 1/2] shared/gatt-server: Request authorization for prepare writes Grzegorz Kolodziejczyk 2018-05-02 11:32 ` Luiz Augusto von Dentz 2018-05-02 13:31 ` Grzegorz Kołodziejczyk 2018-05-03 9:54 ` Luiz Augusto von Dentz 2018-05-02 9:17 ` [RFC 2/2] client: Add authorized property handling to characteristic attribute Grzegorz Kolodziejczyk 2018-05-02 10:42 ` Luiz Augusto von Dentz 2018-05-02 13:06 ` Grzegorz Kołodziejczyk 2018-05-03 9:58 ` Luiz Augusto von Dentz 2018-05-04 14:03 ` Grzegorz Kołodziejczyk
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.