Telefonica test cases proposal #70

fernandopradocabrillo · 2023-11-08T18:01:49Z

What type of PR is this?

Add one of the following kinds:

tests

What this PR does / why we need it:

Include an alternative proposal from Telefónica for the Sim swap ATP

Which issue(s) this PR fixes:

Fixes #63

Special notes for reviewers:

Alternative proposal to #68 from @trehman-gsma
Specifies a similar format but different enough to be placed in a new PR so that it can be easily reviewed.

Changelog input

Created /code/Test_definitions/check_simswap.feature containing test cases for POST /check

Created /code/Test_definitions/retrieve_simswap_date.feature containing test cases for POST /retrieve-date

bigludo7 · 2023-11-09T08:14:04Z

Thanks @fernandopradocabrillo
Adding https://github.com/patrice-conil in the loop

What about one additional test for the check when the msisdn obtained from the network in the authorization first step (and passed in the token) is distinct from the one we get in the POST payload ?

fernandopradocabrillo · 2023-11-13T10:01:49Z

Thanks @fernandopradocabrillo Adding https://github.com/patrice-conil in the loop

What about one additional test for the check when the msisdn obtained from the network in the authorization first step (and passed in the token) is distinct from the one we get in the POST payload ?

@bigludo7
Regarding that, I think we should add the error explicitly in the API, as we do in other APIs, and then add the test case as well, I agree.

Phone number cannot be deducted from access token context.(`{"code": "SIM_SWAP.INVALID_TOKEN_CONTEXT","message": "Phone number cannot be deducted from access token context"}`)

fernandopradocabrillo · 2023-11-13T16:10:00Z

@bigludo7
Approved the PR, seems ok from my side.

For the test cases I'm considering the following:

The phoneNumber in the request body doesn't match the one in the access_token
The phoneNumber is not provided in the request body and cannot be deducted from the access token
The phoneNumber is provided in the request body but cannot be deducted from the access token

sounds good?

bigludo7 · 2023-11-13T16:42:32Z

@bigludo7 Approved the PR, seems ok from my side.

For the test cases I'm considering the following:

The phoneNumber in the request body doesn't match the one in the access_token

The phoneNumber is not provided in the request body and cannot be deducted from the access token

The phoneNumber is provided in the request body but cannot be deducted from the access token

sounds good?

Yes very good. Thanks

fernandopradocabrillo · 2023-11-16T11:13:54Z

Included scenarios for HTTP 403 invalid token context

bigludo7

LGFM
Thanks @fernandopradocabrillo

code/Test_definitions/checkSimSwap.feature

trehman-gsma · 2023-12-07T00:02:06Z

Hi @fernandopradocabrillo,

Sorry I didn't see this earlier (notifications landed in an unmonitored email folder - my bad). Feedback below:

1 - For checkSimSwap, should we include a test case where phone number is missing? The retrieveSimSwapDate feature file has the test case already (@retrieveSimSwapDate_E19.102_NoPhoneNumber) so it aligns.

Sample:

    @checkSimSwap_EN.NNN_NoPhoneNumber
    Scenario: Check that the response shows an error when the phone number is missing
        Given I want to test "checkSimSwap"
        And a valid access_token
        And the request body is set to:
            """
            {}
            """
        When I request "checkSimSwap"
        Then the response status code is "400"
        And the API returns the error code "INVALID_ARGUMENT"
        And the API returns a human readable error message

2 - For checkSimSwap, 2 further test cases to confirm adherence to the maxAge range (min 1, max 2400). Samples below:

    @checkSimSwap_EN.NNN_BelowMaxAgeRange
    Scenario: Check that the response shows an error when the requested maxAge is below the minimum integer allowed (min=1)
        Given I want to test "checkSimSwap"
        And a valid access_token
        And the variable "[CONTEXT: phoneNumber]" is set to an valid phone number
        And the variable "maxAge" is set below the minimum integer allowed
        And the request body is set to:
            """
            {
              "phoneNumber": "[CONTEXT: phoneNumber]"
              "maxAge": -1
            }
            """
        When I request "checkSimSwap"
        Then the response status code is "400"
        And the API returns the error code "INVALID_ARGUMENT"
        And the API returns a human readable error message

    @checkSimSwap_EN.NNN_AboveMaxAgeRange
    Scenario: Check that the response shows an error when the requested maxAge is above the maximum integer allowed (max=2400)
        Given I want to test "checkSimSwap"
        And a valid access_token
        And the variable "[CONTEXT: phoneNumber]" is set to an valid phone number
        And the variable "maxAge" is set above the minimum integer allowed
        And the request body is set to:
            """
            {
              "phoneNumber": "[CONTEXT: phoneNumber]"
              "maxAge": 2401
            }
            """
        When I request "checkSimSwap"
        Then the response status code is "400"
        And the API returns the error code "INVALID_ARGUMENT"
        And the API returns a human readable error message

code/Test_definitions/retrieveSimSwapDate.feature

code/Test_definitions/checkSimSwap.feature

trehman-gsma · 2023-12-08T15:05:48Z

Another suggestion. To confirm adherence to the PhoneNumber pattern, have test cases where we input the phone number with a "+" prefix and without a "+" prefix.

This could be new test cases, or amend existing ones where at least one contains a "+" prefix and at least one does not contain the prefix.

(This is based on experience reviewing multiple Operator deployments - have seen that the prefix is not always supported)

… into telefonica-test-cases-proposal

fernandopradocabrillo · 2024-06-26T15:42:11Z

Hi @bigludo7 @trehman-gsma !

I have updated the test cases following Commonalities guidelines.
Take a look to check if it covers everything

bigludo7

One comment about adding 401 AUTHENTICATION_REQUIRED - this is related to the yaml but has impact on the Test Definition.
I used Location Verification TD as template.

code/Test_definitions/retrieveSimSwapDate.feature

code/Test_definitions/checkSimSwap.feature

bigludo7

LGTM

trehman-gsma · 2024-07-08T18:53:47Z

code/Test_definitions/checkSimSwap.feature

+
+  # Specific errors
+
+  @check_sim_swap_7_unknown_phone_number


Hi @fernandopradocabrillo @bigludo7,

Regarding the test cases where there is a mismatch between access token and phoneNumber - are there wider CAMARA guidelines for the expected error responses (e.g. UNKNOWN_PHONE_NUMBER / NOT_FOUND / INVALID_TOKEN_CONTEXT)? Is it based on Commonalities' "Standardized use of CAMARA error responses"?

Some of the operator implementations I've experienced return a 403 in every case where there is a mismatch between access token (they do not implement the concept of a user owning multiple phone numbers at the CAMARA level), so I'm thinking these different mismatch error responses might be a little difficult to adopt across operators. If there is some guidelines I could point operators towards for these cases then that would be helpful.

I don't think there is a clear guideline on this I'm afraid, but I'll check with my collegues if they have more info.

I have included our view on the scenarios and this is what we are implementing in TEF. We have a more complex filtering structure in the apigw so we are able to differenciate between these cases.

Maybe we can place a comment and clarify that depending on the auth implementations, the operator can return 404 or 403.

Edit: as stated in the issue #119 we are going to remove the unkown_phon_ number error

code/Test_definitions/checkSimSwap.feature

trehman-gsma · 2024-07-08T19:18:33Z

code/Test_definitions/checkSimSwap.feature

+    And the response property "$.code" is "INVALID_TOKEN_CONTEXT"
+    And the response property "$.message" contains a user friendly text
+
+  @check_sim_swap_12_phone_number_conflict


Hi @fernandopradocabrillo,

Apologies, I'm not able to comprehend this test case. Does it mean sending 2 requests using the same access token? If so, how does this result in 409?

Hi @trehman-gsma!

I included this test just to cover all the errors defined in the API but, actually, I don't see any use for it.
I have an open issue to discuss whether we should remove it or not #119

code/Test_definitions/checkSimSwap.feature

trehman-gsma · 2024-07-08T19:33:01Z

Hi @bigludo7 @trehman-gsma !

I have updated the test cases following Commonalities guidelines. Take a look to check if it covers everything

Hi @fernandopradocabrillo - great job with the test cases. I have added a few comments to the changed files to request clarification. Thanks.

gregory1g · 2024-07-25T09:50:40Z

code/Test_definitions/checkSimSwap.feature

+    And the response property "$.message" contains a user friendly text
+
+  @check_sim_swap_8_phone_number_provided_does_not_belong_to_the_user
+  Scenario: Error when provided phone number does not belong to the user


To which "the user" the phone number must belong? The client does not provide a user in the input.
Th test case logic is correct: "the header "Authorization" is set to a valid access token emitted for a different phone number" => access token does not matches provided phoenNumber (both can belong to the same person or nor - does not matter).
Suggest: adjust the name and scenario of the test to something like "access token does not match phone number".

For us this is a step we have prior to any other validation.
To be a valid token, the phoneNumber needs to be related to a subscriber, so we have a validation to check that indeed the phoneNumber belongs to the subscriber.
I can remove this test to not block the release, but maybe we'll like to revisit the possibility to include it in the future.

yes, let's remove this for now.

to be honest, I do not fully understand how this could work. To check that phoneNumber belongs to the user, you need to get the user (user id) from somewhere. But user ID is not part of the input. Probably, you can find the user by access token, but access token is already for the exact phone number anyway. So, if the access token is generated correctly user id amd phone number "in" it must match...

code/Test_definitions/checkSimSwap.feature

code/Test_definitions/retrieveSimSwapDate.feature

gregory1g · 2024-07-25T10:16:06Z

code/Test_definitions/checkSimSwap.feature

+  # This first scenario serves as a minimum, not testing any specific verificationResult
+  @check_sim_swap_1_generic_success_scenario
+  Scenario: Common validations for any sucess scenario
+    Given the request body property "$.phoneNumber" is set to a valid testing phoneNumber


success test cases (all of them) only cover scenario where $.phoneNumber is provided. However, the recommended way to use the API is without phoneNumber in the payload. And, it looks like there is no single test case which covers this.

yes, I would keep the specification about the phone number tho. For example:

And the header "Authorization" is set to a valid access token from which a valid testing phoneNumber can be deducted

wdyt? @gregory1g

This still keeps the main usage scenario outside of the tests coverage. With this change the only valid shown success invocation includes both access token and phoneNumber, meanwhile the recommended way to do not send phoneNumber. Probably we should focus successful tests on access token identifcation only and only keep error tests when phoneNumber does not match access token?

Yes, I think I haven't expressed it correctly. I agree with removing the phoneNumber from the body in the success scenarios, but I think we should keep the phone number characteristics for each case.

It is important for example to state that the phone number associated to the token didn't have a swap in the last X months or that there has been a swap but outside of the maxAge, etc

This way the tests can be set properly for the certification.

Fully agree. phoneNumber is still needed for the service logic. But for the main success cases, it is better to move a source of this information from the request body ("$.phoneNumber" reference request body) to access token.
No sure what is the best way to express this. Probably like:

@check_sim_swap_1_generic_success_scenario Scenario: Common validations for any sucess scenario Given the request header "Authorization" is set to a valid access token from which a valid testing phoneNumber can be deducted . . .

Yep, that sounds like what I had in mind, I'll update it

gregory1g · 2024-07-25T10:18:25Z

code/Test_definitions/retrieveSimSwapDate.feature

+  # Scenarios testing specific situations
+
+  @retrieve_sim_swap_date_2_valid_sim_swap
+  Scenario: Check SIM swap date for a valid SIM swap


Should we probably use "retrieve" instead of "check" here (and for the following test cases)?

code/Test_definitions/retrieveSimSwapDate.feature

+    Then the response status code is 200
+    And the response property "$.latestSimChange" contains a valid timestamp
+
+  @retrieve_sim_swap_date_3_no_sim_swap_returns_activation_date


gregory1g · 2024-07-25T10:23:57Z

code/Test_definitions/retrieveSimSwapDate.feature

+  # This scenario applies when there is a local regulation with a time limitation on the information that can be returned
+  @retrieve_sim_swap_date_5_no_sim_swap_or_activation_date_due_to_legal_constrain
+  Scenario: Check SIM swap date for a valid SIM swap
+    Given the request body property "$.phoneNumber" is set to a valid testing phone number


Given the request body property "$.phoneNumber" is set to a valid testing phone number
=>
Given the request body property "$.phoneNumber" is set to a valid testing phone number and SimSwap event happened before the limited history window threshold.

gregory1g · 2024-07-25T10:25:07Z

code/Test_definitions/retrieveSimSwapDate.feature

+    And the response property "$.code" is "NOT_SUPPORTED"
+    And the response property "$.message" contains a user friendly text
+
+  @retrieve_sim_swap_date_7_phone_number_provided_does_not_belong_to_the_user


same concern as for the corresponding /check tests

sfnuser · 2024-07-29T13:52:27Z

code/Test_definitions/retrieveSimSwapDate.feature

+    When the request "retrieveSimSwapDate" is sent
+    Then the response status code is 422
+    And the response property "$.status" is 422
+    And the response property "$.code" is "NOT_SUPPORTED"


I wonder how we can standardise the usage of code values across implementations? Here we have chosen NOT_SUPPORTED but the spec does not mandate this (it provides only generic error definitions with no specific values - only examples). I can't remember why we chose this approach. This applies to other CAMARA APIs too.

Also Commonalities seems to recommend the below error code

The use of examples is something that comes from the beginning of CAMARA, it is the same in all APIs, I also agree it should be included as generic definition and not only examples.

Regarding the error, we haven't use that one since it only applies to APIs that use the device object. There is an agreement in Commonalities to use the NOT_SUPPORTED error. It is stated for example in this commonalities PR. Probably also in the meeting minutes.

@fernandopradocabrillo Thanks for the references 👍🏼

fernandopradocabrillo · 2024-07-31T13:50:48Z

@gregory1g I have updated the scenarios including your comments and the new UNIDENTIFIABLE_PHONE_NUMBER error. Thanks!

gregory1g

Most of the my comments are non-blocking nice to have improvement suggestions.
The one potentially blocking one is about "@retrieve_sim_swap_date_9_phone_number_not_provided_and_cannot_be_deducted_from_access_token" and "@retrieve_sim_swap_date_10_phone_number_not_provided_and_cannot_be_deducted_from_access_token"

gregory1g · 2024-08-01T05:42:06Z

code/Test_definitions/checkSimSwap.feature

+
+  @check_sim_swap_3_valid_sim_swap_max_age
+  Scenario Outline: Check that the response shows that the SIM has been swapped
+    Given the request header "Authorization" is set to a valid access token from which a phone number connected to the Operator's network that has been swapped in the last "<hours>" hours, where "<hours>" is equal or less than provided "maxAge" request body parameter can be deducted


Is it possible to split tis very long "given" into more "standard" parts like:
Given the request header "Authorization" is set to a valid access token from which a phone number connected to the Operator's network can be deducted
AND $.maxAge is set to "X"
AND SIM for this phone number was swapped in the last "X" hours

@check_sim_swap_5_out_of_max_age is rather close to this already

Similar suggestion for other use cases where "given" combines multiple conditions

Agree, I have split all the success scenarios into different steps, hope it is better now

gregory1g · 2024-08-01T05:55:50Z

code/Test_definitions/checkSimSwap.feature

+
+  @check_sim_swap_5_out_of_max_age
+  Scenario: Check that the response shows that the SIM has not been swapped when the last swap was before the maxAge field
+    Given the request header "Authorization" is set to a valid access token from which a phone number connected to the network whose last SIM swap was more than "$.maxAge" hours ago can be deducted


Suggestion: extract "swap was more than X" in a separate AND condition. And put it after the "maxAge is set" condition to simplify reading. Like:
Given phoneNumber is here
and maxAge is set like ....
and swap was at maxAge+1 ago.

Currently we say:

swap was more than $.maxAge hoursAgo

$.maxAge is set to to swap -1
so we define maxAge via simswap time, and at the same time we define simswap time via maxAge,

gregory1g · 2024-08-01T06:21:49Z

code/Test_definitions/retrieveSimSwapDate.feature

+  #
+  # References to OAS spec schemas refer to schemas specifies in sim_swap.yaml, version 1.0.0
+
+  Get timestamp of last MSISDN <-> IMSI pairing change for a mobile user account provided with MSIDN.


Looks a bit misleading: we do not user user accounts in camara
Suggestion:
Get timestamp of last MSISDN <-> IMSI pairing change for the provided phone number.

Yeah, I basically used the endpoint description. I don't see any problem in changing it to the one you proposed.
We can update the description in the yaml in the future

gregory1g · 2024-08-01T06:28:02Z

code/Test_definitions/retrieveSimSwapDate.feature

+    And the response property "$.code" is "INVALID_TOKEN_CONTEXT"
+    And the response property "$.message" contains a user friendly text
+
+  @retrieve_sim_swap_date_10_phone_number_not_provided_and_cannot_be_deducted_from_access_token


conditions looks identical to @retrieve_sim_swap_date_9_phone_number_not_provided_and_cannot_be_deducted_from_access_token, but the response is different.

totally, I forgot to remove the old one, fixed!

sfnuser · 2024-08-01T07:45:08Z

code/Test_definitions/checkSimSwap.feature

+    Given the request body property "$.phoneNumber" is set to a phone number for which the service is not available
+    When the request "checkSimSwap" is sent
+    Then the response status code is 422
+    And the response property "$.status" is 422


Aren't these error returns depends on the deployment architecture? For e.g. when the service is not supported for the phone number, my ID server may not even issue an access token and can return either 401 or 403. Should we have a list of possible error codes?

Yeah, we have face a similar situation with one of the test cases above. I think this is something we'll have to discuss and decide how we can approach it.
For example, for us in Telefónica, we can differenciate if a phone number belongs to a suscriptor and return a different error when the provided phone number doesn't belong to the user associated to the token.
Maybe you can open an issue and we can discuss there, but it won't be part of this release I believe.

We also encountered similar scenario during GSMA certification test run as these test cases form the basis of the interoperability tests. I agree, this may not be something that can be done in this release and it may be applicable to other APIs as well. I will raise an issue here first - it can be tracked for the next release 👍🏼

gregory1g

lgfm

bigludo7

LGTM

initial proposal

c2b2c47

fernandopradocabrillo self-assigned this Nov 8, 2023

fernandopradocabrillo requested review from DT-DawidWroblewski and bigludo7 as code owners November 8, 2023 18:01

fernandopradocabrillo mentioned this pull request Nov 8, 2023

Submission of test cases #68

Closed

fernandopradocabrillo requested a review from trehman-gsma November 8, 2023 18:27

bigludo7 mentioned this pull request Nov 13, 2023

Add 403 example for Invalid Token Context #71

Closed

include 403 invalid token context test cases

4b64570

remove version date temporary

16a686c

bigludo7 previously approved these changes Nov 16, 2023

View reviewed changes

bigludo7 reviewed Nov 17, 2023

View reviewed changes

code/Test_definitions/checkSimSwap.feature Outdated Show resolved Hide resolved

bigludo7 self-requested a review November 20, 2023 09:54

fix scenario

e1f7dbb

fernandopradocabrillo dismissed bigludo7’s stale review via e1f7dbb November 20, 2023 15:49

bigludo7 mentioned this pull request Nov 30, 2023

Test definitions - implementations guidelines and artifacts camaraproject/Commonalities#94

Closed

trehman-gsma reviewed Dec 7, 2023

View reviewed changes

code/Test_definitions/retrieveSimSwapDate.feature Outdated Show resolved Hide resolved

trehman-gsma reviewed Dec 7, 2023

View reviewed changes

code/Test_definitions/retrieveSimSwapDate.feature Outdated Show resolved Hide resolved

trehman-gsma reviewed Dec 7, 2023

View reviewed changes

code/Test_definitions/checkSimSwap.feature Outdated Show resolved Hide resolved

fernandopradocabrillo added 2 commits June 26, 2024 17:36

Merge branch 'main' of https://github.com/fernandopradocabrillo/SimSwap…

d463d55

… into telefonica-test-cases-proposal

include test cases with commonalities format

fca89f2

fernandopradocabrillo requested review from trehman-gsma and removed request for DT-DawidWroblewski June 26, 2024 15:42

bigludo7 reviewed Jul 2, 2024

View reviewed changes

code/Test_definitions/retrieveSimSwapDate.feature Show resolved Hide resolved

code/Test_definitions/checkSimSwap.feature Show resolved Hide resolved

bigludo7 self-requested a review July 5, 2024 08:21

bigludo7 previously approved these changes Jul 5, 2024

View reviewed changes

trehman-gsma reviewed Jul 8, 2024

View reviewed changes

code/Test_definitions/checkSimSwap.feature Outdated Show resolved Hide resolved

trehman-gsma reviewed Jul 8, 2024

View reviewed changes

code/Test_definitions/checkSimSwap.feature Show resolved Hide resolved

trehman-gsma mentioned this pull request Jul 8, 2024

Submission of test cases (Gherkin feature files) #63

Closed

bigludo7 mentioned this pull request Jul 24, 2024

Add MonitoredPeriod into the API Response if a Telco cannot show the date / data for for Privacy Reasons #124

Open

update test cases

c720872

fernandopradocabrillo dismissed bigludo7’s stale review via c720872 July 24, 2024 15:47

fernandopradocabrillo requested review from bigludo7, gregory1g and trehman-gsma July 24, 2024 15:58

gregory1g reviewed Jul 25, 2024

View reviewed changes

sfnuser reviewed Jul 29, 2024

View reviewed changes

update scenarios

ab5cccd

fernandopradocabrillo requested a review from gregory1g July 31, 2024 13:50

gregory1g reviewed Aug 1, 2024

View reviewed changes

sfnuser reviewed Aug 1, 2024

View reviewed changes

rephrase GIven conditions to improve readability

afb2a03

fernandopradocabrillo requested a review from gregory1g August 1, 2024 14:06

gregory1g approved these changes Aug 2, 2024

View reviewed changes

bigludo7 approved these changes Aug 19, 2024

View reviewed changes

bigludo7 merged commit 55e2703 into camaraproject:main Aug 21, 2024
1 check passed

Telefonica test cases proposal #70

Telefonica test cases proposal #70

Conversation

fernandopradocabrillo commented Nov 8, 2023 • edited Loading

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

bigludo7 commented Nov 9, 2023

fernandopradocabrillo commented Nov 13, 2023

fernandopradocabrillo commented Nov 13, 2023

bigludo7 commented Nov 13, 2023

fernandopradocabrillo commented Nov 16, 2023

bigludo7 left a comment

Choose a reason for hiding this comment

trehman-gsma commented Dec 7, 2023 • edited Loading

trehman-gsma commented Dec 8, 2023 • edited Loading

fernandopradocabrillo commented Jun 26, 2024

bigludo7 left a comment

Choose a reason for hiding this comment

bigludo7 left a 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

trehman-gsma commented Jul 8, 2024

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

fernandopradocabrillo Jul 26, 2024 • edited Loading

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

This comment was marked as resolved.

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

fernandopradocabrillo commented Jul 31, 2024

gregory1g left a 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

gregory1g left a comment

Choose a reason for hiding this comment

bigludo7 left a comment

Choose a reason for hiding this comment

fernandopradocabrillo commented Nov 8, 2023 •

edited

Loading

trehman-gsma commented Dec 7, 2023 •

edited

Loading

trehman-gsma commented Dec 8, 2023 •

edited

Loading

fernandopradocabrillo Jul 26, 2024 •

edited

Loading