search

camaraproject / Commonalities

Repository to describe, develop, document and test the common guidelines and assets for CAMARA APIs
Apache License 2.0
9 stars 24 forks source link

Adapt to ICM Security and Interoperability Profile #208

Closed AxelNennker closed 3 weeks ago

AxelNennker commented 1 month ago

What type of PR is this?

Add one of the following kinds:

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.

Which issue(s) this PR fixes:

Fixes #207

jpengar commented 1 month ago

@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 (https://github.com/camaraproject/IdentityAndConsentManagement/pull/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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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.

AxelNennker commented 1 month ago

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 :

Contra moving arguments:

Options:

jpengar commented 1 month ago

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 https://github.com/camaraproject/IdentityAndConsentManagement/issues/160 that is the issue open after discussion during the ICM meeting on 22 May??

AxelNennker commented 1 month ago

right, thanks @jpengar