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

Initial API proposal #11

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

eric-murray
Copy link
Contributor

@eric-murray eric-murray commented Sep 27, 2024

What type of PR is this?

  • initial API proposal

What this PR does / why we need it:

This is the initial proposal from Vodafone for the KYC Tenure API

Which issue(s) this PR fixes:

Fixes #12

Special notes for reviewers:

None

Changelog input

 release-note
 - Initial API proposal

Additional documentation

None

Copy link

@GillesInnov35 GillesInnov35 left a comment

Choose a reason for hiding this comment

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

hi @eric-murray , thanks a lot for this good proposal, it's OK. I've a question on what we've started to discuss on discussions #7 and #8. What do you think about adding those rules in the API overview.

@eric-murray
Copy link
Contributor Author

@GillesInnov35
Once the rules on generating the tenure response are agreed, then these can indeed be added to the API definition as documentation.

But there is no need to wait for these rules to be agreed before merging this PR. This is just a "work in progress" version, which will be subject to further updates before being released as an initial version. Further updates can be pushed to the repository as they are agreed.

Copy link

@GillesInnov35 GillesInnov35 left a comment

Choose a reason for hiding this comment

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

Thanks
BR
Gilles

@eric-murray
Copy link
Contributor Author

Thanks @GillesInnov35

I won't merge this until others have raised any questions they may have, and have given their approval as well. It should also be discussed during at least one of the bi-weekly KYC meetings. As the next meeting is tomorrow, I think we are looking at waiting until after the subsequent meeting before merging this.

And I also need approval for PR #13. Currently, the YAML linter does not like line lengths more than 80 characters. That PR is required so that that rule can be disabled.

@eric-murray eric-murray requested a review from a team October 8, 2024 15:28
grgpapadopoulos
grgpapadopoulos previously approved these changes Oct 9, 2024
@eric-murray eric-murray requested a review from a team October 10, 2024 09:40
GillesInnov35
GillesInnov35 previously approved these changes Oct 10, 2024
Copy link

@GillesInnov35 GillesInnov35 left a comment

Choose a reason for hiding this comment

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

OK Thanks a lot Eric

@eric-murray
Copy link
Contributor Author

OK Thanks a lot Eric

Hi @GillesInnov35. I was re-adding @camaraproject/tenure_maintainers as a reviewer to bring this PR to the attention of Telefonica, who have no codeowner for this repository, but are maintainers. Now I have just added @claraserranosolsona and @jgarciahospital directly as reviewers, which is perhaps simpler. I would like Telefonica feedback on this proposal before merging.

@GillesInnov35
Copy link

GillesInnov35 commented Oct 14, 2024

Hi @eric-murray, tenureDateCheck is currently a boolean attribute. I wonder if the API will have also to return the information that the date comparison could not been done because the MNO do have the correct information, and then return "not_available".
What do you think about that ?

BR
Gilles

@eric-murray
Copy link
Contributor Author

Hi @GillesInnov35

Can you clarify your proposal? Is this to handle mobile subscribers for which no answer can be given, because their tenureDate is just not known? Or is it to handle the situation that the requested tenureDate is so long ago that the MNO cannot verify that date, but could verify a later date?

The assumption behind the API design was that each mobile subscriber has a recorded tenure date with no limit on how long ago that was. So if a subscriber had the same MSISDN for, say, 20 years, then the tenure date would be known to be 20 years ago, and not "more than 10 years", for example.

Do we need to consider making the requested tenureDate relative? So rather than the API consumer asking about tenure since, for example, 14-10-2004, they would ask about tenure over the last 20 years. The advantage of using relative time periods is that a maximum can be easily specified in the OAS definition, and then all implementations would be expected to support this maximum.

Because date-time is a string format, a "minimum" or "maximum" date cannot be built in to the OAS definition itself, though of course the documentation could specify such a limit. But trapping dates that do not comply with such a requirement takes a bit more work.

@GillesInnov35
Copy link

Thanks Eric
Yes my wondering concerned the case when no answer can be given by the MNO. What will happen when the user is not the owner of the contract according to MNO and countries privacies ?
For the second point, I agree with you, using relative time period seems to to be more useful and simplier.
Perhaps to be reviewed in project meeting.

BR
Gilles

@GillesInnov35
Copy link

Other way, would be to return the date when the line was opened.
in this case operation would be POST /retrieve-tenure

@eric-murray
Copy link
Contributor Author

What will happen when the user is not the owner of the contract according to MNO and countries privacies ?

Commonalities provide an error that can be used when a given service does not apply to a given "device" (in this case, subscription):

            GENERIC_422_DEVICE_NOT_APPLICABLE:
              description: Service is not available for the provided device
              value:
                status: 422
                code: DEVICE_NOT_APPLICABLE
                message: The service is not available for the provided device.

This was intended for cases such as querying the SIM Swap API with a fixed line phone number. I included this error in the proposed YAML.

Curiously, the SIM Swap API did not adopt this error, but instead defines:

            GENERIC_422_NOT_SUPPORTED:
              description: Not Supported
              value:
                status: 422
                code: NOT_SUPPORTED
                message: Service not supported for this phoneNumber

which is maybe because "device" can be viewed as a confusing term to use when it is identifying the subscription. But the mechanism is the same - there is an explicit 422 error defined for this case, rather than returning a 200 response which, on closer inspection, turns out to be an error.

@GillesInnov35
Copy link

Thanks Eric, OK it works and the response error seems to be clear. So "not-available" is not useful.

Gilles

@HuubAppelboom
Copy link

@eric-murray

Hi Eric,

It may be better to follow the SIm Swap 422 error message, because it is indeed for the phone number (and not the device)

Copy link

@fernandopradocabrillo fernandopradocabrillo left a comment

Choose a reason for hiding this comment

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

Hi @eric-murray! Thanks for crafting the initial PR.
Sorry for the late review, from TEF we are also interested in this API.

code/API_definitions/kyc-tenure.yaml Show resolved Hide resolved
code/API_definitions/kyc-tenure.yaml Show resolved Hide resolved
status: 404
code: NOT_FOUND
message: The specified resource is not found.
GENERIC_404_DEVICE_NOT_FOUND:

Choose a reason for hiding this comment

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

DEVICE_* errors are specific to endpoints that have the device object as input. Here we only have the phone number. It should be aligned with other APIs that use the same format like sim swap or number verification.
I think the standard 404 can cover this error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just took the standard errors defined by Commonalities and added then to the first YAML proposal. I agree that some find the term "DEVICE" confusing, but nonetheless we are using phoneNumber to identify what CAMARA calls the "device", but in our case means "device subscription". Just because we exclude IP addresses or the network access identifier does not change that.

So DEVICE_NOT_FOUND is the error code we should be returning when the "device identifier" (in our case, subscription identifier) provided by the API consumer is not one that the API provider knows anything about. NOT_FOUND is the generic code to return when the thing not found is not a "device".

We can maybe argue that the CAMARA concept of a "device" is too broad, and we should clearly separate "physical device" from "device subscription", and maybe have special rules for phoneNumber only. But that is a discussion to have in Commonalities.

Choose a reason for hiding this comment

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

I just took the standard errors defined by Commonalities and added then to the first YAML proposal. I agree that some find the term "DEVICE" confusing, but nonetheless we are using phoneNumber to identify what CAMARA calls the "device", but in our case means "device subscription". Just because we exclude IP addresses or the network access identifier does not change that.

I don't agree with this statement. The Commonalities documentation, both for the errors and the info.description section, refers to the device object, not the concept of device.

Here we are not using the phone number as a device identifier, we are using it as a subscription identifier. I think that the concept of "device subscription" is misleading in this context. Is there an example API in CAMARA where the device object is not referring to a physical device? I only know Device Status and Device Location. There is also Device Swap which doesn't use the device object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking the API applied to all networks, but remember now it was mobile networks only. So forget "device subscription", and let's define "mobile subscription" and "physical mobile device".

  • So the Device object can be used to identify subject of the API, and this can be either the mobile subscription or the physical mobile device. Both meanings are possible.

  • Similarly, the phoneNumber can be used to identify the subject of the API, and again can be either the mobile subscription or the physical mobile device. This is to be expected, as phoneNumber is also a field of the Device object.

  • Whether "mobile subscription" or "physical mobile device" is intended as the subject is entirely dependent on the context of the individual API. For this API, it is "mobile subscription", but that could have been identified using a Device object. I thought that cumbersome for this use case, so proposed phoneNumber only.

  • In general, this API will be called using a 3-legged token. So the API consumer will generally not call the API with either an explicit phoneNumber or (if it had been defined instead) and explicit Device object. Whether phoneNumber or Device object is specified in the YAML makes no difference to how the 3-legged token is obtained and used.

  • We (as API definers) have no idea what specific identifier is associated with the access token by all possible API providers. It does not have to be MSISDN. It does not even have to be requested using MSISDN in the login_hint, as ipport: is entirely acceptable for generating an access token. Access tokens can be valid for for multiple APIs, and whether the end user identifier associated with the access token is used to identify the mobile subscription or the physical mobile device is entirely dependent on the context of each API that the access token is valid for.

  • Because we must define the possibility to pass an explicit mobile subscription identifier as the subject in the case that a 2-legged access token is used (this will not be the normal case) we open ourselves up to 2 additional errors:

    • The API consumer uses a 3-legged token but then anyway specifies an explicit mobile subscription identifier
    • The API consumer uses a 2-legged token, but does not specify any explicit mobile subscription identifier
  • These error cases were considered by Commonalities, who defined two new error codes:

    • DEVICE_IDENTIFIERS_MISMATCH when a 3-legged token is provided but also an explicit identifier for a different mobile subscription or physical mobile device
    • UNIDENTIFIABLE_DEVICE when a 2-legged token is provided, but no explicit identifier for a different mobile subscription or physical mobile device

Now Commonalities have not documented this as well or as comprehensively as it could be, but the important point is that, whether the intended subject of the API is the mobile subscription or the physical mobile device, the error is the same, and hence the same error code should be used.

Defining new error codes just because the explicit subject identifier is restricted to phoneNumber rather than being the more general Device object, or because of an objection to using the term "DEVICE_" when the subject is a mobile subscription is pointless, and increases implementation complexity (for both API consumer and provider).

The error message can always be customised, as these are just examples, and so I am happy to define more relevant error messages. But new error codes not defined by Commonalities should go through Commonalities first, and not just be introduced by APIs arbitrarily.

In this case, I think we could remove all objections by asking Commonalities to rename the error codes to something like DEVICE_OR_SUBSCRIPTION_IDENTIFIER_MISMATCH and UNIDENTIFIABLE_DEVICE_OR_SUBSCRIPTION respectively.

Choose a reason for hiding this comment

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

Hi Eric,

I agree the same error code should be used, but I think the use of the word device is leading to confusion. because people may think this refers to hardware.

One suggestion from my side : It may be a better idea to use for example IDENTIFIER_MISMATCH and MISSING_IDENTIFIER in stead of DEVICE_IDENTIFIERS_MISMATCH and UNIDENTIFIABLE_DEVICE. This keeps it in the middle what identifier it is, and also does not point to a device or sim card.

Choose a reason for hiding this comment

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

sorry Eric, my comment was not clear. Those 2 errors with associated messages are already available in Simswap and to have consistent API development we need also 422 error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those 2 errors with associated messages are already available in Simswap and to have consistent API development we need also 422 error.

SIM Swap should have complied with CAMARA Commonalities guidelines on Error Responses, and that requirement will remain as the guidelines are updated.

So SIM Swap will need to update its error response codes (but not necessarily the error message) in any future version to be CAMARA compliant. This is a consequence of SIM Swap defining custom error messages and not engaging with Commonalities. But there is no obligation to update - SIM Swap can continue as v1.0.0 forever.

But we can't have the situation where new APIs unrelated to SIM Swap maintain an ongoing non-compliance with CAMARA guidelines solely to simplify the development tracks of some API providers.

Choose a reason for hiding this comment

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

Hi @eric-murray @GillesInnov35
I also feel more comfortable aligning with what was decided for Sim Swap. I don't share the opinion that it was not compliant with commonalities, it was not a random decision, there was a reasoning behind.
I like the path that this issue is taking in commonalities but maybe waiting for it to be formilize in Spring25 is too long.

Choose a reason for hiding this comment

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

hi @eric-murray @fernandopradocabrillo, I think we should made a decision before Sring25.
Thanks

BR
Gilles

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fernandopradocabrillo
OK. I'll add the 422 codes NOT_SUPPORTED and UNIDENTIFIABLE_PHONE_NUMBER, and then we can review once the next release of the Commonalities guidelines is available.

Comment on lines 320 to 337
GENERIC_422_DEVICE_IDENTIFIERS_MISMATCH:
description: Inconsistency between device identifiers not pointing to the same device
value:
status: 422
code: DEVICE_IDENTIFIERS_MISMATCH
message: Provided device identifiers are not consistent.
GENERIC_422_DEVICE_NOT_APPLICABLE:
description: Service is not available for the provided device
value:
status: 422
code: DEVICE_NOT_APPLICABLE
message: The service is not available for the provided device.
GENERIC_422_UNIDENTIFIABLE_DEVICE:
description: No identifier provided for any device
value:
status: 422
code: UNIDENTIFIABLE_DEVICE
message: No identifier provided for any device

Choose a reason for hiding this comment

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

Same as the previous comment. Here the device object is not used.

  1. DEVICE_IDENTIFIERS_MISMATCH -> since there are no multiple identifiers in the input this one makes no sense here. It is used, for example, when providing an ip and a phone number from different devices.
  2. DEVICE_NOT_APPLICABLE -> in Sim swap we defined 422 NOT_SUPPORTED to indicate that the service is not supported for the provided phone number
  3. UNIDENTIFIABLE_DEVICE -> Adapted to UNIDENTIFIABLE_PHONE_NUMBER with the same meaning but for APIs that use the phone number as input

references for the errors discussions:
camaraproject/SimSwap#117
camaraproject/Commonalities#145 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

phoneNumber is a subset of the Device object, and I don't think special rules should apply just because we propose not to allow device subscriptions to be identified by IP address or network access identifier.

But also, and this is important, a 3-legged token can be generated from the IP address. So just because we do not explicitly allow IP address as a device subscription identifier does not mean that IP address cannot be used to generate a 3-legged token.

More specifically:

  • DEVICE_IDENTIFIERS_MISMATCH is intended when the "device" identified by a 3-legged token is different to that explicitly passed (in this case, by phoneNumber). There are privacy issues here if we confirm whether or not the identifiers match, so this one may yet be removed by Commonalities.
  • DEVICE_NOT_APPLICABLE is what SIM Swap should have used rather than define their own error code
  • UNIDENTIFIABLE_DEVICE is when a 2-legged token is used with no phoneNumber being provided.

I don't like that APIs define new error codes without discussing this in Commonalities and getting them standardised.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DEVICE_IDENTIFIERS_MISMATCH is redundant because of the privacy issues, so I removed it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See this comment. Proposal would be to use SERVICE_NOT_APPLICABLE and MISSING_IDENTIFIER.

Other errors may be returned as specified below.
# Identifying a device from the access token

Choose a reason for hiding this comment

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

Suggested change
# Identifying a device from the access token
# Identifying a phone number from the access token

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A 3-legged token may be generated from the device IP address

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See Commonalities #324 for updated text proposal

code/API_definitions/kyc-tenure.yaml Outdated Show resolved Hide resolved
code/API_definitions/kyc-tenure.yaml Show resolved Hide resolved
@eric-murray
Copy link
Contributor Author

Hi @HuubAppelboom

The message is just an example, and API providers can always change it to something more relevant. Indeed, we could change the example in the OAS definition if we prefer a different message to that in the Commonalities common JSON file.

What does need to be standardised are the codes. Code NOT_SUPPORTED is not defined as a valid code by Commonalities, so that needs to be fixed. Either SIM Swap (or one of the other APIs that use this code) need to submit it to Commonalities, or they need to adopt an existing valid code.

Commonalities should tighten up code definitions for the next update of their design guidelines, so let's see what they define then.

Co-authored-by: Fernando Prado Cabrillo <pradocabrillo.fernando@gmail.com>
Copy link

@GillesInnov35 GillesInnov35 left a comment

Choose a reason for hiding this comment

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

Thanks Eric

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.

Initial proposal for KYC Tenure API definition
5 participants