Sim Swap Notification subscription yaml #60

bigludo7 · 2023-10-02T15:52:17Z

What type of PR is this?

enhancement/feature

What this PR does / why we need it:

Add a new API to manage sim swap notification subscription & notification.
yaml aligned with CAMARA commonalities design guidelines.

Which issue(s) this PR fixes:

Fixes #47

Special notes for reviewers:

Changelog input

 release-note

Additional documentation

This section can be blank.

docs

Sim Swap Notification subscription yaml aligned with CAMARA commonalities design guidelines.

fernandopradocabrillo

@bigludo7
I leave a few initial comments to discuss.

Thanks for the PR!

fernandopradocabrillo · 2023-10-03T11:41:01Z

code/API_definitions/sim-swap-notification-subscription.yaml

+security:
+  #  - {}
+  - oAuth2ClientCredentials: []
+  #  - BasicAuth: []
+  #  - apiKey: []
+  - threeLegged:
+      - sim-swap:subscriptions:create


A couple comments here:

The use of OpenId securityScheme (the never-ending topic haha)

I think we should have different scopes for each operation:

sim-swap:subscriptions:create for the Create operation

sim-swap:subscriptions:read for the List and Detail operations

sim-swap:subscriptions:delete for the Delete operation

Agree - fixed.

@bigludo7
if we already have the security property in every operation, the global security is redundant right? I think we can remove it

@fernandopradocabrillo Which one do you think we can remove ? I followed https://swagger.io/docs/specification/authentication/openid-connect-discovery/ pattern.

@bigludo7
We can have the security property in every operation or have it global. I think it is better to have it in every endpoint so we can specify the scope that applies to each operation properly

fernandopradocabrillo · 2023-10-03T11:43:09Z

code/API_definitions/sim-swap-notification-subscription.yaml

+      required:
+        - webhook
+      properties:
+        subscriptionDetail:


I know this is the same model we are using in Device Status but, and I would like your opinion here too, for me it does not make sense that the suscriptionDetail is optional. I should not be able to make the request without sending the details, what would be the response?

Yes got your point. As the subscription/notification is still 'fresh' I guess we're still testing it. I changed the cardinality to mandatory.

code/API_definitions/sim-swap-notification-subscription.yaml

gregory1g · 2023-10-05T07:23:37Z

code/API_definitions/sim-swap-notification-subscription.yaml

+        phoneNumber:
+          $ref: '#/components/schemas/PhoneNumber'
+        type:
+          $ref: '#/components/schemas/SimSwapEventType'


should we refence RoamingNotificationEventType here as well?

I will prefer not. If we list all event type in all API that will be complicated to maintain.
Perhaps we should provide 'somewhere' a listing of all event type managed within our CAMARA APIs?

But currently we list only exact one of them. API client must know what events it can receive and implement corresponding logic. One can keep the list open, but at least base type of events must be described. Events like "even notification subscription ends" and "MSISDN subscription ends" are critically important and must be included in the "base" list of event types.
All above if IMO :)

@gregory1g Thanks for the feedback
We have only one type for subscribtion but 2 type are managed by notification. Indeed org.camaraproject.sim-swap.v0.subscription-ends is also described in this yaml. We do not plan to provide subscription for this one and send them automatically once the subscription terminates (for any reason). This is described in the doc part.

We should add in this documentation than the same resource for subscription as notification are managed for all other CAMARA APIs.

@fernandopradocabrillo

Following @fernandopradocabrillo review

code/API_definitions/sim-swap-notification-subscription.yaml

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

gregory1g · 2023-10-17T05:22:35Z

code/API_definitions/sim-swap-notification-subscription.yaml

+
+    Note: Additionnally to these list, ``org.camaraproject.sim-swa.v0.subscription-ends`` notification `type` is sent when the subscription ends. 
+    This notification did not requires dedicated subscription. 
+    It is used when the subscription expire time (required by the requester) has been reached or if the API server has to stop sending notification prematurely. 


"if the API server has to stop sending notification prematurely." sounds a bit unclear, what about using more simple wording like "if the API server has to terminate subscription (e.g. when the corresponding phone number is not served by the operator anymore)"?

gregory1g · 2023-10-17T05:35:03Z

code/API_definitions/sim-swap-notification-subscription.yaml

+  #  - BasicAuth: []
+  #  - apiKey: []
+  - openId:
+      - sim-swap:subscriptions:create


Do we need independent scopes here?
From the end user privacy point of view, allow someone to subscribe to simswap is equal to allow someone to read simswap timestamp. Or, at least a permission to "subscribe" includes permission to read. Like end user revokes permission to read their SimSwap information, this logically also revokes permission to be notified about such events. This is an API specific logic, and it is too risky to completely delegate it to Auth/Consent configuration.

@gregory1g This is a fair comment. Could we consider from a privacy perspective that checking now is same than checking for a coming period of x days?
I tend to agree with you.
@fernandopradocabrillo @DT-DawidWroblewski : could you please provide your view here.

and isn't it possible to have permission to read but not to subscribe to the notification?

permission to read, but not to subscribe is rather questionable, but still could be needed.
however, permission to subscribe without permission to read sounds like an dangerous security inconsistency

gregory1g · 2023-10-17T10:30:12Z

code/API_definitions/sim-swap-notification-subscription.yaml

+          $ref: '#/components/schemas/PhoneNumber'
+        terminationReason:
+          type: string
+          enum: ["SUBSCRIPTION_EXPIRED", "NETWORK_TERMINATED"]


as mentioned above "NETWORK_TERMINTAED" covers a server side termination for whatever reason. Two questions here:

Is "NETWORK_TERMINTAED" a self-explaining term for a server initiated termination? One can expect that user terminated is different from network terminated.

shouldn't we add a possibility for an MNO to provide an optional a human readable text description explaining the reason behind server side termination?

Not sure to get it. You want me to add USER_TERMINATED ?

Yes make sense - I've add a terminationDescription attribute.

I dot think that we need to be introduce "user terminated" here. But NETWORK_TERMINTAED sounds too narrow. I suggest to consider a more generic term which covers everything which is initiated by the server side, either it is network as such, or user his/her self, or contract between application and MNO or even security conserns.

gregory1g · 2023-10-17T10:31:10Z

code/API_definitions/sim-swap-notification-subscription.yaml

+        - specversion
+        - type
+      properties:
+        id:


What about subscription ID, shouldn't it be included in the event as well?

Yes - It should be part of the Data structure - I add it.

Integrated 2 comments.

bigludo7

Answered + updated following @gregory1g review.

bigludo7 · 2023-10-23T11:46:48Z

code/API_definitions/sim-swap-notification-subscription.yaml

+        - specversion
+        - type
+      properties:
+        id:


Yes - It should be part of the Data structure - I add it.

bigludo7 · 2023-10-23T11:51:11Z

code/API_definitions/sim-swap-notification-subscription.yaml

+          $ref: '#/components/schemas/PhoneNumber'
+        terminationReason:
+          type: string
+          enum: ["SUBSCRIPTION_EXPIRED", "NETWORK_TERMINATED"]


Not sure to get it. You want me to add USER_TERMINATED ?

Yes make sense - I've add a terminationDescription attribute.

bigludo7 · 2023-11-02T17:14:58Z

Hello @gregory1g
May I ask you to check the answer provided to your comments and let me know if some of them need additional attention. Thanks for your help.

fernandopradocabrillo

@bigludo7
I leave you a few comments I've found in a more detailed review. Some are suggestions and some are small fixes.

code/API_definitions/sim-swap-notification-subscription.yaml

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

@fernandopradocabrillo

Updated following @fernandopradocabrillo review

bigludo7

Thanks @fernandopradocabrillo for your review. I have updated the yaml accordingly (except the error message that I kept).

bigludo7 · 2023-11-06T08:22:54Z

code/API_definitions/sim-swap-notification-subscription.yaml

      - sim-swap:subscriptions:create
+      - sim-swap:subscriptions:read


Hello @gregory1g
For me the simswap and the subscription are 2 different entities. As subscription is a distinct entity it makes sense to have distinct scope to manage it but worth to be discussed.
As we should have same discussion in subscription for device location and device status probably better to bring this discussion to commonalities.

If a user revokes their "read my SimSwap info" for app XYZ, it is Telco commitment to terminate app XYZ subscription (if any) and prevent any sim swap data sharing with XYZ.

fernandopradocabrillo

@bigludo7
A few format-related additional comments.

code/API_definitions/sim-swap-notification-subscription.yaml

fernandopradocabrillo · 2023-11-06T12:44:55Z

code/API_definitions/sim-swap-notification-subscription.yaml

+          content:
+            application/json:
+              schema:
+                $ref: '#/components/schemas/SubscriptionInfo'


@bigludo7
One doubt here, in case I've missed something, why do we have the phoneNumber as optional in the request body?

And, even if we have it optional in the request, shouldn't it be required in the response? I think the response must give the complete information of the subscription. Applies the same for the GET endpoints

Yes make sense for POST & GET answers --> I've updated.

For POST request, the phoneNumber could be optional as we can have it by the network thru Authorization code flow or CIBA flow.

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

Changed cardinality for phoneNumber

bigludo7

Changed cardinality for phoneNumber as suggested by @fernandopradocabrillo

bigludo7 · 2023-11-13T13:55:26Z

Hello @fernandopradocabrillo @gregory1g @DT-DawidWroblewski Any pending issue preventing to get your approval?
We should probably moving forward with this PR and then use GitHub issue for improving the proposal. Thanks

gregory1g · 2023-11-13T13:57:58Z

Any pending issue preventing to get your approval?

nothing from my side.

fernandopradocabrillo · 2023-11-13T15:25:33Z

Any pending issue preventing to get your approval?

LGTM from my side too. If we detect something in the future we can always fix it

fernandopradocabrillo

LGTM

DT-DawidWroblewski · 2023-11-17T10:44:32Z

code/API_definitions/sim-swap-notification-subscription.yaml

+      - ``org.camaraproject.sim-swap.v0.swapped`` - Event triggered when a sim swap occurs on the associated phoneNumber
+
+
+    Note: Additionnally to these list, ``org.camaraproject.sim-swa.v0.subscription-ends`` notification `type` is sent when the subscription ends. 


Typo:

is:
org.camaraproject.sim-swa.v0.subscription-ends

should be:
org.camaraproject.sim-swap.v0.subscription-ends

DT-DawidWroblewski · 2023-11-17T10:49:03Z

code/API_definitions/sim-swap-notification-subscription.yaml

+
+
+    Note: Additionnally to these list, ``org.camaraproject.sim-swa.v0.subscription-ends`` notification `type` is sent when the subscription ends. 
+    This notification did not requires dedicated subscription. 


Grammar issue:

is:

This notification did not requires dedicated subscription.

should be:
This notification does not require a dedicated subscription.

DT-DawidWroblewski · 2023-11-17T10:50:32Z

code/API_definitions/sim-swap-notification-subscription.yaml

+
+  termsOfService: http://swagger.io/terms/
+  contact:
+    email: project-email@sample.com


should we have an explicit email to sub-project owner here?

Not sure - I followed here what we have in other API but this is a fair question.
My suggestion is to have an issue in Commonalities to have a common rule.

DT-DawidWroblewski · 2023-11-17T10:51:39Z

code/API_definitions/sim-swap-notification-subscription.yaml

+  version: 0.1.0-wip
+externalDocs:
+  description: Product documentation at CAMARA
+  url: https://github.com/camaraproject/


I think here we should use the following URL:
https://github.com/camaraproject/SimSwap

DT-DawidWroblewski · 2023-11-17T10:56:24Z

code/API_definitions/sim-swap-notification-subscription.yaml

+                  $ref: "#/components/responses/Generic503"
+              security:
+                - { }
+                - notificationsBearerAuth: [ ]


is there a dedicated security scheme for notifications? or are we following CIBA and use push mode see CIBA spec for reference?

Not sure - I've replicated same thing than in other Subscription API. If we want to provide more we should probably propose someting here: https://github.com/camaraproject/Commonalities/blob/main/documentation/API-design-guidelines.md#security-considerations

DT-DawidWroblewski · 2023-11-17T11:00:31Z

code/API_definitions/sim-swap-notification-subscription.yaml

+      enum:
+        - org.camaraproject.sim-swap.v0.swapped
+
+    RoamingNotificationEventType:


Roaming notification in sim swap API? is it a new enhancement coming with sim swap notification?

ah ah ah ... no :) good catch Dawid. I've wrongly name this class and forget to use it (2 errors on a same line). I fix it.

@DT-DawidWroblewski

Updated following @DT-DawidWroblewski review.

bigludo7

Updated following @DT-DawidWroblewski review.

DT-DawidWroblewski

LGTM

Sim Swap Notification subscription yaml

02c66bf

Sim Swap Notification subscription yaml aligned with CAMARA commonalities design guidelines.

bigludo7 requested a review from jgarciahospital October 2, 2023 15:52

bigludo7 requested review from DT-DawidWroblewski and fernandopradocabrillo as code owners October 2, 2023 15:52

fernandopradocabrillo reviewed Oct 3, 2023

View reviewed changes

gregory1g reviewed Oct 5, 2023

View reviewed changes

Update sim-swap-notification-subscription.yaml

30eacea

Following @fernandopradocabrillo review

fernandopradocabrillo reviewed Oct 9, 2023

View reviewed changes

bigludo7 and others added 4 commits October 9, 2023 21:01

Update code/API_definitions/sim-swap-notification-subscription.yaml

5f6e49a

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

Update code/API_definitions/sim-swap-notification-subscription.yaml

d71baaa

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

Update code/API_definitions/sim-swap-notification-subscription.yaml

6b8625e

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

Update code/API_definitions/sim-swap-notification-subscription.yaml

de9eb14

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

gregory1g reviewed Oct 17, 2023

View reviewed changes

Update sim-swap-notification-subscription.yaml

a7c6d6b

Integrated 2 comments.

bigludo7 commented Oct 23, 2023

View reviewed changes

fernandopradocabrillo reviewed Nov 2, 2023

View reviewed changes

code/API_definitions/sim-swap-notification-subscription.yaml Show resolved Hide resolved

code/API_definitions/sim-swap-notification-subscription.yaml Outdated Show resolved Hide resolved

bigludo7 and others added 9 commits November 5, 2023 15:52

Update code/API_definitions/sim-swap-notification-subscription.yaml

6686333

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

Update code/API_definitions/sim-swap-notification-subscription.yaml

1ab5966

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

Update code/API_definitions/sim-swap-notification-subscription.yaml

8fca45c

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

Update code/API_definitions/sim-swap-notification-subscription.yaml

3c35417

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

Update code/API_definitions/sim-swap-notification-subscription.yaml

065bbcd

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

Update code/API_definitions/sim-swap-notification-subscription.yaml

29f96a0

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

Update code/API_definitions/sim-swap-notification-subscription.yaml

69a0efb

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

Update code/API_definitions/sim-swap-notification-subscription.yaml

445aafc

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

Update code/API_definitions/sim-swap-notification-subscription.yaml

7d73a77

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

Update sim-swap-notification-subscription.yaml

272a6f1

Updated following @fernandopradocabrillo review

bigludo7 commented Nov 5, 2023

View reviewed changes

bigludo7 commented Nov 6, 2023

View reviewed changes

fernandopradocabrillo reviewed Nov 6, 2023

View reviewed changes

bigludo7 and others added 9 commits November 6, 2023 16:15

Update code/API_definitions/sim-swap-notification-subscription.yaml

8edeb06

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

Update code/API_definitions/sim-swap-notification-subscription.yaml

320f0db

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

Update code/API_definitions/sim-swap-notification-subscription.yaml

a802dac

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

Update code/API_definitions/sim-swap-notification-subscription.yaml

1a7693c

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

Update code/API_definitions/sim-swap-notification-subscription.yaml

e2087e6

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

Update code/API_definitions/sim-swap-notification-subscription.yaml

cd925b5

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

Update code/API_definitions/sim-swap-notification-subscription.yaml

d81a78c

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

Update code/API_definitions/sim-swap-notification-subscription.yaml

92fb132

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

Update sim-swap-notification-subscription.yaml

3534158

Changed cardinality for phoneNumber

bigludo7 commented Nov 6, 2023

View reviewed changes

fernandopradocabrillo previously approved these changes Nov 13, 2023

View reviewed changes

DT-DawidWroblewski suggested changes Nov 17, 2023

View reviewed changes

bigludo7 dismissed fernandopradocabrillo’s stale review via 5ed9b76 November 17, 2023 12:44

Update sim-swap-notification-subscription.yaml

5ed9b76

Updated following @DT-DawidWroblewski review.

bigludo7 commented Nov 17, 2023

View reviewed changes

DT-DawidWroblewski approved these changes Nov 20, 2023

View reviewed changes

DT-DawidWroblewski merged commit c12ac23 into main Nov 20, 2023

bigludo7 deleted the bigludo7-patch-2 branch April 4, 2024 08:39

		- sim-swap:subscriptions:create
		- sim-swap:subscriptions:read

		- ``org.camaraproject.sim-swap.v0.swapped`` - Event triggered when a sim swap occurs on the associated phoneNumber


		Note: Additionnally to these list, ``org.camaraproject.sim-swa.v0.subscription-ends`` notification `type` is sent when the subscription ends.



		Note: Additionnally to these list, ``org.camaraproject.sim-swa.v0.subscription-ends`` notification `type` is sent when the subscription ends.
		This notification did not requires dedicated subscription.

Sim Swap Notification subscription yaml #60

Sim Swap Notification subscription yaml #60

Conversation

bigludo7 commented Oct 2, 2023

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

fernandopradocabrillo left a comment

Choose a reason for hiding this comment

fernandopradocabrillo Oct 3, 2023 • 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

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 Oct 17, 2023 • 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

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

bigludo7 commented Nov 2, 2023

fernandopradocabrillo 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

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

bigludo7 commented Nov 13, 2023

gregory1g commented Nov 13, 2023

fernandopradocabrillo commented Nov 13, 2023

fernandopradocabrillo 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

bigludo7 left a comment

Choose a reason for hiding this comment

DT-DawidWroblewski left a comment

Choose a reason for hiding this comment

fernandopradocabrillo Oct 3, 2023 •

edited

Loading

gregory1g Oct 17, 2023 •

edited

Loading