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

Resolution on where the documentation of ICM AuthN/AuthZ common guidelines for API specs must be located #160

Closed
jpengar opened this issue May 23, 2024 · 10 comments
Labels
documentation Improvements or additions to documentation

Comments

@jpengar
Copy link
Collaborator

jpengar commented May 23, 2024

Problem description

Current status: The ICM AuthN/AuthZ common guidelines for the CAMARA API specifications are currently documented in CAMARA-API-access-and-user-consent.md. So far it standardises the specification of securitySchemes and security openAPI fields across all CAMARA API subprojects with common mandatory guidelines as agreed by the TSC and the participants of this working group (see #57 and #93 for further details on the process resulting in this documentation).

An issue has been opened in Commonalities camaraproject/Commonalities#207 to reflect the decisions of Camara Security and Interoperablity Profile in Camara API Design Guidelines. It would be similar to the work done in ICM in #154 and #155 but with CAMARA-API-access-and-user-consent.md document.

A new PR camaraproject/Commonalities#208 has also been created to address issue camaraproject/Commonalities#207. However, this PR not only includes changes to the new ICM profile, but also moves without notice in ICM the information related to the securitySchemes and security guidelines specified in the ICM documentation, which are not part of the ICM profile document. This PR did not even include in the description that the PR was moving this piece of information as well (this was later corrected by @AxelNennker after it was raised in the PR comments). And the PR also changed the source information in the ICM documentation. This PR changes even the content of such a sensitive part as the info.description template, which needed a long review and approval by the ICM working group participants. Changing this information should be reviewed and approved by ICM.

After discussion during the ICM meeting on 22 May, it was agreed to create a new issue on the ICM working group github in order to make this issue visible to the ICM working group participants, let them express their opinions, and eventually agree on a resolution on where the documentation of the ICM AuthN/AuthZ common guidelines for API specs should be located: ICM doc vs. Commonalities doc.

CC @Elisabeth-Ericsson @tanjadegroot @diegogonmar @AxelNennker

Expected action

  • Adapt to ICM Security and Interoperability Profile Commonalities#208 must be set as DRAFT until a decision is made in the ICM WG. Or, if we don't want to block that PR, limit the PR scope just to the topics related to ICM profile removing the changes related to the the securitySchemes and security guidelines specified in ICM CAMARA-API-access-and-user-consent.md document.
  • ICM WG to express their opinions on this matter to make a final decision.
  • And then, if it is eventually needed, create PRs according to the final resolution reached at ICM WG.

Additional context

You can find additional context in the comments of:

@jpengar
Copy link
Collaborator Author

jpengar commented May 23, 2024

Now the current situation is described and the issue raised. From my point of view:

  • ICM, as Commonalities WG, also provides guidance to API subprojects and this is well-known.
  • IMHO, Adapt to ICM Security and Interoperability Profile Commonalities#208 is unilaterally moving information without notice from ICM to Commonalities and it basically decides on behalf of the ICM WG to do so. I agree that Commonalities should decide what is documented in the API Design Guidelines (Commonalities WG is the owner of that doc), but first the ICM WG should decide whether this information should be removed from the ICM or not. It is skipping any potential decision of the ICM WG. That's why this issue Resolution on where the documentation of ICM AuthN/AuthZ common guidelines for API specs must be located #160 has been open (among other things).
  • I think it is better to keep ICM and Commonalities documentation as is and keep the reference to the ICM documentation in Commonalities "API Design Guidelines" specifically for this securitytSchemes matter which has been very difficult to agree on. And I think the best place to have it is in the ICM, where our context of past discussions and decisions is. I think it will be much easier to handle, and I don't see the problem of having a reference in Commonalities that would take an API designer 1 click to resolve.
  • 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.

@shilpa-padgaonkar
Copy link
Collaborator

Thanks to ICM for defining the openid security scheme for all APIs to use.

However, I also see that the PR camaraproject/Commonalities#208, makes the design guidelines doc look more complete.
I see a similar example from the release management wg. The API versioning section, which was worked on and agreed in the release management working group also makes its way now into the design guidelines with the PR camaraproject/Commonalities#215 to ensure completeness of the design guidelines doc. If changes are needed to the API versioning section in the future, they are going to happen in alignment with release management WG by creating an issue in this WG.
The same is true for the alignment between Commonalities and ICM, if changes are needed in the future then they are going to happen in alignment with ICM by working on the issue in ICM.

IMHO I see benefit to the readers if all OpenAPI defintions are in one place in the API design guidelines doc.

@jpengar
Copy link
Collaborator Author

jpengar commented Jun 5, 2024

If changes are needed to the API versioning section in the future, they are going to happen in alignment with release management WG by creating an issue in this WG.
The same is true for the alignment between Commonalities and ICM, if changes are needed in the future then they are going to happen in alignment with ICM by working on the issue in ICM.

IMHO I see benefit to the readers if all OpenAPI defintions are in one place in the API design guidelines doc.

This has been a very polarising issue (the one related to securitySchemes), and it is already happening (with all the overhead and confusion that entails) that there are more than one open issue in Commonalities, API subprojects, Slack messages, and even multiple issues in the ICM just related to securitySchemes. 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. This is a BIG benefit IMHO to keep the information where it is now. As I said, in general I tend to agree with you that it is better to have everything in one place if possible, but for this matter I believe it will bring as more problems than benefits. From a document reader's perspective, it is just one click to browse the github documentation in ICM, which means nothing. There are plenty of other references like this in the API design guidelines or the ICM profile itself and it is not a problem. It's like browsing through the Confluence pages... I see a clear benefit, and I don't see a problem with keeping it the way it is.

@AxelNennker
Copy link
Collaborator

Copied from camaraproject/Commonalities#208 (comment)
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 Author

jpengar commented Jun 5, 2024

I vote in favour of keeping From my point of view, I would keep the information in ICM doc for the reasons I've given above.

@AxelNennker
Copy link
Collaborator

Please avoid the word "vote" :-)

@Elisabeth-Ericsson
Copy link
Contributor

Proposal is to keep the document https://github.com/camaraproject/IdentityAndConsentManagement/blob/main/documentation[/CAMARA-API-access-and-user-consent.md intact.
I made a proposal in https://github.com/camaraproject/Commonalities/pull/208/files#top to shorten the text, which is proposed there and instead point to the relevant sections in the CAMARA-API-access-and-user-consent.md#use-of-openidconnect-for-securityschemes.
I only concentrated on chapter 10.6.
I hope that this is a middle way, we can agree on.

@jpengar
Copy link
Collaborator Author

jpengar commented Jun 5, 2024

Proposal is to keep the document [https://github.com/camaraproject/IdentityAndConsentManagement/blob/main/documentation/CAMARA-API-access-and-user-consent.md intact. I made a proposal in https://github.com/camaraproject/Commonalities/pull/208/files#top to shorten the text, which is proposed there and instead point to the relevant sections in the CAMARA-API-access-and-user-consent.md#use-of-openidconnect-for-securityschemes. I only concentrated on chapter 10.6. I hope that this is a middle way, we can agree on.

For some reason, some of your suggestions in https://github.com/camaraproject/Commonalities/pull/208/files#top don't seem to be formatted correctly by github. But If I understand your suggestions correctly, it would be fine with me. Maybe @AxelNennker could try to apply your suggestions to confirm it.

@jpengar
Copy link
Collaborator Author

jpengar commented Jun 5, 2024

For some reason, some of your suggestions in https://github.com/camaraproject/Commonalities/pull/208/files#top don't seem to be formatted correctly by github. But If I understand your suggestions correctly, it would be fine with me. Maybe @AxelNennker could try to apply your suggestions to confirm it.

@AxelNennker has already done this. I've just added a few suggestions. So hopefully we can all agree to close this. CC @Elisabeth-Ericsson @shilpa-padgaonkar

@jpengar
Copy link
Collaborator Author

jpengar commented Jun 11, 2024

Commonalities PR camaraproject/Commonalities#208 is MERGED (with changes agreed after ICM 5 June meeting discussions) and issue camaraproject/Commonalities#207 is CLOSED. So, I'm going to close this issue after agree on a resolution on where and how to document the ICM AuthN/AuthZ common guidelines for API specs.

@jpengar jpengar closed this as completed Jun 11, 2024
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

No branches or pull requests

4 participants