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

Proposal to simplify device object and improve interoperability #233

Merged

Conversation

jpengar
Copy link
Collaborator

@jpengar jpengar commented Jun 11, 2024

What type of PR is this?

  • enhancement/feature
  • documentation

What this PR does / why we need it:

PR incorporating the existing DRAFT proposal to simplify the device object and improve interoperability as per issue #171.

The solution proposed to be covered in the scope of this meta release was:

  1. Apply the mechanism to rely on the access_token (not providing the device object in the API request) for 3-legged access scenarios. This would fix interoperability in all these scenarios, which is a huge improvement. We can define the device identifier as optional in the API specification and, if not provided, simply refer to access_token. The developer would simply not provide any further device information, which is simpler. The operator does not need to check that the device identifier actually matches the access_token provided and could simply rely on the access_token itself.
  2. Validate the current situation among operators regarding the support and use of the networkAccessIdentifier in order to agree on the removal of this identifier for this meta-release.
    At least these two points would be a big improvement over the current situation. To cover all 3-legged scenarios, and in other cases where it is necessary to explicitly provide the device object in the request (such as 2-legged access with client credentials), we would have a smaller set of identifiers that would mitigate the problem a bit. So far, the APIs we are discussing (Device Location family, Device Status, Quality On demand) are mostly consumed with 3-legged access_tokens.

Reference: #171 (comment)

Which issue(s) this PR fixes:

Fixes #171

Special notes for reviewers:

DISCLAIMER: The proposal is a minimum proposal that does not aim to solve all existing problems, and it is recognised that it does not cover all possible future use cases. However, in the context of the next meta-release, the two proposed measures could already be of great benefit.

  • Please find relevant context in Revise the device object definition to simplify it #171
  • As agreed at the 10 June WG meeting, the DRAFT proposal has been included as an appendix to the API Design Guidelines for the time being. But we would need to decide what is the best place to document it. It could be as an appendix, in another specific document in the Commonalities, or even document it in ICM if the mechanism for relying on the access token is considered more appropriate for the ICM (e.g. in "CAMARA APIs access and user consent management" doc).

Changelog input

- Device object simplification
- Mechanism to rely on access token to obtain device information

Additional documentation

2024-05-23 - Commonalities Ad-Hoc Meeting - Device Object review

PedroDiez
PedroDiez previously approved these changes Jun 11, 2024
Copy link
Collaborator

@PedroDiez PedroDiez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor indentation comment

In my view proposal and information allocation is fine

LGTM in advance

documentation/API-design-guidelines.md Outdated Show resolved Hide resolved
documentation/API-design-guidelines.md Show resolved Hide resolved
documentation/API-design-guidelines.md Outdated Show resolved Hide resolved
documentation/API-design-guidelines.md Outdated Show resolved Hide resolved
PedroDiez
PedroDiez previously approved these changes Jun 11, 2024
Copy link
Collaborator

@PedroDiez PedroDiez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

PedroDiez
PedroDiez previously approved these changes Jun 11, 2024
Copy link
Collaborator

@PedroDiez PedroDiez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -59,14 +59,11 @@ components:
* `ipv4Address`
* `ipv6Address`
* `phoneNumber`
* `networkAccessIdentifier`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not think that it is reasonable to remove the networkAccessIdentifier, even though it is not widely used yet.
We actually would like to add a further option to allow identification via the operator token.
For security reasons we see a mandatory need to allow verification of the data subject named in the access token against the target resource, which is input to the api call. We need to keep all option open, especially since we are discussing (in issue #145 in ICM group requesting to support the operator token).

Copy link
Collaborator Author

@jpengar jpengar Jun 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are focusing on a proposal in the scope of the next meta release. The proposal to remove the networkAccessIdentifier is precisely because the feedback so far has been favourable to doing so, and there is no explicit feedback from any WG participant that they are currently using it. Of course, it could be revisited for future releases if the WG needs it, and it could be re-evaluated whether or not to include it again.

The operatorToken discussed in camaraproject/IdentityAndConsentManagement#145 is just an open discussion for now (in ICM backlog), so I would suggest not even considering it for this meta release. In fact, operatorToken might fit as networkAccessIdentifier from our point of view, but it should be discussed for a release when there is a clear position and solution defined in CAMARA for operatorToken.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we try to find a middle way here? I would propose that we could write in the guidelines that the networkAccessIdentifier is only part of the schema to be able to be future-proof and Camara currently does not permit its use.
Post meta-release work, and after the resolution of the relevant issues in ICM, its use will have to be explicitly documented in ICM and in the guidelines.

This will be similar to what we have done for subscriptions where even though event type is an array (for being future-proof), we are allowing for the meta release the use of just single event type and after the meta-release we will target usecases which need subscriptions to support multiple event types.

WDYT?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it works and helps us reach an agreement, that would be fine with me. But I think it would be better to just have an artifact definition of what needs to be supported and allowed to be used. And then just evolve it in the next releases.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Elisabeth-Ericsson : Could you please confirm if this suggested middle way would be ok for you as well?

Copy link
Collaborator Author

@jpengar jpengar Jun 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Middle way agreement applied in 4ae03bb
Please have a look @shilpa-padgaonkar @Elisabeth-Ericsson and suggest any necessary changes to the wording I use.

Copy link
Collaborator Author

@jpengar jpengar Jun 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If an API client only provides the networkAccessIdentifier, we can return a 422 error, and if the networkAccessIdentifier is provided along with other allowed identifiers, it should be ignored.

Should we also add this to the NOTE included in the common.yaml file? Should it be documented in the corresponding API subprojects?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This clarification is very useful, could you confirm if we can add this in our API subprojects?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This clarification is very useful, could you confirm if we can add this in our API subprojects?

There is general guideline in API Design Guidelines section 6.2 Error Responses - Device Object.
The clarification above is in line with it, so it can be added in subprojects.
If more detailed guideline is needed for networkAccessIdentifier it can be added as a fix for API Design Guidelines.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with Rafal

### Optional device object for 3-legged tokens:

- When using a 3-legged access token, the device associated with the access token must be considered as the device for the API request. This means that the device object is not required in the request, and if included it must identify the same device, therefore **it is recommended NOT to include it in these scenarios** to simplify the API usage and avoid additional validations.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We (E///) think that we rather should recommend to have the additional validation in place and compare the target device in the access token against the target device named on the API input. This gives additional security and allows for a better reporting in line with GDPR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see how comparing the device identifier in the API request to the target device in the access token provides any additional security. And if we take the access token information as the source of truth, if we can rely directly on the access token, we would avoid additional validations that add complexity and new potential points of failure. In addition to improving interoperability as explained in #171 (comment)

Copy link
Collaborator

@hdamker hdamker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some suggestion for changes. Especially the template should not be strictly mandatory ("must") as we identified the potential need in QoD for cases where additional information from the device object might be needed. Even if we haven't identified them in detail yet, we want to have the option to describe them in our documentation.

Personal note: I'm not sure if all developers can decide if "the device can be uniquelly identified by the token". If unsure they might provide additional information within the device object, effectively creating the situation that the token is another parameter beside the ones within the device object. Some of them might be validated, others ignored (if not supported).

documentation/API-design-guidelines.md Outdated Show resolved Hide resolved
documentation/API-design-guidelines.md Outdated Show resolved Hide resolved
documentation/API-design-guidelines.md Outdated Show resolved Hide resolved
documentation/API-design-guidelines.md Outdated Show resolved Hide resolved
@jpengar jpengar requested a review from hdamker June 19, 2024 13:47
Copy link
Collaborator

@shilpa-padgaonkar shilpa-padgaonkar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/LGTM

Copy link
Collaborator

@patrice-conil patrice-conil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Collaborator

@PedroDiez PedroDiez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Revise the device object definition to simplify it
8 participants