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

Documentation update with securitySchemes final agreement #93

Merged
merged 3 commits into from
Jan 8, 2024

Conversation

jpengar
Copy link
Collaborator

@jpengar jpengar commented Dec 5, 2023

What type of PR is this?

Add one of the following kinds:

  • documentation

What this PR does / why we need it:

This PR is intended to include in the WG documentation the final agreement reached by the TSC and the WG participants on the securitySchemes and security of the CAMARA API specification, and the mandatory template for info.description, explaining to API customers why the API specification does not list specific grant types, and how to find out what authorization flows they can use.

Which issue(s) this PR fixes:

Fixes #57

Special notes for reviewers:

Changelog input

 CAMARA API Specification - Authorization and authentication common guidelines

Additional documentation

N/A

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.

LGTM

Copy link
Collaborator

@diegogonmar diegogonmar left a comment

Choose a reason for hiding this comment

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

LGTM

```
### Authorization and authentication

CAMARA guidelines defines a set of authorization flows which can grant API clients access to the API functionality, as outlined in the document [CAMARA-API-access-and-user-consent.md](https://github.com/camaraproject/IdentityAndConsentManagement/blob/main/documentation/CAMARA-API-access-and-user-consent.md). Which specific authorization flows are to be used will be determined during onboarding process, happening between the API Client and the Telco Operator exposing the API, taking into account the declared purpose for accessing the API, while also being subject to the prevailing legal framework dictated by local legislation.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Document "CAMARA-API-access-and-user-consent.md" says nothing about client credentials, and yet that remains a valid authorisation scheme for some CAMARA API endpoints. When it was proposed that CAMARA adopt type: openIdConnect as the ONLY valid security scheme, this was justified on the basis that .well-known/openid-configuration allows client_credentials in the list of grant_types_supported. And yet here we have a reference to a document that does not "outline" this as a possibility.

So for the proposed text to be applicable, "CAMARA-API-access-and-user-consent.md" must be updated to list client credentials as a valid authorisation scheme for some scenarios.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agree. This is a fair point. We will commit an update to clarify that client credentials is a valid authorization scheme for some scenarios.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done in commit 4f09ec6

Copy link
Collaborator

@murthygorty murthygorty left a comment

Choose a reason for hiding this comment

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

In terms and concepts, can you consider changing "User" term to "Subscriber" please? If approved and changed, we will have

  1. End User -> human participant using the application (can be a parent or alternate delegate of the carrier-resource)
  2. Subscriber -> subscriber of the telco operator, owner of the carrier-resource at hand. (eg., parent)

@shilpa-padgaonkar
Copy link
Collaborator

@jpengar: Instead of replicating all info from AuthN-AuthZ doc again in this document, may be it is easier to simply say that

CAMARA guidelines defines a set of authorization flows which can grant API clients access to the API functionality, as outlined in the document CAMARA-AuthN-AuthZ-Concept.md.

All possible grant types are already documented in this doc.

@jpengar
Copy link
Collaborator Author

jpengar commented Dec 20, 2023

In terms and concepts, can you consider changing "User" term to "Subscriber" please? If approved and changed, we will have

End User -> human participant using the application (can be a parent or alternate delegate of the carrier-resource)
Subscriber -> subscriber of the telco operator, owner of the carrier-resource at hand. (eg., parent)

@murthygorty This specific topic is not related to the original issue #57 or the content included in this PR. Would you kindly open a new issue to discuss this specific topic? thank you so much in advance.
I always encourage WG participants to keep the original scope of the discussion to avoid creating unnecessary dependencies, to keep the discussion focused on the topic, and to prevent the discussion from becoming deadlocked.

UPDATE: I just saw a new issue of OIDF (#98) that's pretty much related. We can move this discussion there.

@jpengar
Copy link
Collaborator Author

jpengar commented Dec 20, 2023

@jpengar: Instead of replicating all info from AuthN-AuthZ doc again in this document, may be it is easier to simply say that

CAMARA guidelines defines a set of authorization flows which can grant API clients access to the API functionality, as outlined in the document CAMARA-AuthN-AuthZ-Concept.md.

All possible grant types are already documented in this doc.

@shilpa-padgaonkar That document actually mentions additional grant types/flows to those considered and defined so far as a valid option to access CAMARA APIs: Auth Code Flow, CIBA, and Client Credentials.

@shilpa-padgaonkar
Copy link
Collaborator

@jpengar : That is indeed correct. My main concern is that we will soon get requests to cover more options here if not explicitly specified for eg. there might be a request to add details about PKCE or other possible extensions.

Another option would be that we keep the link as is, in your PR, but mention that details about possible extensions etc. can be found in the AuthN-AuthZ doc.

Also, for recommended flows under oauth2, just like client credentials we should also specify auth code flow.

@jpengar
Copy link
Collaborator Author

jpengar commented Dec 20, 2023

Another option would be that we keep the link as is, in your PR, but mention that details about possible extensions etc. can be found in the AuthN-AuthZ doc.

Also, for recommended flows under oauth2, just like client credentials we should also specify auth code flow.

@shilpa-padgaonkar Could you use the github.com change suggestion feature to add your suggestion to the current status of the PR? That way we avoid a trail and error process in case I don't fully understand your point. :S

@@ -18,6 +18,11 @@ This document defines guidelines for telco operator exposure platforms to manage
- [Authorization flows / grant types](#authorization-flows--grant-types)
- [Authorization code flow (Frontend flow)](#authorization-code-flow-frontend-flow)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- [Authorization code flow (Frontend flow)](#authorization-code-flow-frontend-flow)
Detailed information on the below specified grant types (for eg. extensions to some of the grant types, links to original RFC documents etc.) is available in [CAMARA-AuthN-AuthZ-Concept.md](https://github.com/camaraproject/IdentityAndConsentManagement/blob/main/documentation/CAMARA-AuthN-AuthZ-Concept.md)
- [Authorization code flow (Frontend flow)](#authorization-code-flow-frontend-flow)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We cannot just put this paragraph there. You have to put it in the middle of the auto-generated index. When the document is saved, the index is regenerated and this change is lost.

If you want to include it, it might fit as a NOTE at the beginning of the Authorization Flows / Grant Types section.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in d20fe33

@jpengar
Copy link
Collaborator Author

jpengar commented Dec 21, 2023

@shilpa-padgaonkar @rartych @bigludo7 @eric-murray As agreed in the 20/12 WG meeting call, now that the PR has already addressed your change suggestions, could you do a final review to merge the PR and close the issue?

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

Copy link
Collaborator

@rartych rartych 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

@AxelNennker AxelNennker left a comment

Choose a reason for hiding this comment

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

LGTM

@murthygorty
Copy link
Collaborator

@jpengar
about this, you are 100% right, PR comments should only be about the issue at hand. Thanks, will follow #98

@jpengar jpengar merged commit 4741eaf into main Jan 8, 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.

Align securitySchemes and security of CAMARA API specs with IdentityAndConsentManagement WG
9 participants