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 content for Number Verify #2

Merged
merged 19 commits into from
Jan 5, 2023

Conversation

DT-DawidWroblewski
Copy link
Collaborator

Please find initial content for Number Verify

@monamok
Copy link
Collaborator

monamok commented Nov 21, 2022

Thank you @DT-DawidWroblewski for your proposal.
Based on the documentation you uploaded I think Telefonica’s roadmap in general is similar to yours but I would like to go through some of the concepts and make some suggestions so we can hopefully align the differences.
A question that was raised and briefly discussed during the meeting was if the MC service already exists what is the point to duplicate it in CAMARA? I think the key here, as you also mentioned at the meeting, is to simplify the API and make it easier to use globally.
The API as its name indicates, should focus on verifying the number. The authentication flow and getting the token, from our point of view should be managed outside of this API. In this case, there should be a 3-legged token as a result of user authenticating using their device IP address through the mobile network (as you mentioned WIFI connections are out of scope here). Checking token AMR value can help us to make sure that the authentication was via IP address.

About the part that we would like to include in this API, we agree with you that “Share” use case is out of scope of this API, as we definitely don’t want to share personal information with 3d parties.
For the “Match” use case, even though the issue is still open in Commonalities, we think that the nature of this API forces us to use MSISDN as input.
Having that in mind, in your proposal based on MC, there is only one endpoint and the Resource Request contains a JSON Payload with mc_claims parameter which contains the requested service value and you can send either device_msisdn or device_msisdn_hash as input. Instead of having a general endpoint as /userinfo here we would prefer to have two separate endpoints, one for device_msisdn and another for device_msisdn_hash. Our suggestion is to use something like this:

POST /verify
{
"phone_number": "+346661113334"
}


POST /verify-hashed
{
"hashed_phone_number": "32f67ab4e4312618b09cd23ed8ce41b13e095fe52b73b2e8da8ef49830e50dba"
}

The reason why we prefer to separate them is to have control over the scopes and be able to manage the permissions separately. But as a result, the suggested API remains very simple and straight forward.
About the response, we can’t find any reason to return the ‘sub’ back. The API receives the token to get the MSISDN from it. It doesn’t make sense to return token information in the response.
I agree with you that it’s a good idea to have “correlation_id” but adapting the concept to the Design Guidelines that were approved last week, we should change it to “x-correlator” which will be included as a header parameter.

In both cases the response could look like this:
{
"phone_number_verified": true
}

I also think once we get into an agreement between all the participants, we should proceed to update the document as the idea is not to follow the exact same steps as MC and for example to exclude “Share” use case from CAMARA API.

Please feel free to share your thoughts and add any comments.

@DT-DawidWroblewski
Copy link
Collaborator Author

DT-DawidWroblewski commented Nov 23, 2022 via email

@monamok
Copy link
Collaborator

monamok commented Nov 24, 2022

Thank you @DT-DawidWroblewski for the detailed explanation.
I would like to add my comments.

“… Based on current experience I can state that not every MNO is capable of making it over IP address. IP address can be confusing, because some MNOs use CNAT, so it is impossible to identify the user based on IP address that is shared on API by Service Provider/Developer (this is public IP address, NOT private). There are solutions on the market that utilize additional client-server components that delivers tokens, that identifies data session, so no IP address is shared here as well, although token can be exchanged for customer/user identity (MSISDN, subscriberId, etc.)”

I totally agree. In fact in TEF we also use CNAT and we actually use IP+port not just the IP address. What I was referring to was that the authentication must be via mobile network and as long as it’s not based on user/password or SMS+OTP it’ll be considered in the scope.

The “Share” option is related to issue #101 that is still under discussion. TEF is strongly against returning PI and sharing that information with 3d-parties. Maybe I got a wrong idea but I think we discussed that point at the meeting and all of us were at the same page about it that we were going to remove this functionality from the CAMARA API’s scope. In any case, if we should consider this UC we can include it as a separate endpoint so its use can be limited by its individual scope.

About having a general endpoint, this was raised by you in issue #105 and TEF’s approach is similar to Orange, as Ruben our colleague also expressed there. We prefer to have individual endpoints/resources per API and of course their own corresponding scopes.

As it is always easier to review a written version I’m going to open a PR including the suggested version.

@monamok monamok mentioned this pull request Nov 28, 2022
@DT-DawidWroblewski
Copy link
Collaborator Author

@monamok
I think we're getting closer to alignment.
@bigludo7 mentioned in his comment that using

{ "mc_claims": { "device_msisdn": "+33640049871" } }

in resource request is fine. Moreover, it is following the idea of the service, meaning that we check if device "holds" the same phone number, as network detects it.

Therefore, resource response should be similar and the one that comes inside MC standard should work perfectly:

{ "sub": "cd45a691-d311-4134-9a0c-2747e5110d22 " "device_msisdn_verified": true }

This response is common for "plain" or "hashed" versions.

As Number Verify requires Device Initiated request, (either OIDC or OAUTH2.0 "code flow"), the scope is a mandatory parameter and should reflect the actual API service. I recommend following MC Verified MSISDN specification and passing:

mc_vm_match
for "plain" version
mc_vm_match_hash
for hash

"share" is out of scope.

I see no issue with having multiple configurations, meaning utilizing generic or specific endpoints. This is a common approach and I think it can work in CAMARA as well.

paulocesardiaslima referenced this pull request in paulocesardiaslima/NumberVerification Dec 8, 2022
TIM Italy and TIM Brasil reviews points and contributions proposal:

Suggest to converge on a YAML from previous proposal A+B+C:
	A) Proposal by TEF (Telefonica Proposal #3)
	B) Proposal by DT (Initial content for Number Verify #2)
	C) Proposal initially by TIM Brasil (IP and Port header input parameters) 

Review points include in new YAML file version regarding path /authorize:
	IP and Port as optional input header parameters
	oAuth2 already mapped in DT (Initial content for Number Verify #2)
	Error code handling according to CAMARA others API (e.g., the Traffic Influence API)
paulocesardiaslima referenced this pull request in paulocesardiaslima/NumberVerification Dec 8, 2022
TIM Italy and TIM Brasil reviews points and contributions proposal:

Suggest to converge on a YAML from previous proposal A+B+C:
	A) Proposal by TEF (Telefonica Proposal #3)
	B) Proposal by DT (Initial content for Number Verify #2)
	C) Proposal initially by TIM Brasil (IP and Port header input parameters) 

Review points include in new YAML file version regarding path /authorize:
	IP and Port as optional input header parameters
	oAuth2 already mapped in DT (Initial content for Number Verify #2)
	Error code handling according to CAMARA others API (e.g., the Traffic Influence API)
@MarkusKuemmerle
Copy link
Contributor

@DT-DawidWroblewski : Please contribute API definitions in the code/API_definitions/ subdirectory and not in code/API_code/. Please change this pull request in this way.

@monamok
Copy link
Collaborator

monamok commented Dec 14, 2022

Hello @DT-DawidWroblewski,
I suggest to open an specific PR for the MoM. This way if people coment and it gets approved, it can be merged instead of getting mixed up with other files such as API documentation and defenition files.

@DT-DawidWroblewski
Copy link
Collaborator Author

DT-DawidWroblewski commented Dec 14, 2022 via email

@monamok
Copy link
Collaborator

monamok commented Dec 14, 2022

AFAIK for each branch you can create a new PR. If you create a branch from master and put the MoM file for example there, you can create a new PR for that branch. If it doesn't let you do that maybe there's a permission problem there and maybe @MarkusKuemmerle can help you with that.

@MarkusKuemmerle
Copy link
Contributor

MarkusKuemmerle commented Dec 14, 2022 via email

@DT-DawidWroblewski
Copy link
Collaborator Author

@bigludo7 I updated swagger based on MC Verified MSISDN specs - please review


-------

>following items were not presented during meeting, although sharing status and recommendations
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove the recomendation sections and the points that were not discussed during the meeting from the MoM.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We continued discussion about these APIs on the follow-up meeting that was on 15th Dec'22. I changed the naming of this item.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this part still might be misleading because you might have mentioned some of these points in the meeting but the rest of discussion and the answeres to your suggestion are missing here.
For example here it says that in the other API defenition "IP Address+Port passed in resource call" and it's not true. IP address is and was not included in any resource call. It uses 3-legged token and requires authentication via mobile network (excluding for example by SMS/OTP or user/password as an authentication method). It's NOT a query API either.
If someone reads this MoM might think that it was discussed and agreed in the meeting that the other proposal is a bad idea and it's not valid when it's not what happened. Could you please edit it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi @monamok !

I fixed this section - now it should be clear - please check.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please could you add some more information such as that it was decided to have two solutions, as you did for Sim Swap, one based on MC and another with specific endpoints aligned with other CAMARA APIs. Thanks.


1. GSMA to provide information about Openverse stakeholders (Done - Helen provided list to Vodafone and Hutchoison).
2. simSwap - work with 2 yaml files with API description
3. Number Verify - continue work based on MC Verified MSISDN specification, considering whole service flow.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This AP was not agreed on the meeting and might be confusing. Please remove it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

On a meeting we had on Thursday 15.12.2022 (week 50) we discussed following:

  1. deliver API description based on MC specs & with a dedicated endpoint
    MC APIs & Camara with dedicated API endpoint has a lot in common. MC APIs deliver enhanced service with additional info, meaning additional attributes in a single API call. Therefore, having 2 yaml files: for MC & Camara makes sense, as these services are complement to each other (not competing). This aggreement is for both Number Verify and SimSwap
  2. Provide additional description inside NumberVerify service description that explains how service flow looks like (with options like HE, redirects, additional headers etc.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, the result of the meeting on Dec 15th was the same as Sim Swap on Dec 13th --> work with 2 yaml files with API description. So maybe you could update the AP or give more info about it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated

documentation/API_documentation/API_definition.md Outdated Show resolved Hide resolved
code/API_definitions/CAMARA_numberVerify.yaml Outdated Show resolved Hide resolved
type: string
format: uri
example: http://localhost
responses:
Copy link
Collaborator

Choose a reason for hiding this comment

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

What happens in the case of success?

Copy link
Collaborator

Choose a reason for hiding this comment

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

same comment for me.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed...

@@ -0,0 +1,444 @@
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why adding the Postman collection ? is it something that should be provided for all Camara API as I'm not sure there is this for other API proposal.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

deleted...

Mobile Connect Verified MSISDN is defined as two service variants:

- **Verified MSISDN Match**: in which the Mobile Operator compares the MSISDN associated with the mobile device against that provided by the SP in the service request1. The MSISDN can be supplied in an E164 format [5] or in a hashed form. Verified MSISDN Match ensures no data is shared by the Mobile Operator.
- **Verified MSISDN Share**: in which the Mobile Operator provides the mobile device MSISDN to the SP who can then perform the check itself
Copy link
Collaborator

Choose a reason for hiding this comment

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

This share function is not provided in the swagger - It should be clearly indicated I guess or added in the swagger.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fine - I deleted this piece.

security:
- openId: [mc_vm_match,mc_vm_match_hash]
summary: Authorize request for MC Verified MSISDN service
parameters:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I do not understand why the scope attribute is missing ?
Is it because this is a dedicated API for number verified and we considered as set in stone for this API ?
This is not aligned with API documentation table 1

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I aligned with table - deleted "share".
these scopes are working only for MC Number Verify service - these are dedicated to API service flow.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Developer shall see the difference on /userInfo:

mc_vm_match

resource request requires plain msisdn passed inside the resource body

mc_vm_match_hash

resource request requires hashed msisdn passed inside the resource body

type: string
format: uri
example: http://localhost
responses:
Copy link
Collaborator

Choose a reason for hiding this comment

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

same comment for me.


## Verified MSISDN Service Specification

### OIDC Authorization Request Parameters - scope
Copy link
Collaborator

Choose a reason for hiding this comment

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

Scope missing in the swagger. See previous comment

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this was fixed with last commit

Co-authored-by: Mona Mokhber <mona.mokhber@telefonica.com>
Co-authored-by: Mona Mokhber <mona.mokhber@telefonica.com>
Copy link
Collaborator

@bigludo7 bigludo7 left a comment

Choose a reason for hiding this comment

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

Hi @DT-DawidWroblewski

  • Why no scope attribute ?
  • Got an error on swagger UI:
    `Errors
    Hide

Semantic error at paths./authorize.get.security.0
Security requirements must match a security definition
Jump to line 38`

@DT-DawidWroblewski
Copy link
Collaborator Author

@monamok @bigludo7 - I'd appreciate you sharing your review so that I can merge this PR.

Thx!

bigludo7
bigludo7 previously approved these changes Jan 5, 2023
Copy link
Collaborator

@bigludo7 bigludo7 left a comment

Choose a reason for hiding this comment

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

Look good for me.

Copy link
Collaborator

@monamok monamok left a comment

Choose a reason for hiding this comment

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

Some suggestions to make it more clear. Thanks.

documentation/MeetingMinutes/Week50_syncCalls.md Outdated Show resolved Hide resolved
documentation/MeetingMinutes/Week50_syncCalls.md Outdated Show resolved Hide resolved
Co-authored-by: Mona Mokhber <mona.mokhber@telefonica.com>
Co-authored-by: Mona Mokhber <mona.mokhber@telefonica.com>
Copy link
Collaborator

@monamok monamok left a comment

Choose a reason for hiding this comment

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

Thanks.

@DT-DawidWroblewski
Copy link
Collaborator Author

@bigludo7 can you please approve PR, as I placed some MoM updates from @monamok review. Thx!

Copy link
Collaborator

@bigludo7 bigludo7 left a comment

Choose a reason for hiding this comment

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

Look good for me.

@DT-DawidWroblewski DT-DawidWroblewski merged commit 60714ec into camaraproject:main Jan 5, 2023
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.

4 participants