eclipse / kuksa.val

kuksa.val
Apache License 2.0
95 stars 51 forks source link

Use entries iterator that handles permissions correctly #654

Closed rafaeling closed 1 year ago

rafaeling commented 1 year ago

New expected behaviour

  1. Client remains as it was, just pre-commit style was updated.
  2. Databroker responses works as follow:

    • getValue(path): return value or error if there is any.

    • getValues(path1, path2, ... , n) return all path values if there are no errors, if there is/are any error/errors it would return no partial values and just its corresponding error for each error path.

    • getValue(wildcard): return value or error if there is any but no partial values

    • getValues(wildcard1, wildcard2, ... , n) return all wildcard values if there are no errors, if there is/are any error/errors it would return no partial values of any wildcard and just its corresponding error for each error wildcard.

    • getValues(wildcard1, path2, ... , n) return all path/wildcard values if there are no errors, if there is/are any error/errors it would return no partial values and just its corresponding error for each error path/wildcard.

    • getMetaData(wildcard or path) return values without permission errors.

(Outdated) How wildards and request errors are handled?

rafaeling commented 1 year ago

A separate PR should be created to fix the actuator filter on kuksa-client for getTargetValue method

SebastianSchildt commented 1 year ago

My understanding was, last discussion went to having metaData free for all for now, or has that changed? Espiecially wouldnt this break "Learning" with "*" what data is available?

rafaeling commented 1 year ago

My understanding was, last discussion went to having metaData free for all for now, or has that changed? Espiecially wouldnt this break "Learning" with "*" what data is available?

It is a bug, getMetaData should work anyways, have to fix it, thanks Lukas :)

rafaeling commented 1 year ago

PR description was updated with the new behaviour -> https://github.com/eclipse/kuksa.val/pull/654#issue-1893980884

Discussions regarding modifications to the gRPC interface for metadata, as well as new features for separating wildcard and paths, will be moved to a new issue.

argerus commented 1 year ago

This is perhaps overly pedantic, but the server is still returning "partial success" and silently ignoring permission errors.

from kuksa_client.grpc import VSSClient, EntryRequest, View, Field

read_vehicle_speed_token = "eyJ0eXAiOiJKV1QiLCJhbGciOiJSUzI1NiJ9.eyJzdWIiOiJsb2NhbCBkZXYiLCJpc3MiOiJjcmVhdGVUb2tlbi5weSIsImF1ZCI6WyJrdWtzYS52YWwiXSwiaWF0IjoxNTE2MjM5MDIyLCJleHAiOjE3NjcyMjU1OTksInNjb3BlIjoicmVhZDpWZWhpY2xlLlNwZWVkIn0.QyaKO-vjjzeimAAyUMjlM_jAvhlmYVguzXp76jAnW9UUiWowrXeYTa_ERVzZqxz21txq1QQmee8WtCO978w95irSRugTmlydWNjS6xPGAOCan6TEXBryrmR5FSmm6fPNrgLPhFEfcqFIQm07B286JTU98s-s2yeb6bJRpm7gOzhH5klCLxBPFYNncwdqqaT6aQD31xOcq4evooPOoV5KZFKI9WKkzOclDvto6TCLYIgnxu9Oey_IqyRAkDHybeFB7LR-1XexweRjWKleXGijpNweu6ATsbPakeVIfJ6k3IN-oKNvP6nawtBg7GeSEFYNksUUCVrIg8b0_tZi3c3E114MvdiDe7iWfRd5SDjO1f8nKiqDYy9P-hwejHntsaXLZbWSs_GNbWmi6J6pwgdM4XI3x4vx-0Otsb4zZ0tOmXjIL_tRsKA5dIlSBYEpIZq6jSXgQLN2_Ame0i--gGt4jTg1sYJHqE56BKc74HqkcvrQAZrZcoeqiKkwKcy6ofRpIQ57ghooZLkJ8BLWEV_zJgSffHBX140EEnMg1z8GAhrsbGvJ29TmXGmYyLrAhyTj_L6aNCZ1XEkbK0Yy5pco9sFGRm9nKTeFcNICogPuzbHg4xsGX_SZgUmla8Acw21AAXwuFY60_aFDUlCKZh93Z2SAruz1gfNIL43I0L8-wfw"

client = VSSClient(host="127.0.0.1", port=55555)
client.connect()
client.authorize(token=read_vehicle_speed_token)

# This will give no error / exception. I.e. permission denied is silently ignored and
# all metadata is returned + the value for the one thing we have permission for ("Vehicle.Speed").
resp1 = client.get([EntryRequest(path="Vehicle", view=View.UNSPECIFIED, fields=[Field.VALUE, Field.METADATA])])

# Causes an exception
# kuksa_client.grpc.VSSClientError: ({'code': 403, 'reason': 'forbidden', 'message': 'Permission denied for some entries'}, [{'path': 'Vehicle', 'error': {'code': 403, 'reason': 'forbidden', 'message': 'Permission denied for some entries'}}])
resp2 = client.get([EntryRequest(path="Vehicle", view=View.UNSPECIFIED, fields=[Field.VALUE])])

PS Unrelated, but the python client just returns None if one fails to call connect before making a call.

client = VSSClient(host="127.0.0.1", port=55555)
client.authorize(token=read_vehicle_speed_token)

# client.get(...) returns None

and fails to use the access token provided to it if client.connect() is called after client.authorize(...)

client = VSSClient(host="127.0.0.1", port=55555)
client.authorize(token=read_vehicle_speed_token)
client.connect()

# client.get(...) fails with
# kuksa_client.grpc.VSSClientError: ({'code': 16, 'reason': 'unauthenticated', 'message': 'Invalid auth token'}, [])
SebastianSchildt commented 1 year ago

I tested the behaviors lined out, and those work for me as I expect.

for Johns first comment: If several things are requested, i.e. in that case Metadata and Value, would it be possible @rafaeling to also just deny the whole request, i.e. that always "deny" wins, i.e. if requesting value, target and metaData for a single data point, but you miss e.g. read rights, it will just deny the whole request? or is this hard with how this is currently architected?

The other comment, i.e. returning none on get when not connected, might be worth a separate issue. Not ideal, but let's not fix it here.

I think, that the token is not automatically used after "connect" is maybe ok, because as I see it, connect() resets the whole state client side, and I understand authorize more like a function, and not an inherent property fo the client object. I know this is not how it is in reality, but for waht you expect @argerus I would maybe expect the function not be called "authorize", but rather set_authorization_token or something like that, but same -> Sounds like material for a separate issue

rafaeling commented 1 year ago

This is perhaps overly pedantic, but the server is still returning "partial success" and silently ignoring permission errors.


from kuksa_client.grpc import VSSClient, EntryRequest, View, Field

read_vehicle_speed_token = "eyJ0eXAiOiJKV1QiLCJhbGciOiJSUzI1NiJ9.eyJzdWIiOiJsb2NhbCBkZXYiLCJpc3MiOiJjcmVhdGVUb2tlbi5weSIsImF1ZCI6WyJrdWtzYS52YWwiXSwiaWF0IjoxNTE2MjM5MDIyLCJleHAiOjE3NjcyMjU1OTksInNjb3BlIjoicmVhZDpWZWhpY2xlLlNwZWVkIn0.QyaKO-vjjzeimAAyUMjlM_jAvhlmYVguzXp76jAnW9UUiWowrXeYTa_ERVzZqxz21txq1QQmee8WtCO978w95irSRugTmlydWNjS6xPGAOCan6TEXBryrmR5FSmm6fPNrgLPhFEfcqFIQm07B286JTU98s-s2yeb6bJRpm7gOzhH5klCLxBPFYNncwdqqaT6aQD31xOcq4evooPOoV5KZFKI9WKkzOclDvto6TCLYIgnxu9Oey_IqyRAkDHybeFB7LR-1XexweRjWKleXGijpNweu6ATsbPakeVIfJ6k3IN-oKNvP6nawtBg7GeSEFYNksUUCVrIg8b0_tZi3c3E114MvdiDe7iWfRd5SDjO1f8nKiqDYy9P-hwejHntsaXLZbWSs_GNbWmi6J6pwgdM4XI3x4vx-0Otsb4zZ0tOmXjIL_tRsKA5dIlSBYEpIZq6jSXgQLN2_Ame0i--gGt4jTg1sYJHqE56BKc74HqkcvrQAZrZcoeqiKkwKcy6ofRpIQ57ghooZLkJ8BLWEV_zJgSffHBX140EEnMg1z8GAhrsbGvJ29TmXGmYyLrAhyTj_L6aNCZ1XEkbK0Yy5pco9sFGRm9nKTeFcNICogPuzbHg4xsGX_SZgUmla8Acw21AAXwuFY60_aFDUlCKZh93Z2SAruz1gfNIL43I0L8-wfw"

client = VSSClient(host="127.0.0.1", port=55555)
client.connect()
client.authorize(token=read_vehicle_speed_token)

# This will give no error / exception. I.e. permission denied is silently ignored and
# all metadata is returned + the value for the one thing we have permission for ("Vehicle.Speed").
resp1 = client.get([EntryRequest(path="Vehicle", view=View.UNSPECIFIED, fields=[Field.VALUE, Field.METADATA])])
Good you found it but, what response should be expected here? An error or the metadata?
argerus commented 1 year ago

Sounds like material for a separate issue

Yes, definitely not in this PR.

Good you found it but, what response should be expected here? An error or the metadata?

An error. Partial success are:

  1. Not supported by kuksa-client.
  2. Confusing in general.