Add Test Definition for OTPValidationAPI #61

bigludo7 · 2024-05-31T07:59:50Z

Add Test Definition for OTPValidationAPI

What type of PR is this?

Add one of the following kinds:

enhancement/feature

What this PR does / why we need it:

Add Test Definition for OTPValidationAPI

Which issue(s) this PR fixes:

Fixes #57

Special notes for reviewers:

Need agreement on camaraproject/Commonalities#203 before to be fully validated
Adding @jlurien as reviewer to check alignement with the guideline.

Changelog input

 release-note

Additional documentation

This section can be blank.

docs

Add Test Definition for OTPValidationAPI

github-actions · 2024-05-31T08:01:00Z

🦙 MegaLinter status: ✅ SUCCESS

Descriptor	Linter	Files	Errors	Elapsed time
✅ ACTION	actionlint	2	0	0.03s
✅ OPENAPI	spectral	1	0	1.66s
✅ REPOSITORY	git_diff	yes	no	0.01s
✅ REPOSITORY	secretlint	yes	no	0.7s
✅ YAML	yamllint	1	0	0.3s

See detailed report in MegaLinter reports

MegaLinter is graciously provided by

fernandopradocabrillo

@bigludo7
I think the scenarios that are included are fine. I kind of like it more separated in 2 different feature files one for each operation but I can live with it being in only one 😄

I leave you a list with a few scenarios that I think are missing and if we target for an stable version we should include them.

/send-code:

Reached max number of codes that can be requested for a phone number -> ONE_TIME_PASSWORD_SMS.MAX_OTP_CODES_EXCEEDED
The phone number specified is not allowed to receive messages -> ONE_TIME_PASSWORD_SMS.PHONE_NUMBER_NOT_ALLOWED
The phone number specified does not belong to the operator -> ONE_TIME_PASSWORD_SMS.PHONE_NUMBER_NOT_ALLOWED
The phone number specified is a landline -> ONE_TIME_PASSWORD_SMS.PHONE_NUMBER_NOT_ALLOWED
The phone number specified is blocked for any business reason -> ONE_TIME_PASSWORD_SMS.PHONE_NUMBER_BLOCKED
The property message is missing -> INVALID_ARGUMENT
The property phone number is missing -> INVALID_ARGUMENT
The message specified reached a max length stablished by the operator -> INVALID_ARGUMENT

/validate-code:

Verification expired because a new one has been requested for the same phone number -> ONE_TIME_PASSWORD_SMS.VERIFICATION_EXPIRED
The authenticationId is invalid (i.e because the client sends invalid characters) -> ONE_TIME_PASSWORD_SMS.INVALID_OTP
The authenticationId is no longer valid because it has already been used -> ONE_TIME_PASSWORD_SMS.VERIFICATION_EXPIRED
Missing authenticationId in the request -> INVALID_ARGUMENT

code/Test_Definitions/OTPvalidationAPI.feature

Co-authored-by: Fernando Prado Cabrillo <fernando.pradocabrillo@telefonica.com>

@fernandopradocabrillo

Review file after @fernandopradocabrillo review

bigludo7 · 2024-07-08T17:18:28Z

Thanks @fernandopradocabrillo for the review.

I've added these scenarios but also I've reorganized the file to make it easier to read.
I have one point point: 403 did not seems to be relevant for validate-code operation. wdyt?

trehman-gsma · 2024-07-10T16:23:04Z

code/Test_Definitions/OTPvalidationAPI.feature

+    Given the header "Authorization" is removed
+    And the resource "/one-time-password-sms/v0/send-code"
+    When the HTTP "POST" request is sent
+    Then the response property "$.status" is 403


Should this be a 401 instead of a 403?

Yes ! fixed.

trehman-gsma · 2024-07-10T16:42:49Z

code/Test_Definitions/OTPvalidationAPI.feature

+#  404 errors for send_code
+###########################
+
+  @OTPvalidationAPI_404.1_send_code_resource_not_found


Hi @bigludo7 - I think this might be challenging for some Operators to implement. Are there wider guidelines that state that sub-paths should be treated with CAMARA error handling?

I think some implementations will expose only specific paths/routes in their API Gateway e.g. only /one-time-password-sms/v0/send-code is explicitly exposed as a CAMARA API, and anything else is considered an arbitrary URL. In this test case, /one-time-password-sms/send-code might be considered an arbitrary URL and therefore CAMARA error handling is not applied.

Yes make sense. I've added a comment that this Test Case could be considered as optional.
As for this API we target stable maturity level I tend to prefer to keep it as the testing guideline indicates: Any public release must include a full test plan, covering both sunny and rainy day scenarios, to be considered stable. This may include unusual or not explicitly specified error scenarios, semantic validations, and corner cases.
We're in a corner case here probably

trehman-gsma · 2024-07-10T17:02:30Z

code/Test_Definitions/OTPvalidationAPI.feature

+    And the response property "$.message" contains a user friendly text
+    And the response header "x-correlator" has same value as the request header "x-correlator"
+
+  @OTPvalidationAPI_400.3_validate_code_missing_authenticationId


I think this is a duplicate title. Should it be named @OTPvalidationAPI_400.3_validate_code_missing_code instead?

trehman-gsma · 2024-07-10T17:06:55Z

code/Test_Definitions/OTPvalidationAPI.feature

+    And the response property "$.code" is "INVALID_ARGUMENT"
+    And the response property "$.message" contains a user friendly text
+    And the response header "x-correlator" has same value as the request header "x-correlator"
+


What do you think about an additional case to test compliance with the maxLength of the code field?
Example:

@OTPvalidationAPI_400.4_validate_code_exceed_code_max_length Scenario: exceed the maxLength for code Given request body property "$.authenticationId" is set to the value from send-code request And the request body property "$.code" is set to "thisCodeExceedsTenCharacters" And the resource "/one-time-password-sms/v0/validate-code" When the HTTP "POST" request is sent Then the response status code is 400 And the response property "$.status" is 400 And the response property "$.code" is "INVALID_ARGUMENT" And the response property "$.message" contains a user friendly text And the response header "x-correlator" has same value as the request header "x-correlator"

Yes !
Added

trehman-gsma · 2024-07-10T17:12:32Z

code/Test_Definitions/OTPvalidationAPI.feature

+    And the response header "x-correlator" has same value as the request header "x-correlator" 
+
+###########################
+#  401 errors for send_code


Just an observation about the 401 test cases. There is no x-correlator validation in the 401 test cases whereas the other test cases in the document do check for x-correlator.

On the same topic. Should there be happy path test cases that do not input an x-correlator? This would be to ensure that x-correlator is treated an optional header in the implementation.

I've added the x-correlator for 401

Adding testing without x-correlator could be added as this is not mandatory. I've added them. I did not test it in response as it is optional to provide one if not provided in the request.

trehman-gsma · 2024-07-10T17:17:03Z

code/Test_Definitions/OTPvalidationAPI.feature

+#     *For additional test scenarios, operator could provide phone number for line that cannot receive SMS, line that blocked SMS reception, not belonging to the operator, designing a landline number.
+# * message: Message template used to compose the content of the SMS sent to the phone number. It must include the following label indicating where to include the short code {{code}}. Operator could specified a max_lenght for the message.
+# * authentication_id: identifier of a a send-code request. Identifier retrieved in send-code response.
+# * max_sent: Maximum allowed request to send code for a given device


I have a silly question. Is it theoretically possible for an implementation to not have max_sent and max_try functionality?

If max_sent and max_try are mandatory, should the yaml documentation state so?

From GSMA experience, I think there will be some operator developers who will not code this unless it's explicitly stated :)

For me globally the functionality to burn a code is mandatory but we should probably not enforce a way to do it. Here we have max try but another rule stating that you have 5 minutes to send back the code before expiration is fine as well. So I'm not sure we have to enter in this details.

Took into Toyeeb comments ! Thanks !!

bigludo7

Thanks @trehman-gsma !!
This is very helpful.
I took into consideration all your comments.

bigludo7 · 2024-07-10T19:57:37Z

code/Test_Definitions/OTPvalidationAPI.feature

+#     *For additional test scenarios, operator could provide phone number for line that cannot receive SMS, line that blocked SMS reception, not belonging to the operator, designing a landline number.
+# * message: Message template used to compose the content of the SMS sent to the phone number. It must include the following label indicating where to include the short code {{code}}. Operator could specified a max_lenght for the message.
+# * authentication_id: identifier of a a send-code request. Identifier retrieved in send-code response.
+# * max_sent: Maximum allowed request to send code for a given device


For me globally the functionality to burn a code is mandatory but we should probably not enforce a way to do it. Here we have max try but another rule stating that you have 5 minutes to send back the code before expiration is fine as well. So I'm not sure we have to enter in this details.

bigludo7 · 2024-07-10T20:05:43Z

code/Test_Definitions/OTPvalidationAPI.feature

+    And the response header "x-correlator" has same value as the request header "x-correlator" 
+
+###########################
+#  401 errors for send_code


I've added the x-correlator for 401

Adding testing without x-correlator could be added as this is not mandatory. I've added them. I did not test it in response as it is optional to provide one if not provided in the request.

bigludo7 · 2024-07-10T20:07:01Z

code/Test_Definitions/OTPvalidationAPI.feature

+    Given the header "Authorization" is removed
+    And the resource "/one-time-password-sms/v0/send-code"
+    When the HTTP "POST" request is sent
+    Then the response property "$.status" is 403


Yes ! fixed.

bigludo7 · 2024-07-10T20:12:09Z

code/Test_Definitions/OTPvalidationAPI.feature

+    And the response property "$.message" contains a user friendly text
+    And the response header "x-correlator" has same value as the request header "x-correlator"
+
+  @OTPvalidationAPI_400.3_validate_code_missing_authenticationId


bigludo7 · 2024-07-10T20:13:18Z

code/Test_Definitions/OTPvalidationAPI.feature

+    And the response property "$.code" is "INVALID_ARGUMENT"
+    And the response property "$.message" contains a user friendly text
+    And the response header "x-correlator" has same value as the request header "x-correlator"
+


Yes !
Added

bigludo7 · 2024-07-10T20:16:10Z

code/Test_Definitions/OTPvalidationAPI.feature

+#  404 errors for send_code
+###########################
+
+  @OTPvalidationAPI_404.1_send_code_resource_not_found


Yes make sense. I've added a comment that this Test Case could be considered as optional.
As for this API we target stable maturity level I tend to prefer to keep it as the testing guideline indicates: Any public release must include a full test plan, covering both sunny and rainy day scenarios, to be considered stable. This may include unusual or not explicitly specified error scenarios, semantic validations, and corner cases.
We're in a corner case here probably

Shift 403 to 404 when phoneNumber did not belong to the operator

For test case @OTPvalidationAPI_404.1_send_code_phone_number_not_belong_to_operator

code/Test_Definitions/OTPvalidationAPI.feature

Co-authored-by: Fernando Prado Cabrillo <fernando.pradocabrillo@telefonica.com>

fernandopradocabrillo

LGTM

Thanks @bigludo7 @trehman-gsma !

Add Test Definition for OTPValidationAPI

bc8b024

Add Test Definition for OTPValidationAPI

bigludo7 requested a review from jlurien May 31, 2024 07:59

bigludo7 requested a review from fernandopradocabrillo as a code owner May 31, 2024 07:59

bigludo7 added the Fall24 Fall 24 Meta release label Jun 19, 2024

fernandopradocabrillo reviewed Jul 5, 2024

View reviewed changes

code/Test_Definitions/OTPvalidationAPI.feature Outdated Show resolved Hide resolved

code/Test_Definitions/OTPvalidationAPI.feature Outdated Show resolved Hide resolved

bigludo7 and others added 3 commits July 8, 2024 16:56

Update code/Test_Definitions/OTPvalidationAPI.feature

ffbd4ec

Co-authored-by: Fernando Prado Cabrillo <fernando.pradocabrillo@telefonica.com>

Update code/Test_Definitions/OTPvalidationAPI.feature

b2fa564

Co-authored-by: Fernando Prado Cabrillo <fernando.pradocabrillo@telefonica.com>

Update OTPvalidationAPI.feature

cce1fb9

Review file after @fernandopradocabrillo review

trehman-gsma reviewed Jul 10, 2024

View reviewed changes

Update OTPvalidationAPI.feature

e866709

Took into Toyeeb comments ! Thanks !!

bigludo7 commented Jul 10, 2024

View reviewed changes

bigludo7 added 3 commits July 11, 2024 14:49

Shift 403 to 404 when phoneNumber did not belong to the operator

4753fab

Shift 403 to 404 when phoneNumber did not belong to the operator

Update OTPvalidationAPI.feature

93a7e07

Shift DEVIVE_NOT_FOUND to NOT_FOUND

3565d60

For test case @OTPvalidationAPI_404.1_send_code_phone_number_not_belong_to_operator

fernandopradocabrillo self-requested a review July 19, 2024 09:33

fernandopradocabrillo reviewed Jul 24, 2024

View reviewed changes

code/Test_Definitions/OTPvalidationAPI.feature Outdated Show resolved Hide resolved

fernandopradocabrillo reviewed Jul 24, 2024

View reviewed changes

code/Test_Definitions/OTPvalidationAPI.feature Outdated Show resolved Hide resolved

bigludo7 and others added 2 commits July 24, 2024 18:05

Update code/Test_Definitions/OTPvalidationAPI.feature

07d8e33

Co-authored-by: Fernando Prado Cabrillo <fernando.pradocabrillo@telefonica.com>

Update code/Test_Definitions/OTPvalidationAPI.feature

da34cd6

Co-authored-by: Fernando Prado Cabrillo <fernando.pradocabrillo@telefonica.com>

fernandopradocabrillo approved these changes Jul 24, 2024

View reviewed changes

bigludo7 merged commit c98627b into main Jul 25, 2024
1 check passed

bigludo7 deleted the Test-Defintion branch July 25, 2024 06:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Test Definition for OTPValidationAPI #61

Add Test Definition for OTPValidationAPI #61

bigludo7 commented May 31, 2024

github-actions bot commented May 31, 2024 •

edited

Loading

fernandopradocabrillo left a comment

bigludo7 commented Jul 8, 2024

trehman-gsma Jul 10, 2024

bigludo7 Jul 10, 2024

trehman-gsma Jul 10, 2024 •

edited

Loading

bigludo7 Jul 10, 2024

trehman-gsma Jul 10, 2024

bigludo7 Jul 10, 2024

trehman-gsma Jul 10, 2024

bigludo7 Jul 10, 2024

trehman-gsma Jul 10, 2024

bigludo7 Jul 10, 2024

trehman-gsma Jul 10, 2024 •

edited

Loading

bigludo7 Jul 10, 2024

bigludo7 left a comment

bigludo7 Jul 10, 2024

bigludo7 Jul 10, 2024

bigludo7 Jul 10, 2024

bigludo7 Jul 10, 2024

bigludo7 Jul 10, 2024

bigludo7 Jul 10, 2024

fernandopradocabrillo left a comment

Add Test Definition for OTPValidationAPI #61

Add Test Definition for OTPValidationAPI #61

Conversation

bigludo7 commented May 31, 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 May 31, 2024 • edited Loading

🦙 MegaLinter status: ✅ SUCCESS

fernandopradocabrillo left a comment

Choose a reason for hiding this comment

bigludo7 commented Jul 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trehman-gsma Jul 10, 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

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 Jul 10, 2024 • edited Loading

Choose a reason for hiding this 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fernandopradocabrillo left a comment

Choose a reason for hiding this comment

github-actions bot commented May 31, 2024 •

edited

Loading

trehman-gsma Jul 10, 2024 •

edited

Loading

trehman-gsma Jul 10, 2024 •

edited

Loading