New specific 403 token error and guidelines alignment #19

monamok · 2023-02-01T16:07:41Z

This PR is to solve Issue #18.
Please review and share your feedback.
Thank you.

The errors status will be changed also into integer.

bigludo7

Look good for me but a question: do you think same must done for SIM Swap or is it a number verification specific?

monamok · 2023-02-02T08:20:10Z

Look good for me but a question: do you think same must done for SIM Swap or is it a number verification specific?

Here we are using just 3-legged token for both endpoints but in Sim Swap we have Client Credentials as well. I'll talk to @jlurien and see how we can align the errors there. Thank you @bigludo7.

jlurien · 2023-02-02T11:27:27Z

Look good for me but a question: do you think same must done for SIM Swap or is it a number verification specific?

Here we are using just 3-legged token for both endpoints but in Sim Swap we have Client Credentials as well. I'll talk to @jlurien and see how we can align the errors there. Thank you @bigludo7.

In SIMSwap we have client_redentials and 3_legged, and also msisdn is required as input, while here I see that it is not, so for SIM Swap only would make sense to validate that, when using 3_legged, if access_token is linked to a different MSISDN than the one provided in the request body, then reject the request. We could add an error for that, but with an appropriate description.

monamok · 2023-02-02T13:14:38Z

The guideline for errors has been changed today and now status is an integer instead of a string:

camaraproject/WorkingGroups#143
camaraproject/WorkingGroups#145

I'm going to update the branch and include this change as well :)

bigludo7 · 2023-02-02T15:43:10Z

Look good for me but a question: do you think same must done for SIM Swap or is it a number verification specific?

Here we are using just 3-legged token for both endpoints but in Sim Swap we have Client Credentials as well. I'll talk to @jlurien and see how we can align the errors there. Thank you @bigludo7.

In SIMSwap we have client_redentials and 3_legged, and also msisdn is required as input, while here I see that it is not, so for SIM Swap only would make sense to validate that, when using 3_legged, if access_token is linked to a different MSISDN than the one provided in the request body, then reject the request. We could add an error for that, but with an appropriate description.

Thanks for the clarification. So probably no hurry to add this for simswap as it will be used for one specific UC and bring some confusion.

monamok · 2023-02-08T16:03:10Z

Dear @DT-DawidWroblewski @bigludo7 I also included a fix to an operationId and description that we had missed in the first version. Please review and merged if you find everything ok. Thanks.

bigludo7

Look good for me

monamok · 2023-02-14T15:04:15Z

Kindly reminder: @DT-DawidWroblewski could you please review and merge this PR?

DT-DawidWroblewski · 2023-02-15T08:08:34Z

code/API_definitions/CAMARA/NUMBER_VERIFICATION_API.md

@@ -28,7 +28,7 @@ Two endpoints are defined in Number Verification API:

 **Authentication**

-Security access keys such as OAuth 2.0 3-legged Access Tokens used by Client applications to invoke this API with dedicated scope. Client must authenticate via IP to use this service.
+Security access keys such as OAuth 2.0 3-legged Access Tokens used by Client applications to invoke this API with dedicated scope. Client **must authenticate via IP** to use this service.


GSMA IDG works currently on alternatives to data session, that is required to fulfill Number Verify. Additionally, I don't see this as a must, because some MNOs use tokens that work "like IP address", so no IP address is shared between the client and MNO.

Edited. Please check it out.

code/API_definitions/CAMARA/number_verification.yaml

DT-DawidWroblewski · 2023-02-15T08:10:49Z

code/API_definitions/CAMARA/number_verification.yaml

@@ -42,12 +42,12 @@ paths:
    post:
      tags:
        - Phone number verify
-      summary: Verifies if the received hashed phone number matches the phone number associated with the access token
+      summary: Verifies if the received hashed/clear phone number matches the phone number associated with the access token


plain text looks better than clear

Edited here and couple of more pleaces.

code/API_definitions/CAMARA/number_verification.yaml

DT-DawidWroblewski · 2023-02-15T08:18:22Z

code/API_definitions/CAMARA/number_verification.yaml

          - Client authentication was not via mobile network. In order to check the authentication method, AMR parameter value in the 3-legged user's access token can be used and make sure that the authentication was not either by SMS+OTP nor username/password (`{"code": "NUMBER_VERIFICATION.USER_NOT_AUTHENTICATED_BY_IP","message": "Client must authenticate via IP to use this service"}`)
+          - Phone number cannot be deducted from access token context.(`{"code": "NUMBER_VERIFICATION.INVALID_TOKEN_CONTEXT","message": "Phone number cannot be deducted from access token context"}`)


I'm not so convinced about the approach that we share info about the context. Can we simplify the response and use just 401 Unauthorized, invalid_token?

I think 401 is unauthenticated and in this case the user has authenticated but it's kind of forbidden because they autheticated using a wrong method for example, and so the token lacks informations that is needed to go on with the process. We're just saying that the token is not what it's supposed to be and it can be helpful for the developer. I prefer 403 to 401. Specific errors are specially to give some more information and telling why they're not allowed to do something and they're more developer-friendly in general.

monamok · 2023-02-20T14:50:09Z

Dear @DT-DawidWroblewski did you get a chance to take a look to the comments and changes that you had asked for?

DT-DawidWroblewski · 2023-02-20T15:18:53Z

code/API_definitions/CAMARA/NUMBER_VERIFICATION_API.md

 - GET /number-verification/v0/device-phone-number : Returns the phone number associated with the access token so API clients can verify the number themselves.

 **Authentication**

-Security access keys such as OAuth 2.0 3-legged Access Tokens used by Client applications to invoke this API with dedicated scope. Client must authenticate via IP to use this service.
+Security access keys such as OAuth 2.0 3-legged Access Tokens used by Client applications to invoke this API with dedicated scope. Client **must use network based authentication methods** to use this service. 


this looks good

DT-DawidWroblewski · 2023-02-20T15:19:04Z

code/API_definitions/CAMARA/NUMBER_VERIFICATION_API.md

@@ -50,7 +50,7 @@ Following table defines API endpoints of exposed REST based for Number Verificat

 | **Endpoint** | **Operation** | **Description** |
 | -------- | --------- | ----------- |
-| POST /number-verification/v0/verify | **Request to verify a number** | Create request in order to verify if the specified phone number (clear or hashed format) matches the one that the user is currently using |
+| POST /number-verification/v0/verify | **Request to verify a number** | Create request in order to verify if the specified phone number (plain text or hashed format) matches the one that the user is currently using |


DT-DawidWroblewski · 2023-02-20T15:21:53Z

code/API_definitions/CAMARA/NUMBER_VERIFICATION_API.md

@@ -61,7 +61,7 @@ Following table defines API endpoints of exposed REST based for Number Verificat
 | -------------------------- |
 | **HTTP Request**<br> POST /number-verification/v0/verify<br>**Query Parameters**<br> No query parameters are defined.<br>**Path Parameters**<br> No path parameters are defined.<br>**Request Body Parameters**<br> **One of:** <br> **phone_number**: A phone number belonging to the user. 'E164 with +' format.<br> **hashed_phone_number**: Hashed phone number. SHA-256 (in hexadecimal representation) of the mobile phone number in 'E164 with +' format.

- <br>**Response**<br> **200: OK**<br>  Response body: <br>**device_phone_number_verified** : Boolean <br> **400:** **INVALID_ARGUMENT**<br> **401:** **UNAUTHENTICATED** <br> **403:** **PERMISSION_DENIED** <br> **403:** **NUMBER_VERIFICATION.USER_NOT_AUTHENTICATED_BY_IP** <br> **500:** **INTERNAL**<br> **503:** **UNAVAILABLE**<br> **504:** **TIMEOUT**<br>
+ <br>**Response**<br> **200: OK**<br>  Response body: <br>**device_phone_number_verified** : Boolean <br> **400:** **INVALID_ARGUMENT** <br> **401:** **UNAUTHENTICATED** <br> **403:** **PERMISSION_DENIED** <br> **403:** **NUMBER_VERIFICATION.USER_NOT_AUTHENTICATED_BY_IP** <br> **403:** **NUMBER_VERIFICATION.INVALID_TOKEN_CONTEXT** <br> **500:** **INTERNAL**<br> **503:** **UNAVAILABLE**<br> **504:** **TIMEOUT**<br>


If we say that we use network authentication for Number Verify should we give 403 NUMBER_VERIFICATION.USER_NOT_AUTHENTICATED_BY_IP?

DT-DawidWroblewski · 2023-02-20T15:22:09Z

code/API_definitions/CAMARA/NUMBER_VERIFICATION_API.md

@@ -70,7 +70,7 @@ Following table defines API endpoints of exposed REST based for Number Verificat
 | -------------------------- |
 | **HTTP Request**<br> GET /number-verification/v0/device-phone-number<br>**Query Parameters**<br> No query parameters are defined.<br>**Path Parameters**<br> No path parameters are defined.<br>**Request Body Parameters**<br> No body

- <br>**Response**<br> **200: OK**<br>  Response body: <br>**device_phone_number** : The device phone number associated to the access token. 'E164 with +' format. <br> **400:** **INVALID_ARGUMENT**<br> **401:** **UNAUTHENTICATED** <br> **403:** **PERMISSION_DENIED** <br> **403:** **NUMBER_VERIFICATION.USER_NOT_AUTHENTICATED_BY_IP** <br> **500:** **INTERNAL**<br> **503:** **UNAVAILABLE**<br> **504:** **TIMEOUT**<br>
+ <br>**Response**<br> **200: OK**<br>  Response body: <br>**device_phone_number** : The device phone number associated to the access token. 'E164 with +' format. <br> **400:** **INVALID_ARGUMENT**<br> **401:** **UNAUTHENTICATED** <br> **403:** **PERMISSION_DENIED** <br> **403:** **NUMBER_VERIFICATION.USER_NOT_AUTHENTICATED_BY_IP** <br>  **403:** **NUMBER_VERIFICATION.INVALID_TOKEN_CONTEXT** <br> **500:** **INTERNAL** <br> **503:** **UNAVAILABLE**<br> **504:** **TIMEOUT**<br>


If we say that we use network authentication for Number Verify should we give 403 NUMBER_VERIFICATION.USER_NOT_AUTHENTICATED_BY_IP?

DT-DawidWroblewski · 2023-02-20T15:22:43Z

code/API_definitions/CAMARA/NUMBER_VERIFICATION_API.md

@@ -88,9 +88,10 @@ Following table provides an overview of common error names, codes, and messages
 |2	|401 |	UNAUTHENTICATED |	"Request not authenticated due to missing, invalid, or expired credentials" |
 |3	|403 |	PERMISSION_DENIED |	"Client does not have sufficient permissions to perform this action" |
 |4	|403 |	NUMBER_VERIFICATION.USER_NOT_AUTHENTICATED_BY_IP |	"Client must authenticate via IP to use this service" |


If we say that we use network authentication for Number Verify should we give 403 NUMBER_VERIFICATION.USER_NOT_AUTHENTICATED_BY_IP?

DT-DawidWroblewski · 2023-02-20T15:25:37Z

code/API_definitions/CAMARA/number_verification.yaml

          - Client authentication was not via mobile network. In order to check the authentication method, AMR parameter value in the 3-legged user's access token can be used and make sure that the authentication was not either by SMS+OTP nor username/password (`{"code": "NUMBER_VERIFICATION.USER_NOT_AUTHENTICATED_BY_IP","message": "Client must authenticate via IP to use this service"}`)
+          - Phone number cannot be deducted from access token context.(`{"code": "NUMBER_VERIFICATION.INVALID_TOKEN_CONTEXT","message": "Phone number cannot be deducted from access token context"}`)


DT-DawidWroblewski · 2023-02-20T15:25:56Z

code/API_definitions/CAMARA/number_verification.yaml

@@ -278,11 +281,11 @@ components:
      content:
        application/json:
          schema:
-            $ref: '#/components/schemas/NotAuthenticatedByIpPermissionDenied'
+            $ref: '#/components/schemas/PhoneNumberVerificationPermissionDenied'


DT-DawidWroblewski · 2023-02-20T15:26:01Z

code/API_definitions/CAMARA/number_verification.yaml

@@ -310,7 +313,7 @@ components:
          examples:
            response:
              value:
-                status: "500"
+                status: 500


DT-DawidWroblewski · 2023-02-20T15:26:10Z

code/API_definitions/CAMARA/number_verification.yaml

@@ -338,7 +341,7 @@ components:
          examples:
            response:
              value:
-                status: "503"
+                status: 503


DT-DawidWroblewski · 2023-02-20T15:26:17Z

code/API_definitions/CAMARA/number_verification.yaml

@@ -366,7 +369,7 @@ components:
          examples:
            response:
              value:
-                status: "504"
+                status: 504


bigludo7

Thanks

new specific 403 token error added

e9a2ed6

monamok requested review from DT-DawidWroblewski and bigludo7 as code owners February 1, 2023 16:07

reformat

215c723

bigludo7 previously approved these changes Feb 2, 2023

View reviewed changes

error status changed into integer to be aligned with the guidelines

21ca9cf

monamok dismissed bigludo7’s stale review via 21ca9cf February 2, 2023 15:30

status range changed

8311920

monamok changed the title ~~New specific 403 token error added~~ New specific 403 token error and guidelines alignment Feb 3, 2023

fix

8b18c18

monamok requested a review from bigludo7 February 8, 2023 16:03

bigludo7 previously approved these changes Feb 9, 2023

View reviewed changes

DT-DawidWroblewski reviewed Feb 15, 2023

View reviewed changes

monamok dismissed bigludo7’s stale review via 44400db February 15, 2023 18:47

reviewer comments applied

44400db

monamok requested a review from DT-DawidWroblewski February 15, 2023 19:39

DT-DawidWroblewski reviewed Feb 20, 2023

View reviewed changes

monamok added 2 commits February 21, 2023 16:53

specific error edited

05e4681

IP replaced by mobile network

54639c9

DT-DawidWroblewski self-requested a review February 21, 2023 16:02

bigludo7 approved these changes Feb 21, 2023

View reviewed changes

DT-DawidWroblewski merged commit 992cd25 into camaraproject:main Feb 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New specific 403 token error and guidelines alignment #19

New specific 403 token error and guidelines alignment #19

monamok commented Feb 1, 2023 •

edited

Loading

bigludo7 left a comment

monamok commented Feb 2, 2023

jlurien commented Feb 2, 2023

monamok commented Feb 2, 2023

bigludo7 commented Feb 2, 2023

monamok commented Feb 8, 2023

bigludo7 left a comment

monamok commented Feb 14, 2023

DT-DawidWroblewski Feb 15, 2023

monamok Feb 15, 2023

DT-DawidWroblewski Feb 15, 2023

monamok Feb 15, 2023

DT-DawidWroblewski Feb 15, 2023

monamok Feb 15, 2023

monamok commented Feb 20, 2023

DT-DawidWroblewski Feb 20, 2023

DT-DawidWroblewski Feb 20, 2023

DT-DawidWroblewski Feb 20, 2023

DT-DawidWroblewski Feb 20, 2023

DT-DawidWroblewski Feb 20, 2023

DT-DawidWroblewski Feb 20, 2023

DT-DawidWroblewski Feb 20, 2023

DT-DawidWroblewski Feb 20, 2023

DT-DawidWroblewski Feb 20, 2023

DT-DawidWroblewski Feb 20, 2023

bigludo7 left a comment

		- Client authentication was not via mobile network. In order to check the authentication method, AMR parameter value in the 3-legged user's access token can be used and make sure that the authentication was not either by SMS+OTP nor username/password (`{"code": "NUMBER_VERIFICATION.USER_NOT_AUTHENTICATED_BY_IP","message": "Client must authenticate via IP to use this service"}`)
		- Phone number cannot be deducted from access token context.(`{"code": "NUMBER_VERIFICATION.INVALID_TOKEN_CONTEXT","message": "Phone number cannot be deducted from access token context"}`)

New specific 403 token error and guidelines alignment #19

New specific 403 token error and guidelines alignment #19

Conversation

monamok commented Feb 1, 2023 • edited Loading

bigludo7 left a comment

Choose a reason for hiding this comment

monamok commented Feb 2, 2023

jlurien commented Feb 2, 2023

monamok commented Feb 2, 2023

bigludo7 commented Feb 2, 2023

monamok commented Feb 8, 2023

bigludo7 left a comment

Choose a reason for hiding this comment

monamok commented Feb 14, 2023

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

monamok commented Feb 20, 2023

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

Choose a reason for hiding this comment

monamok commented Feb 1, 2023 •

edited

Loading