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

ciba-grant-3.2.0.md #3615

Merged
merged 16 commits into from
Apr 25, 2024
Merged

ciba-grant-3.2.0.md #3615

merged 16 commits into from
Apr 25, 2024

Conversation

shilpa-padgaonkar
Copy link
Contributor

@shilpa-padgaonkar shilpa-padgaonkar commented Feb 29, 2024

Below are details of the proposed changes in this PR to add CIBA grant flow:

  • The supported OAuth flows list has been extended with the CIBA flow
image
  • The OAuth flow object is extended with 3 new fields for CIBA
  • The authorizationUrl and tokenUrl fields are updated to specify that they are also applicable for the CIBA flow
image
  • The OAuth flow object example is extended with a CIBA flow example
image

This PR:
Fixes #3587

Branch and file used:

  • 3.2.0 spec: v3.2.0-dev branch, versions/3.2.0.md

@LasneF
Copy link
Member

LasneF commented Mar 21, 2024

@shilpa-padgaonkar :

we should upgrade as well the schema
https://github.com/OAI/OpenAPI-Specification/blob/v3.2.0-dev/schemas/v3.1/schema.yaml

@handrews this raise the point that the 3.2 should have its own schema version (compatible but still its own version)

to me the delivery mode should be an enum following the acceptable values
"backchannel_token_delivery_mode: REQUIRED. One of the following values: poll, ping, or push."

about user code , we may need to include it as part of the sample
not sure if as well it should not be a boolean , not clear why a string array

another point should it be called ciba , or should it be called backchannel as the OIDC spec mentionned those name as of parameter (ie it s just a legitimate question , to me ciba is fine as pointing the precise concept, not aware if there are other back channel technology leveraging those concept)

@darrelmiller
Copy link
Member

Could you help me understand why there is a need for a new OAuth2Flow when this information could be communicated via an OpenIDConnect configuration document considering it is an OIDC specific flow?

@AxelNennker
Copy link
Contributor

Could you help me understand why there is a need for a new OAuth2Flow when this information could be communicated via an OpenIDConnect configuration document considering it is an OIDC specific flow?

Hi @darrelmiller although it sounds like "OpenID Connect Client-Initiated Backchannel Authentication Flow" is the same as OpenId Connect is is not.
Also an OpenId Provider can offer CIBA and Authorization Code flow and then the API consumer would not know if the API only allows CIBA (or is intended to be used with CIBA).

This idea of introducing ciba to OAI comes out of the Camara project where we discovered the need to specify this flow.
Many Camara APIs use CIBA. Many APIs from MobileConnect also use CIBA but the flow was communicated offline and not documented in the e.g. openapi.yaml. We found that the flow should be part of the Camara API specification, and here we are.
Hope this explains why we need this and why openIdConnectUrl is not enough.

@shilpa-padgaonkar
Copy link
Contributor Author

@LasneF : Thanks for your feedback.

  • I have updated the schema files.
  • fixed the type of ciba_user_code to boolean
  • for ciba_delivery_modes, it is also possible that not just one, but rather 2 or all 3 of the delivery modes are selected.

versions/3.2.0.md Outdated Show resolved Hide resolved
versions/3.2.0.md Outdated Show resolved Hide resolved
@lornajane
Copy link
Contributor

Sorry, I know we've had some back-and-forth on the schemas, but since this change won't go into 3.1, it would be better to omit the schema updates from this pull request (the 3.2 schemas haven't been created yet), we can't merge this pull request with these schema changes in.

<a name="oauthFlowRefreshUrl"></a>refreshUrl | `string` | `oauth2` | The URL to be used for obtaining refresh tokens. This MUST be in the form of a URL. The OAuth2 standard requires the use of TLS.
<a name="oauthFlowScopes"></a>scopes | Map[`string`, `string`] | `oauth2` | **REQUIRED**. The available scopes for the OAuth2 security scheme. A map between the scope name and a short description for it. The map MAY be empty.
<a name="backchannel_token_delivery_modes_supported"></a>ciba_delivery_modes | Array[`string`] | `oauth2` (`"ciba"`) | **REQUIRED**. JSON array containing one or more of the following values: poll, ping, and push.
Copy link
Member

Choose a reason for hiding this comment

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

OpenAPI uses camelCasing for parameter values. So although the CIBA specification uses snake case, for consistency we should require tooling to do the translation and maintain consistency in the OpenAPI description.

Copy link
Member

@darrelmiller darrelmiller left a comment

Choose a reason for hiding this comment

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

Remove OPTIONAL keywords and change casing of parameters

@handrews
Copy link
Member

@darrelmiller

Remove OPTIONAL keywords and change casing of parameters

I presume you meant "Remove OPTIONAL from keywords..." rather than removing the keywords marked OPTIONAL

shilpa-padgaonkar and others added 2 commits April 18, 2024 21:17
Co-authored-by: Darrel <darrmi@microsoft.com>
Co-authored-by: Darrel <darrmi@microsoft.com>
versions/3.2.0.md Outdated Show resolved Hide resolved
versions/3.2.0.md Outdated Show resolved Hide resolved
versions/3.2.0.md Outdated Show resolved Hide resolved
versions/3.2.0.md Outdated Show resolved Hide resolved
versions/3.2.0.md Outdated Show resolved Hide resolved
versions/3.2.0.md Outdated Show resolved Hide resolved
versions/3.2.0.md Outdated Show resolved Hide resolved
@shilpa-padgaonkar
Copy link
Contributor Author

Sorry, I know we've had some back-and-forth on the schemas, but since this change won't go into 3.1, it would be better to omit the schema updates from this pull request (the 3.2 schemas haven't been created yet), we can't merge this pull request with these schema changes in.

@lornajane Reverted schema changes

@shilpa-padgaonkar
Copy link
Contributor Author

@darrelmiller Done changes for camelCase. Kindly check.

versions/3.2.0.md Outdated Show resolved Hide resolved
Co-authored-by: Ralf Handl <ralf.handl@sap.com>
@lornajane lornajane requested a review from darrelmiller April 25, 2024 16:40
Copy link
Contributor

@lornajane lornajane left a comment

Choose a reason for hiding this comment

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

Thank you for being patient with us and working through the amendments.

@lornajane lornajane dismissed darrelmiller’s stale review April 25, 2024 16:41

changes were made

@lornajane lornajane merged commit 66f90d6 into OAI:v3.2.0-dev Apr 25, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
security: auth Authentication including overlap with authorization security
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants