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

Consolidate api information into yaml file #52

Conversation

fernandopradocabrillo
Copy link
Collaborator

@fernandopradocabrillo fernandopradocabrillo commented Sep 6, 2023

What type of PR is this?

Add one of the following kinds:

  • cleanup
  • documentation

What this PR does / why we need it:

  1. Include API documentation into yaml file and remove extra md files
  2. Rename api spec file to "sim_swap.yaml" since previous name "checkSimSwap.camara.swagger.yaml" no longer reflects the actual status of the API:
    • check is no longer the only operation available in the spec
    • there is no need to keep the distinction of camara since we no longer have MC version
    • since we are developing in openapi I think we don't need to include the swagger reference

Which issue(s) this PR fixes:

Fixes (#42)

Special notes for reviewers:

Issue #42 still has an open issue regarding the authorization flows to be implemented in the API, as we were waiting for a resolution in the discussions on Identity and Consent. This decission could lead to changes in the requests properties (Link to Mona's comment) so, if you are okey with it @bigludo7, @DT-DawidWroblewski, I suggest moving that topic to a new Issue and close #42 with the embeded documentation and the complete cleanup.

Changelog input

N/A

Additional documentation

N/A


[GSMA Mobile Connect Account Takeover Protection specification](https://www.gsma.com/identity/wp-content/uploads/2022/12/IDY.24-Mobile-Connect-Account-Takeover-Protection-Definition-and-Technical-Requirements-v2.0.pdf) was used as source of input for this API. For more about Mobile Connect, please see [Mobile Connect website](https://mobileconnect.io/).
Copy link
Collaborator

Choose a reason for hiding this comment

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

@fernandopradocabrillo I think we agreed to leave this reference in documentation... please change accordingly and do not delete this piece.

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 @DT-DawidWroblewski It is still there, I moved it to the section Further info and support.

@@ -35,13 +59,14 @@ paths:
/retrieve-date:
post:
security:
- oAuth2ClientCredentials: []
Copy link
Collaborator

Choose a reason for hiding this comment

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

what about countries where oauth2ClientCredentials is allowed? we should keep this.

Copy link
Collaborator Author

@fernandopradocabrillo fernandopradocabrillo Sep 6, 2023

Choose a reason for hiding this comment

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

The idea that is being discuss in the commonalities Issue (camaraproject/Commonalities#53) is to encapsulate the Authorization flows into the .well-known file of the OIDC protocol. This does not mean that we removed the client_credentials grant type, it is just not explicitly described inside the securitySchemes

Edited

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd keep it to avoid confusion while reading the yaml file

Copy link
Collaborator

Choose a reason for hiding this comment

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

@DT-DawidWroblewski may I suggest you also raise your concern in the camaraproject/Commonalities#53 ? i think it is important to provide your feedback at this level as it will impact all API.
@fernandopradocabrillo probably better to wait Commonalities outcome on this PR before to apply no ?

Copy link
Collaborator Author

@fernandopradocabrillo fernandopradocabrillo Sep 6, 2023

Choose a reason for hiding this comment

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

@bigludo7 yes I also think we can wait until the commonalities PR is merged. I was just advancing some work!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@DT-DawidWroblewski I retract this as I was not aware that it has been agreed in the discussions on Identity and Consent that resources that manage personal data cannot be accessed using client_credentials.

Source: https://github.com/camaraproject/IdentityAndConsentManagement/blob/main/documentation/CAMARA-API-access-and-user-consent.md

and:

camaraproject/IdentityAndConsentManagement#57

Any reference to other securitySchemes, such as client credentials, would generally be removed. Client credentials access (2-legged) must only be used "when user resource is NOT involved & the client belongs to confidential category".

@@ -76,13 +104,14 @@ paths:
/check:
post:
security:
- oAuth2ClientCredentials: []
Copy link
Collaborator

Choose a reason for hiding this comment

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

what about countries where oauth2ClientCredentials is allowed? we should keep this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Idem

Copy link
Collaborator

Choose a reason for hiding this comment

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

idem

@@ -116,21 +148,9 @@ paths:
$ref: "#/components/responses/Generic504"
components:
securitySchemes:
oAuth2ClientCredentials:
Copy link
Collaborator

Choose a reason for hiding this comment

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

we cannot delete this option as oauth2clientCredentials is an option that is available on many markets

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Idem

Copy link
Collaborator

Choose a reason for hiding this comment

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

idem

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.

Thanks for that.
One question about the version - as you change code for scope & header should be still v4.1 or v4.2 or v5.0-wip?

@hdamker
Copy link
Collaborator

hdamker commented Sep 7, 2023

@fernandopradocabrillo

I strongly propose to have two different PRs:

  • One to consolidate the documentation into the YAML file. It's documentation only and will not impact the upcoming version number of a release (or just the patch number)
  • Changes of the YAML file, e.g. discussion about security schema

Doing it in one PR will make it complex and will block the independent changes.

@fernandopradocabrillo fernandopradocabrillo force-pushed the include-api-documentation-into-yaml-file branch from c9fed64 to a9fe4c2 Compare September 7, 2023 11:47
@fernandopradocabrillo
Copy link
Collaborator Author

@fernandopradocabrillo

I strongly propose to have two different PRs:

  • One to consolidate the documentation into the YAML file. It's documentation only and will not impact the upcoming version number of a release (or just the patch number)
  • Changes of the YAML file, e.g. discussion about security schema

Doing it in one PR will make it complex and will block the independent changes.

@hdamker @bigludo7 @DT-DawidWroblewski Updated this PR to only include the documentation in the yaml. I'll create a new one with then changes in the scopes, headers, etc

@fernandopradocabrillo fernandopradocabrillo changed the title Consolidate api information into yaml file and sync with commonalities Consolidate api information into yaml file Sep 7, 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.
I'm fine for this one to keep same version as we just added documentation but probably in future update we should change the last didit version no?

@DT-DawidWroblewski
Copy link
Collaborator

it is ok. thanks @fernandopradocabrillo

@DT-DawidWroblewski DT-DawidWroblewski merged commit 1f65514 into camaraproject:main Sep 7, 2023
@fernandopradocabrillo
Copy link
Collaborator Author

Look good for me. I'm fine for this one to keep same version as we just added documentation but probably in future update we should change the last didit version no?

Yes, I have updated to v0.5.0-wip in the PR with the yaml changes. I don't know exactly what convention are we following in the versioning so I'm open to suggestions!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Embedding documentation in CAMARA API spec and decision about documenting authorization methods
4 participants