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

Adapt to ICM Security and Interoperability Profile #208

Merged
merged 17 commits into from
Jun 11, 2024

Conversation

AxelNennker
Copy link
Contributor

@AxelNennker AxelNennker commented May 17, 2024

What type of PR is this?

Add one of the following kinds:

  • documentation

What this PR does / why we need it:

ICM created the Camara Security and Interoperablity Profile which is not reflected in Camara API Design Guidelines.

  • adapt glossary, new OIDC and CIBA, correct TLS
  • refer to Camara Security and Interoperablity Profile
  • simplify JSON definition
  • add links to standards
  • consistent naming of OAuth
  • Add the OpenAPI guideline from ICM to Commonalities' OpenAPI guidelines, so all OpenAPI guidelines are in one document. If approved, remove the OpenAPI text from ICM's document

Which issue(s) this PR fixes:

Fixes #207

@jpengar
Copy link
Collaborator

jpengar commented May 20, 2024

@AxelNennker @rartych This PR should be set as DRAFT until a decision is made in the ICM WG (or at least keep the 11.6 Security Definition section out of the scope of the PR as it is not related to the content of the new ICM profile document). ICM CAMARA-API-access-and-user-consent.md already documents "CAMARA API Specification - Authorization and authentication common guidelines" and this document is also being adapted to ICM Security and Interoperability Profile (camaraproject/IdentityAndConsentManagement#155). ICM, as Commonalities WG, also provides guidance to API subprojects and this should be well-known. This particular guideline comes from ICM and it is no related with the profile and I personally would keep this information within ICM. But in any case, whatever it's ultimately done, it should be an ICM WG decision.

@AxelNennker
Copy link
Contributor Author

I think it is a Commonalities decision what to have in API design guidelines, and because the OpenAPI guidelines are already in there in section 11 I suggest to have the OpenAPI security-scheme guidelines there as well.

@jpengar
Copy link
Collaborator

jpengar commented May 20, 2024

I think it is a Commonalities decision what to have in API design guidelines, and because the OpenAPI guidelines are already in there in section 11 I suggest to have the OpenAPI security-scheme guidelines there as well.

Agree, but what you are doing here is unilaterally moving information from the ICM to Commonalities deciding on behalf of the ICM WG. I agree that Commonalities should decide what is documented in the API Design Guidelines (Commonalities WG is the owner), but first the ICM WG should decide whether this information should be removed from the ICM or not. Right?
And also this PR description does not explain that the content of ICM "CAMARA-API-access-and-user-consent.md" will be moved to section 11.6 and only mentions adaptations according to the ICM profile which is misleading. Because the text content of 11.6 section is not related to any content of the ICM profile.

@AxelNennker
Copy link
Contributor Author

I think a Pull Request is never unilateral because it is a "request".
If Commonalities approves the request to have the OpenAPI guideline in API Design Guidelines, then ICM should probably agree to remove duplicate guidelines from its documents. In ICM we still need to adapt to the new Security and Interoperability anyway.

@AxelNennker
Copy link
Contributor Author

Added text to the description of the Pull Request

Add the OpenAPI guideline from ICM to Commonalities' OpenAPI guidelines, so all OpenAPI guidelines are in one document. If approved, remove the OpenAPI text from ICM's document

@jpengar
Copy link
Collaborator

jpengar commented May 20, 2024

I think a Pull Request is never unilateral because it is a "request". If Commonalities approves the request to have the OpenAPI guideline in API Design Guidelines, then ICM should probably agree to remove duplicate guidelines from its documents. In ICM we still need to adapt to the new Security and Interoperability anyway.

I disagree, I think you are skipping the decision of the ICM WG.

@AxelNennker
Copy link
Contributor Author

I guess that the root problem is that the OpenAPI security schemes are in the intersection of Commonalities and ICM.
I assumed that all OpenAPI guidelines should be in one place and that that place is the Commonalities API Design Guidelines.
An API Designer should not be forced to read more than one Camara document to learn about OpenAPI guidelines, I think.

I am creating issues and PRs as "me" not as an ICM representative.

@jpengar
Copy link
Collaborator

jpengar commented May 21, 2024

I guess that the root problem is that the OpenAPI security schemes are in the intersection of Commonalities and ICM. I assumed that all OpenAPI guidelines should be in one place and that that place is the Commonalities API Design Guidelines. An API Designer should not be forced to read more than one Camara document to learn about OpenAPI guidelines, I think.

I am creating issues and PRs as "me" not as an ICM representative.

Just as the ICM Security and Interoperability Profile contains many references to standards and other documents, I don't see what the problem is in including in the API design guidelines a reference to the corresponding official CAMARA ICM document where these guidelines are specified. These guidelines are the result of work done in the ICM and are documented in a document under the ownership of the ICM, which I think is correct. So, IMHO, it is only by decision of the ICM that this information could be moved.

And not only that, the information contained in section 11.6 is NOT even the same information documented in the ICM doc. This PR changes the content, even the content of such a sensitive part as the info.description template, which needed a long review and approval of the ICM working group participants. Changing that information should be reviewed and approved by ICM.

@AxelNennker
Copy link
Contributor Author

Just as the ICM Security and Interoperability Profile contains many references to standards and other documents, I don't see what the problem is in including in the API design guidelines a reference to the corresponding official CAMARA ICM document where these guidelines are specified.

As I said, I think that guidelines on OpenAPI should all be in one place and because the majority of the OpenAPI guidelines are in API design guidelines section 11 the guideline regarding security schemes should be there as well.

These guidelines are the result of work done in the ICM and are documented in a document under the ownership of the ICM, which I think is correct. So, IMHO, it is only by decision of the ICM that this information could be moved.

I think there is no such thing as "ownership" in Camara. We all work together to achieve secure and interoperable API definitions and guidelines. "IdentityAndConsent" is such a big chunk of work that it was decided to create the IdentityAndConsent WG and, of course, the results of our ICM work flow to were they help implementers, API designers, API consumers, ... the most.
In this case, I think, that is the Commonalities' document.

And not only that, the information contained in section 11.6 is NOT even the same information documented in the ICM doc. This PR changes the content, even the content of such a sensitive part as the info.description template, which needed a long review and approval of the ICM working group participants. Changing that information should be reviewed and approved by ICM.

As "CAMARA guidelines defines a set of authorization flows which can grant API clients access to the API functionality" is now done in the Security and Interoperability Profile the text had to be changed to link to the Profile.
Please suggest text in your review.

@AxelNennker
Copy link
Contributor Author

Regarding

This PR changes the content, even the content of such a sensitive part as the info.description template, which needed a long review and approval of the ICM working group participants. Changing that information should be reviewed and approved by ICM.

I amended info.description in this Pull Request. That was mostly a copy-past error as I forgot to copy the last section.
I hope, that fixes my mistake.

@diegogonmar
Copy link

Regarding

This PR changes the content, even the content of such a sensitive part as the info.description template, which needed a long review and approval of the ICM working group participants. Changing that information should be reviewed and approved by ICM.

I amended info.description in this Pull Request. That was mostly a copy-past error as I forgot to copy the last section. I hope, that fixes my mistake.

That is the issue @AxelNennker. No one should be reviewing this to ensure yo did not make any "mistake" acting on your behalf proposing to move agreed text from a group to another.
Because the text was agreed in ICM (in TSC indeed, as it was escalated there!) so decision to propose moving the text elsewhere cannot be taken by any individual without former agreement in ICM. Otherwise a "mistake" can lead into an inconsistency or duplicity of information. This is quite risky, so please close PR and move issue to ICM to discuss there.

@AxelNennker
Copy link
Contributor Author

That is the issue @AxelNennker. No one should be reviewing this to ensure yo did not make any "mistake" acting on your behalf proposing to move agreed text from a group to another. Because the text was agreed in ICM (in TSC indeed, as it was escalated there!) so decision to propose moving the text elsewhere cannot be taken by any individual without former agreement in ICM. Otherwise a "mistake" can lead into an inconsistency or duplicity of information. This is quite risky, so please close PR and move issue to ICM to discuss there.

@diegogonmar I believe that mistakes can always happen. E.g. when a API subproject copies the info.description from OpenAPI guidelines to their own yaml file that last paragraph might be temporarily be lost again until a reviewer catches that. That's why we have reviews.

Co-authored-by: Jesús Peña García-Oliva <jesus.penagarcia-oliva@telefonica.com>
@AxelNennker
Copy link
Contributor Author

My paraphrasing of the arguments presented - with edits by @jpengar in the cons section

Main question is whether or not moving OpenAPI security schemes and info.description to OpenAPI definitions

Pro :

  • API Design guidelines should be in one place and not be scattered into several.
  • OpenAPI Definitions should be in one place which is currently the API Design Guidelines document
  • There is precedence that release management created guidelines that are now part of Commonalities documents.
  • Camara_common.yaml is also a Commonalities document

Contra moving arguments:

  • Specifically for this securitytSchemes issue, it is a very polarising issue that took weeks to agree and required a TSC escalation and decision, and it is already happening (with all the overhead and confusion that entails) that there is more than one open issue in Commonalities, API subprojects, slack messages and even multiple issues in ICM just related to security schemes. Having this information in the ICM would allow us to have better control over it, and also guide CAMARA participants to the decisions made by the ICM and TSC, avoiding overhead and confusion.
  • The security scheme expertise is in ICM and future changes are easier to do if security schemes and info.description are in an ICM document.
  • ICM, as Commonalities WG, also provides guidance to API subprojects and this is well-known.
  • These guidelines are the result of work done in the ICM and are documented in a document under the ownership of the ICM
  • For a document reader perspective, it is just one click browsing github documentation, which means nothing. There are many other references like that in the API design guidelines or the ICM profile itself and it is not a problem. It's like browsing through Confluence pages...

Options:

  • move openapi definitions to API Design guidelines
  • keep security scheme and info.description in access and user consent

@jpengar
Copy link
Collaborator

jpengar commented Jun 5, 2024

My paraphrasing of the arguments presented - with edits by @jpengar in the cons section

Main question is whether or not moving OpenAPI security schemes and info.description to OpenAPI definitions

Pro :

  • API Design guidelines should be in one place and not be scattered into several.
  • OpenAPI Definitions should be in one place which is currently the API Design Guidelines document
  • There is precedence that release management created guidelines that are now part of Commonalities documents.
  • Camara_common.yaml is also a Commonalities document

Contra moving arguments:

  • Specifically for this securitytSchemes issue, it is a very polarising issue that took weeks to agree and required a TSC escalation and decision, and it is already happening (with all the overhead and confusion that entails) that there is more than one open issue in Commonalities, API subprojects, slack messages and even multiple issues in ICM just related to security schemes. Having this information in the ICM would allow us to have better control over it, and also guide CAMARA participants to the decisions made by the ICM and TSC, avoiding overhead and confusion.
  • The security scheme expertise is in ICM and future changes are easier to do if security schemes and info.description are in an ICM document.
  • ICM, as Commonalities WG, also provides guidance to API subprojects and this is well-known.
  • These guidelines are the result of work done in the ICM and are documented in a document under the ownership of the ICM
  • For a document reader perspective, it is just one click browsing github documentation, which means nothing. There are many other references like that in the API design guidelines or the ICM profile itself and it is not a problem. It's like browsing through Confluence pages...

Options:

  • move openapi definitions to API Design guidelines
  • keep security scheme and info.description in access and user consent

Shouldn't all this be in camaraproject/IdentityAndConsentManagement#160 that is the issue open after discussion during the ICM meeting on 22 May??

@AxelNennker
Copy link
Contributor Author

right, thanks @jpengar

AxelNennker and others added 4 commits June 5, 2024 18:03
Co-authored-by: Elisabeth-Ericsson <121795930+Elisabeth-Ericsson@users.noreply.github.com>
Co-authored-by: Elisabeth-Ericsson <121795930+Elisabeth-Ericsson@users.noreply.github.com>
Co-authored-by: Elisabeth-Ericsson <121795930+Elisabeth-Ericsson@users.noreply.github.com>
Co-authored-by: Elisabeth-Ericsson <121795930+Elisabeth-Ericsson@users.noreply.github.com>
Copy link
Collaborator

@jpengar jpengar left a comment

Choose a reason for hiding this comment

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

Some suggested adjustments.

documentation/API-design-guidelines.md Outdated Show resolved Hide resolved
documentation/API-design-guidelines.md Outdated Show resolved Hide resolved
documentation/API-design-guidelines.md Outdated Show resolved Hide resolved
documentation/API-design-guidelines.md Outdated Show resolved Hide resolved
documentation/API-design-guidelines.md Outdated Show resolved Hide resolved
documentation/API-design-guidelines.md Outdated Show resolved Hide resolved
AxelNennker and others added 3 commits June 5, 2024 18:23
Co-authored-by: Jesús Peña García-Oliva <jesus.penagarcia-oliva@telefonica.com>
Co-authored-by: Jesús Peña García-Oliva <jesus.penagarcia-oliva@telefonica.com>
Co-authored-by: Jesús Peña García-Oliva <jesus.penagarcia-oliva@telefonica.com>
Co-authored-by: Jesús Peña García-Oliva <jesus.penagarcia-oliva@telefonica.com>
Copy link
Collaborator

@jpengar jpengar left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Collaborator

@shilpa-padgaonkar shilpa-padgaonkar left a comment

Choose a reason for hiding this comment

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

/LGTM

PedroDiez
PedroDiez previously approved these changes Jun 10, 2024
Copy link
Collaborator

@PedroDiez PedroDiez left a comment

Choose a reason for hiding this comment

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

Two minor points (if not possible to integrate, we can merge this and raise fix PR after)

LGTM in advance

documentation/API-design-guidelines.md Outdated Show resolved Hide resolved
documentation/API-design-guidelines.md Outdated Show resolved Hide resolved
Co-authored-by: Pedro Díez García <pedro.diezgarcia@telefonica.com>
Co-authored-by: Pedro Díez García <pedro.diezgarcia@telefonica.com>
Copy link
Collaborator

@patrice-conil patrice-conil left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@PedroDiez PedroDiez left a comment

Choose a reason for hiding this comment

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

LGTM

@rartych rartych merged commit 734792a into camaraproject:main Jun 11, 2024
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.

Adapt API Guidelines to ICM Securtiy and Interoperability Profile
8 participants