Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DeviceSecret key is required by protocol v2.0 #845

Closed
qsdigor opened this issue Oct 12, 2022 · 5 comments
Closed

DeviceSecret key is required by protocol v2.0 #845

qsdigor opened this issue Oct 12, 2022 · 5 comments
Assignees
Labels
failing-test Where an automated test is failing either locally or in CI. Perhaps flakey, wrong or bug.
Milestone

Comments

@qsdigor
Copy link
Contributor

qsdigor commented Oct 12, 2022

Field deviceSecret is required from protocol version 2.0 or even 1.2 but it was not implemented so far.

As we implement protocol 2.0 various tests have failed indicating deviceSecret key is missing.

In Java SDK, deviceSecret is present in LocalDevice class which is part of the Android build.
As deviceSecret is required by API, tests fail in the core library as well as android ones.

We have multiple solutions for this problem:

  • Add deviceSecret to DeviceDetails class. This will solve the issue as LocalDevice is extending DeviceDetails and will be able to set that field just as before. Spec change about DeviceDetails will be required as well.
  • We can create LocalDeviceBase class in core and extend it in Java and Android build with their implementation.
  • Remove or move failing tests to Android build only. This option will depend on the android build and won't cover all use cases which is not preferred.
@qsdigor qsdigor linked a pull request Oct 12, 2022 that will close this issue
@qsdigor qsdigor added this to the 2.0.0 milestone Oct 12, 2022
@qsdigor qsdigor added the failing-test Where an automated test is failing either locally or in CI. Perhaps flakey, wrong or bug. label Oct 12, 2022
@paddybyers
Copy link
Member

The apparent java regression has surfaced a problem with the current spec.

The original idea, and what is in the spec today, is that:

  • DeviceDetails encapsulates the details of registerable devices, such as push transport/recipient details, for the purposes of the push admin API (which allows clients to interrogate device details and list registered devices);
  • LocalDevice extends DeviceDetails, and additionally includes device identity credentials that are only known to that target device specifically (deviceSecret and deviceIdentityToken). These are used when a client on that device performs operations with push-subscribe rights, to authenticate the device identity to the server.

However, there is a problem with this, which is that we also support a flow where the registration of devices can be performed not by the device itself but by a custom registrar, and a custom registrar has to use the push admin API to do this. As a result, the registrar must be able to populate the device identity credentials, and for that to be possible (as the API is declared right now) those attributes need to exist in DeviceDetails.

The whole thing is messy because other users of the push admin API shouldn't be able to discover device identity credentials (even with push-admin rights) and so those details are removed always in admin API responses that get or list registered device details.

ably-js declares the device identity attributes in DeviceDetails: https://github.com/ably/ably-js/blob/main/src/common/lib/types/devicedetails.ts#L43.

So really the original idea I think is still the correct one - push admin clients shouldn't get to specify, or discover, device identity credentials - but the custom registrar requirement messes this up and this needs to be rethought (or removed). The question is what we do about this right now - do we update this java library to align with ably-js and others and include the device identity attributes in DeviceDetails, or make a much bigger change to fix the broken API with respect to custom registrars?

@qsdigor
Copy link
Contributor Author

qsdigor commented Oct 17, 2022

I have implemented this change in the protocol 2.0 branch as it is a part of it.

@deanna-lad
Copy link

The corresponding Jira issue is https://ably.atlassian.net/browse/SDK-2713

@sacOO7
Copy link
Collaborator

sacOO7 commented Feb 6, 2024

This is fixed serverside as a part of https://github.com/ably/realtime/pull/5394. It was part of the slack discussion https://ably-real-time.slack.com/archives/C030C5YLY/p1700741804479899?thread_ts=1699279152.506959&cid=C030C5YLY.
So, no loner needed as such as a part of protocol 2.0

@sacOO7 sacOO7 self-assigned this Feb 6, 2024
@sacOO7
Copy link
Collaborator

sacOO7 commented Feb 6, 2024

Not needed as a part of protocol 2.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
failing-test Where an automated test is failing either locally or in CI. Perhaps flakey, wrong or bug.
Development

Successfully merging a pull request may close this issue.

4 participants