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

Sim swap subscriptions alignement with commonalities 0.5 #179

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

bigludo7
Copy link
Collaborator

What type of PR is this?

Add one of the following kinds:

  • enhancement/feature

What this PR does / why we need it:

Alignement of sim_swap-subscriptions.yaml & sim_swap-subscriptions.feature with commonalities 0.5 rules.
See detail in release note.

Which issue(s) this PR fixes:

Fixes #161

Special notes for reviewers:

This is breaking change.

Changelog input

 release-note
sim_swap-subscriptions.yaml:
- Change error structure definition to normalize error & status
- Update error code to introduce 429 & 422 - removed 5xx, 410 and 415 error
- Follow principle of data minimization by removing sinkCredential in POST & GET response
- Adapt request for 3-legs usage featuring identification of the phoneNumber
- Improve documentation part
- Add 3-legs examples

sim_swap-subscriptions.feature:
- Align test scenario with commonalities 0.5 improvements

Additional documentation

This section can be blank.

docs

Align sim-swap-subscriptions.yaml with comm 0.5
Align sim-swap-subscriptions.feature with comm 0.5
Copy link

github-actions bot commented Dec 23, 2024

🦙 MegaLinter status: ✅ SUCCESS

Descriptor Linter Files Fixed Errors Elapsed time
✅ ACTION actionlint 2 0 0.02s
✅ OPENAPI spectral 2 0 3.18s
✅ REPOSITORY git_diff yes no 0.01s
✅ REPOSITORY secretlint yes no 0.83s
✅ YAML yamllint 2 0 0.66s

See detailed report in MegaLinter reports

MegaLinter is graciously provided by OX Security

Mega-Linter - remove trailing spaces
Try to fix examples for subscriptionDetail
@bigludo7
Copy link
Collaborator Author

Hello @rartych
I will need your help for the megalinter issue.

Megalinter is unhappy because in the 3LEGS examples the subscriptionDetail is null.
This attribute is mandatory but in these examples we will have an empty object.

I tried
subscriptionDetail: null
or
subscriptionDetail:

but neither work. Any idea?

Thanks for you help.

@@ -1194,3 +1351,17 @@ components:
startsAt: '2024-06-07T16:10:15.302Z'
expiresAt: '2024-06-07T16:10:15.302Z'
status: ACTIVATION_REQUESTED
SUBSCRIPTIONS_3LEGS:
Copy link
Collaborator

Choose a reason for hiding this comment

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

How one is expected to use a 3-legged token to read all subscriptions? A 3-legged token is valid for a single phon number only.

Therefore,

  • either the response must include only subscription for this exactly phone number, what creates an implicit filtering functionality AND reading subscription without specifying an ID must also support 2-legged token to really read all subscriptions
  • or reading subscription without specifying an ID must support 2-legged auth only .

Copy link
Collaborator Author

@bigludo7 bigludo7 Jan 6, 2025

Choose a reason for hiding this comment

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

This is a good comment. I propose to update the documentation part:


These rules apply for subscription:

If 3-legged access token is used:

  • For POST and GET{id} request & responses must not provide any device identifier phoneNumber.
  • For GET (list) only subscription for this exactly phone number is retrieved (implicit filtering)

If 2-legged access token is used:

  • For POST and GET{id} the presence of a device identifier phoneNumber in the request t& response is mandatory.
  • For GET (list) the phoneNumber could be used as filtering criteria.

WDYT?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Makes sense for me.
But

  • for usual operation it is up to the MNO to decide if it wants/can support 3-legged or 2-legged option.
  • meanwhile for GET (list) MNO must always support client credentials (with or without 3-legged option in addition), because otherwise this API is hardly usable - one cannot get a complete list of subscription it has created.

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 got your point for the GET (list) but I'm wondering if we have to strictly enforce that MNO must support client credential. I will prefer to keep it as a recommandation and probably waiting for live implementation to get feedbacks. If we enforce it now it will be tough from a certification perspective.

@@ -63,7 +63,7 @@ Feature: CAMARA sim swap subscriptions API, v0.1.1
Scenario: Check existing subscription is retreived in list
Given valid subscriptions are existing
And use BaseURL
When get sim swap subscription
When the HTTP "GET" request is sent
Copy link
Collaborator

Choose a reason for hiding this comment

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

This PQ makes response depending on the token type. As a result statement "And the response body complies with an array of OAS schema defined at "#/components/schemas/Subscription"" does not describe the expected result clearly enough anymore. Separate test cases are needed for 2 and 3 legged requests.

And for 3-legged case one need to specify which exactly subscriptions must be returned.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes make sense
I've reworked the proposal with @sim_swap_subscription_retrieve_04_retrieve_list_2legs, @sim_swap_subscription_retrieve_07_retrieve_list_3legs and @sim_swap_subscription_retrieve_08_retrieve_empty_list_3legs

@bigludo7
Copy link
Collaborator Author

bigludo7 commented Jan 9, 2025

@fernandopradocabrillo @gregory1g @rartych Following camaraproject/Commonalities#369 discussion I think it's fair to remove 403 INVALID_TOKEN_CONTEXT example in the yaml.

May I ask you to review this new proposal soon as we have to prepare the RC candidate before Jan17th. We can still modify after but we have to achieve the milestone.

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.

Scope for Spring25 release (in preparation)
3 participants