Sim swap alignement with commonalities 0.5 #180

bigludo7 · 2024-12-23T15:51:53Z

What type of PR is this?

Add one of the following kinds:

enhancement/feature

What this PR does / why we need it:

Alignement of sim_swap.yaml, sim_swap-retrieveDate.feature and sim_swap-check.feature with commonalities 0.5 rules.
See detail in release note

Which issue(s) this PR fixes:

Fixes #161

Special notes for reviewers:

This is breaking change.

Changelog input

release-note
sim_swap.yaml:
- Change error structure definition to normalize error & status
- Update error code to introduce 429
- Remove 5xx errors
- Change in 422 code
- Add 3-legged token examples
- Improve documentation part

sim_swap-retrieveDate.feature:
- update test case scenario for 422
- Add optional test case for 401 AUTHENTICATION_REQUIRED

sim_swap-check.feature:
- update test case scenario for 422
- Add optional test case for 401 AUTHENTICATION_REQUIRED

Additional documentation

This section can be blank.

docs

Align sim-swap.yaml with commonalities 0.5

Update sim-swap-check.feature with commonalities 0.5

github-actions · 2024-12-23T15:53:00Z

🦙 MegaLinter status: ✅ SUCCESS

Descriptor	Linter	Files	Errors	Elapsed time
✅ ACTION	actionlint	2	0	0.02s
✅ OPENAPI	spectral	2	0	3.24s
✅ REPOSITORY	git_diff	yes	no	0.01s
✅ REPOSITORY	secretlint	yes	no	0.78s
✅ YAML	yamllint	2	0	0.57s

See detailed report in MegaLinter reports

MegaLinter is graciously provided by

gregory1g · 2025-01-06T07:05:03Z

code/API_definitions/sim-swap.yaml


-    This specification defines the `phoneNumber` field as optional in API requests, specifically in cases where the API is accessed using a 3-legged access token, and the phone number can be uniquely identified by the token. This approach simplifies API usage for API consumers by relying on the information associated with the access token used to invoke the API.
+    This API requires the API consumer to identify a device as the subject of the API as follows:


For SimSwap a subject of the API is a phone number, not a device. The "device-centric" language is misleading.

Agreed. I will apply.

gregory1g · 2025-01-06T07:07:57Z

code/API_definitions/sim-swap.yaml

@@ -326,7 +354,7 @@ components:
              value:
                status: 403
                code: INVALID_TOKEN_CONTEXT
-                message: phoneNumber is not consistent with access token
+                message: "{{field}} is not consistent with access token."


What "field" other than the phoneNumber can it be?

I followed what is described in the CAMARA_common.yaml

I tend to think that this error should not be triggered in our case because if you pass a 3-legs access token identifying the phone number and a phone number in the body the 422 UNNECESSARY_IDENTIFIER must be triggered.

Yes, I understood that you follow commonalities. But should we really copy paste its wording 1:1 when we know better. A developer will read this text and can be confused.

And you are right - with the change this PR introduces, this check will not be executed: 422 to be returned without checking if both match or not. Let's remove it.

Yes make sense for me. to remove ut but I would like to hear from @fernandopradocabrillo before to proceed.

Fernando do you think we can remove 403 INVALID_TOKEN_CONTEXT ? as we do not see when this error will be triggered.

code/API_definitions/sim-swap.yaml

gregory1g · 2025-01-06T07:16:54Z

code/API_definitions/sim-swap.yaml

-    ### Restrictions for tokens without an associated authenticated phone number:
-
-    - For scenarios which do not have a phone number associated to the token during the authentication flow, e.g. 2-legged access tokens, the `phoneNumber` field MUST be provided in the API request. This ensures that the phone number is explicit and valid for each API call made with these tokens.
+    - If the subject can be identified from the access token and the optional `phoneNumber` identifier is also included in the request, then the server will return an error with the `422 UNNECESSARY_IDENTIFIER` error code. This will be the case even if the same device is identified by these two methods, as the server is unable to make this comparison.


I agree with the logic of the change, but this is a not-backward compatible change. Currently one may send 3-legged token and the phoneNumber (even if this is not recommended), with this change this will cause a error.

Agree we have a breaking change here but we have to follow commonalities. Indeed, If you use 3-legs access token with a phoneNumber in the body you'll have a 422 UNNECESSARY_IDENTIFIER while previously it worked.

As a reminder we introduced this to avoid 'hidden' number verification

The purpose of my comment was to highlight this, and make sure that the consequences of new commonalities are not overlooked.

code/API_definitions/sim-swap.yaml

removed 403 INVALID_TOKEN_CONTEXT

bigludo7 · 2025-01-09T07:18:36Z

@fernandopradocabrillo @gregory1g Following camaraproject/Commonalities#369 discussion I think it's fair to remove 403 INVALID_TOKEN_CONTEXT example in the yaml.

May I ask you to review this new proposal soon as we have to prepare the RC candidate before Jan17th. We can still modify after but we have to achieve the milestone.

gregory1g · 2025-01-09T07:40:16Z

code/API_definitions/sim-swap.yaml


    It is important to remark that in cases where personal user data is processed by the API, and users can exercise their rights through mechanisms such as opt-in and/or opt-out, the use of 3-legged access tokens becomes mandatory. This measure ensures that the API remains in strict compliance with user privacy preferences and regulatory obligations, upholding the principles of transparency and user-centric data control.

-    # Identifying a phone number from the access token
+    # Identifying the device from the access token


I thought that we agreed that for this API we keep phoneNumber as a the API subject.

You're right. I fixed line 49 but missed this one. I will fix. Thanks

gregory1g · 2025-01-09T07:54:07Z

code/API_definitions/sim-swap.yaml

+                code: SERVICE_NOT_APPLICABLE
+                message: The service is not available for the provided identifier.
+            GENERIC_422_MISSING_IDENTIFIER:
+              description: An identifier is not included in the request and the device or phone number identification cannot be derived from the 3-legged access token


If this error for 3-legged token invocation only it should not say "not included in the request", because this is not allowed for the 3-legged invocation.

If the error is generic it should not explicitly mention 3-legged token (because it could be a 2-legged token). Like "An identifier is not included in the request and the device or phone number identification cannot be derived from access token".

Since "MISSING_IDENTIFIER" error message uses a generic "access token" style, I suggest to use generic style here as well.

This is generic.
I followed commonalities guideline see here.

I suggest @gregory1g that you post your comment in commonalities as it makes sense.

created an issue there: camaraproject/Commonalities#370

according to Eric, commonality's "description" is not a error message to be send, it is a description for the reader of the table in the document. While example of the message is in the "Message example" column. An this is just an example, different API can use different text

Proposal discussed with @fernandopradocabrillo : We propose to add precision about this error in the Test Case

I've reworked the yaml & TC to add all clarification - please review.

gregory1g · 2025-01-09T08:04:54Z

code/API_definitions/sim-swap.yaml

-              description: API Server Timeout
+                status: 429
+                code: QUOTA_EXCEEDED
+                message: Either out of resource quota or reaching rate limiting.


As a API client I would prefer an error message that describes the issue precisely, especially since the server distinguishes these tow issues (code is different). Currently, the human readable description is more generic that error code.

This is a good comment @gregory1g but it should be triggered to commonalities first as here I used what we have defined for commonalities v0.5 (in https://github.com/camaraproject/Commonalities/blob/main/artifacts/CAMARA_common.yaml)

I suggest to not change this for this release to keep alignement with Commonalities 0.5 and then change this depending of commonalities outcome for next iteration. Work for you?

ok. Added camaraproject/Commonalities#372

according to Eric "Message example" column where the text came from us just an example, different API can use different text. So, there is no need to use exactly the same text as in the commonalities.

Thanks @gregory1g
As the implementation is free to change the description do we have to provide here something different from other API for 429?
I 'm fine to provide 2 exemple if is it a blocking point for you but me preference is to keep as it.

@fernandopradocabrillo

Discussed with @fernandopradocabrillo --> I will fix the yaml as suggested by @gregory1g

Fixe line 47 (device -> phone number)

bigludo7 added 3 commits December 23, 2024 16:42

Align sim-swap.yaml with commonalities 0.5

8749bf3

Align sim-swap.yaml with commonalities 0.5

Update sim-swap-check.feature with commonalities 0.5

43a76ba

Update sim-swap-check.feature with commonalities 0.5

Update sim-swap-retrieveDate.feature with Commonalities 0.5

178f304

bigludo7 requested a review from fernandopradocabrillo as a code owner December 23, 2024 15:51

Fix MegaLinter issues for the examples

8d3ccaa

gregory1g suggested changes Jan 6, 2025

View reviewed changes

bigludo7 added 2 commits January 6, 2025 18:34

Add precision that phoneNumber is used to identify the device

0b7a44e

Removed 403 INVALID_TOKEN_CONTEXT

393e05f

removed 403 INVALID_TOKEN_CONTEXT

gregory1g reviewed Jan 9, 2025

View reviewed changes

bigludo7 added 4 commits January 9, 2025 10:11

Update sim-swap.yaml

ce7b6ca

Fixe line 47 (device -> phone number)

Align 429 message examples with the code

57cbe1f

Improve GENERIC_422_MISSING_IDENTIFIER description

dfa475e

reworked MISSING_IDENTIFIER TC

25752f6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sim swap alignement with commonalities 0.5 #180

Sim swap alignement with commonalities 0.5 #180

bigludo7 commented Dec 23, 2024

github-actions bot commented Dec 23, 2024 •

edited

Loading

gregory1g Jan 6, 2025

bigludo7 Jan 6, 2025

bigludo7 Jan 6, 2025

gregory1g Jan 6, 2025

bigludo7 Jan 6, 2025

gregory1g Jan 7, 2025

bigludo7 Jan 7, 2025

gregory1g Jan 6, 2025

bigludo7 Jan 6, 2025

gregory1g Jan 7, 2025

bigludo7 commented Jan 9, 2025

gregory1g Jan 9, 2025

bigludo7 Jan 9, 2025

bigludo7 Jan 9, 2025

gregory1g Jan 9, 2025

bigludo7 Jan 9, 2025

gregory1g Jan 9, 2025

gregory1g Jan 10, 2025

bigludo7 Jan 10, 2025

bigludo7 Jan 10, 2025

gregory1g Jan 9, 2025

bigludo7 Jan 9, 2025

gregory1g Jan 9, 2025

gregory1g Jan 10, 2025

bigludo7 Jan 10, 2025

bigludo7 Jan 10, 2025

bigludo7 Jan 10, 2025


		This specification defines the `phoneNumber` field as optional in API requests, specifically in cases where the API is accessed using a 3-legged access token, and the phone number can be uniquely identified by the token. This approach simplifies API usage for API consumers by relying on the information associated with the access token used to invoke the API.
		This API requires the API consumer to identify a device as the subject of the API as follows:

Sim swap alignement with commonalities 0.5 #180

Are you sure you want to change the base?

Sim swap alignement with commonalities 0.5 #180

Conversation

bigludo7 commented Dec 23, 2024

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for reviewers:

Changelog input

Additional documentation

github-actions bot commented Dec 23, 2024 • edited Loading

🦙 MegaLinter status: ✅ SUCCESS

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bigludo7 commented Jan 9, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 23, 2024 •

edited

Loading