feat: do not ask for client secret when using auth code with PKCE #7438

ch-egli · 2021-07-28T07:19:54Z

According to OAuth 2.0 Security Best Current Practice the implicit grant should no longer be used, but be replaced by the authorization code grant with PKCE.
While the swagger-ui permits to use the PKCE grant type, the authorization dialog still prompts for a client secret. From a user perspective, this is confusing and the current pull request omits the client secret field if authorization code grant with PKCE is configured (see #6290).

Description

When the flag usePkceWithAuthorizationCodeGrant is set to true, the input field for the client secret will not be shown.
Moreower, we specify in the authorization dialogue, that the authorization code flow with PKCE is used.

Motivation and Context

See description above.
Fixes #6290

How Has This Been Tested?

generated a new dist and included it in different projects of Schweizerische Bundesbahnen (SBB)
cypress e2e test added

Screenshots (if appropriate):

Checklist

My PR contains...

No code changes (src/ is unmodified: changes to documentation, CI, metadata, etc.)
Dependency changes (any modification to dependencies in package.json)
Bug fixes (non-breaking change which fixes an issue)
Improvements (misc. changes to existing features)
Features (non-breaking change which adds functionality)

My changes...

are breaking changes to a public API (config options, System API, major UI change, etc).
are breaking changes to a private API (Redux, component props, utility functions, etc.).
are breaking changes to a developer API (npm script behavior changes, new dev system dependencies, etc).
are not breaking changes.

Documentation

My changes do not require a change to the project documentation.
My changes require a change to the project documentation.
If yes to above: I have updated the documentation accordingly.

Automated tests

My changes can not or do not need to be tested.
My changes can and should be tested by unit and/or integration tests.
If yes to above: I have added tests to cover my changes.
If yes to above: I have taken care to cover edge cases in my tests.
All new and existing tests passed.

… with PKCE (issue swagger-api#6290)

…ecret

Messhias · 2021-08-06T12:03:28Z

@swaggerhub-bot anyone to check this?

…ecret

ilozano2 · 2021-09-13T10:11:20Z

This looks really good @Messhias

I'd appreciate having this improvement merged. Is there anything I can do to help?

Messhias · 2021-09-13T10:54:28Z

This looks really good @Messhias

I'd appreciate having this improvement merged. Is there anything I can do to help?

You can update the current PR with the base branch for workflow approval.

ilozano2 · 2021-09-13T11:14:19Z

@Messhias I don't have permission to update @ch-egli fork. Can I create a new PR picking his commits to get a more updated PR?

Messhias · 2021-09-13T15:45:51Z

@Messhias I don't have permission to update @ch-egli fork. Can I create a new PR picking his commits to get a more updated PR?

In fact you should be able to do updates and push commits on this branch.

ilozano2 · 2021-09-14T07:13:16Z

@Messhias I cannot edit any file on this PR (I guess I am not a maintainer on this repo or the PR wasn't marked to be editable by maintainers).

I've just created a copy of this PR #7498 enabling the edition from maintainers.

Messhias · 2021-09-14T09:29:39Z

@Messhias I cannot edit any file on this PR (I guess I am not a maintainer on this repo or the PR wasn't marked to be editable by maintainers).

I've just created a copy of this PR #7498 enabling the edition from maintainers.

Ok, I'll check this new PR and let you know. Thank you.

ch-egli · 2021-09-14T11:10:33Z

Hi all
I have marked the pull request with Allow edits by maintainers.
Can I do anything else?

Messhias · 2021-09-14T12:48:58Z

Hi all
I have marked the pull request with Allow edits by maintainers.
Can I do anything else?

No, only the repository owner/maintainers can do the stuff of merging and etc... What could be done is the repository owner add more maintainers to keep the package evolve more constantly and quickly.

ponelat

Thanks @ch-egli and @Messhias @ilozano2 ! This is great work, first time I'm learning about the PKCE extension. Added a small comment.

Will try to get this in the pipeline for a core contributor to review.

ponelat · 2021-09-14T12:58:32Z

src/core/components/auth/oauth2.jsx

    let flow = schema.get("flow")
+    let flowToDisplay = isPkceCodeGrant ? flow + " with PKCE" : flow


pkce only applies to authorization flow yeah? This would show " with PKCE" even if the implicit flow is used. Perhaps we should ignore the usePkCE flag if flows aren't authorization.

AFAIK, PKCE is an improvement on top of the authorization_code so public clients don't need to expose the client_secret in their code.

I would keep the pkce flag there because there is no way to determine if pkce is enabled in the server using the openid-configuration file. On the other hand, even a user can figure out that it is pkce because client_secret is not presented, I'd keep "with PKCE" title ending.

The code looks good to me

@ponelat: Thank you for pointing this out to me. You are correct, you really only want to show this additional text if authorizationCode flow is used. I have updated the corresponding code in my fork of the repository by adding an additional test.

…ce-without-secret

…kceCodeGrant are given

…-without-secret' into feature/6290-auth-code-flow-pkce-without-secret

…ecret

ilozano2 · 2021-10-19T14:15:24Z

Hi @ponelat I was wondering if there is any update for this PR, please?

ponelat · 2021-10-19T16:08:38Z

@ilozano2 thanks for the ping. I think it looks just dandy, we may not get a merge this week, as we finish off the current release, but likely next week. cc @char0n

…ecret

ilozano2 · 2022-01-19T14:30:15Z

@ponelat any news about this improvement?

char0n · 2022-01-19T15:13:22Z

@ilozano2 yep, this is planned to be included in 4.4.0 release.

ponelat · 2022-01-25T13:03:53Z

Apologies @ilozano2 I dropped the ball on this one!

…ecret

char0n

Thanks for contributing! This PR is good to go. There are two suggestions that I've made, which IMHO are more appropriate given SwaggerUI uses non-mutating mechanism of redux.

test/e2e-cypress/tests/features/auth-code-flow-pkce-without-secret.js

chrklin · 2022-03-24T12:09:03Z

Hmm does this break the client credentials. I have 2 flows, one with auth code that requires pkce and a second that requires client credentials. Seems like I can't add the secret for client_credentials flow anymore because I need pkce on my authorization code flow

chrklin · 2022-03-24T12:18:09Z

Hmm does this break the client credentials. I have 2 flows, one with auth code that requires pkce and a second that requires client credentials. Seems like I can't add the secret for client_credentials flow anymore because I need pkce on my authorization code flow

(flow === AUTH_FLOW_APPLICATION || flow === AUTH_FLOW_ACCESS_CODE || flow === AUTH_FLOW_PASSWORD) && !isPkceCodeGrant &&

should have been

(flow === AUTH_FLOW_APPLICATION || (flow === AUTH_FLOW_ACCESS_CODE && !isPkceCodeGrant) || flow === AUTH_FLOW_PASSWORD) &&

PKCE and Client Secrets are allowed to coexist and neither is designed as a replacement for the other. [1] It is wrong to assume that a client secret must not or cannot be used in combination with PKCE. Quite the opposite, when possible both PKCE and client secret should be used. [2] So the premises of swagger-api#6290 and swagger-api#8146 are not correct. Admittedly, for users of the PKCE mechanism WITHOUT a client secret it might be a minor nuisance to see the client secret input in the Swagger UI. But they can just leave it empty. On the other hand, for users of the PKCE mechanism WITH a client secret it is more than just a nuisance if the client secret input is not shown. The Swagger UI becomes unusable for them (unless they've set a default value for the client secret, which will be used hiddenly without being shown to the user). Therefore the right course of action for now would be to revert swagger-api#7438 to show the client secret input always regardless of PKCE. In the future a new flag could be introduced to hide the client secret input regardless of the PKCE flag. [1] https://oauth.net/2/pkce/ [2] https://www.oauth.com/oauth2-servers/pkce/

* fix: show client secret input for PKCE auth code flow PKCE and Client Secrets are allowed to coexist and neither is designed as a replacement for the other. [1] It is wrong to assume that a client secret must not or cannot be used in combination with PKCE. Quite the opposite, when possible both PKCE and client secret should be used. [2] So the premises of #6290 and #8146 are not correct. Admittedly, for users of the PKCE mechanism WITHOUT a client secret it might be a minor nuisance to see the client secret input in the Swagger UI. But they can just leave it empty. On the other hand, for users of the PKCE mechanism WITH a client secret it is more than just a nuisance if the client secret input is not shown. The Swagger UI becomes unusable for them (unless they've set a default value for the client secret, which will be used hiddenly without being shown to the user). Therefore the right course of action for now would be to revert #7438 to show the client secret input always regardless of PKCE. In the future a new flag could be introduced to hide the client secret input regardless of the PKCE flag. [1] https://oauth.net/2/pkce/ [2] https://www.oauth.com/oauth2-servers/pkce/ * docs: explain why client secret input is shown despite PKCE

feat: do not ask for client secret when using authorization code flow…

6074b99

… with PKCE (issue swagger-api#6290)

ch-egli changed the title ~~feat: do not ask for client secret when using authorization code flow…~~ feat: do not ask for client secret when using auth code with PKCE Jul 28, 2021

Merge branch 'master' into feature/6290-auth-code-flow-pkce-without-s…

2ba25d0

…ecret

Messhias approved these changes Aug 6, 2021

View reviewed changes

ch-egli added 2 commits August 10, 2021 10:47

Merge branch 'master' into feature/6290-auth-code-flow-pkce-without-s…

50018e6

…ecret

Merge branch 'master' into feature/6290-auth-code-flow-pkce-without-s…

3ebc5e7

…ecret

ilozano2 mentioned this pull request Sep 14, 2021

feat: do not ask for client secret when using auth code with PKCE #7498

Closed

11 tasks

ponelat reviewed Sep 14, 2021

View reviewed changes

ponelat added cat: auth type: enhancement labels Sep 14, 2021

ch-egli and others added 4 commits September 14, 2021 18:23

Merge branch 'swagger-api:master' into feature/6290-auth-code-flow-pk…

2ba9951

…ce-without-secret

make sure that description is only updated if both authCodeFlow and P…

6bef6c4

…kceCodeGrant are given

Merge remote-tracking branch 'origin/feature/6290-auth-code-flow-pkce…

6c5265e

…-without-secret' into feature/6290-auth-code-flow-pkce-without-secret

Merge branch 'master' into feature/6290-auth-code-flow-pkce-without-s…

77cb0f3

…ecret

ilozano2 approved these changes Sep 16, 2021

View reviewed changes

Merge branch 'master' into feature/6290-auth-code-flow-pkce-without-s…

95fa4c0

…ecret

ponelat mentioned this pull request Sep 17, 2021

[Announcement] OAuth2.1 and OAuth3 drafts OAI/OpenAPI-Specification#2248

Open

ilozano2 added 2 commits November 17, 2021 09:42

Merge branch 'master' into feature/6290-auth-code-flow-pkce-without-s…

370c1fc

…ecret

Merge branch 'master' into feature/6290-auth-code-flow-pkce-without-s…

8395fc5

…ecret

char0n self-assigned this Jan 25, 2022

Merge branch 'master' into feature/6290-auth-code-flow-pkce-without-s…

ced9f55

…ecret

char0n approved these changes Jan 25, 2022

View reviewed changes

test/e2e-cypress/tests/features/auth-code-flow-pkce-without-secret.js Show resolved Hide resolved

test/e2e-cypress/tests/features/auth-code-flow-pkce-without-secret.js Show resolved Hide resolved

Update auth-code-flow-pkce-without-secret.js

9720362

char0n merged commit c6d8d77 into swagger-api:master Jan 26, 2022

Pilchard123 mentioned this pull request Mar 16, 2022

authorization_code flow with PKCE does not allow entering a client secret #7913

Closed

chrklin mentioned this pull request Mar 24, 2022

Hiding of password field with PKCE enabled, prevents usage of client_credentials, when API has multiple authentication methods. #7939

Open

Risthart mentioned this pull request Aug 4, 2022

fix: client_secret doesn't appear in non-authcode flows while usnig PKCE #8146

Merged

17 tasks

Phoosha mentioned this pull request Oct 31, 2022

fix: show client secret input for PKCE auth code flow #8268

Merged

17 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: do not ask for client secret when using auth code with PKCE #7438

feat: do not ask for client secret when using auth code with PKCE #7438

ch-egli commented Jul 28, 2021 •

edited

Messhias commented Aug 6, 2021

ilozano2 commented Sep 13, 2021

Messhias commented Sep 13, 2021

ilozano2 commented Sep 13, 2021

Messhias commented Sep 13, 2021

ilozano2 commented Sep 14, 2021

Messhias commented Sep 14, 2021

ch-egli commented Sep 14, 2021

Messhias commented Sep 14, 2021 •

edited

ponelat left a comment

ponelat Sep 14, 2021

ilozano2 Sep 14, 2021

ch-egli Sep 14, 2021

ilozano2 commented Oct 19, 2021

ponelat commented Oct 19, 2021

ilozano2 commented Jan 19, 2022

char0n commented Jan 19, 2022 •

edited

ponelat commented Jan 25, 2022

char0n left a comment

chrklin commented Mar 24, 2022

chrklin commented Mar 24, 2022

		let flow = schema.get("flow")
		let flowToDisplay = isPkceCodeGrant ? flow + " with PKCE" : flow

feat: do not ask for client secret when using auth code with PKCE #7438

feat: do not ask for client secret when using auth code with PKCE #7438

Conversation

ch-egli commented Jul 28, 2021 • edited

Description

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Checklist

My PR contains...

My changes...

Documentation

Automated tests

Messhias commented Aug 6, 2021

ilozano2 commented Sep 13, 2021

Messhias commented Sep 13, 2021

ilozano2 commented Sep 13, 2021

Messhias commented Sep 13, 2021

ilozano2 commented Sep 14, 2021

Messhias commented Sep 14, 2021

ch-egli commented Sep 14, 2021

Messhias commented Sep 14, 2021 • edited

ponelat left a comment

Choose a reason for hiding this comment

ponelat Sep 14, 2021

Choose a reason for hiding this comment

ilozano2 Sep 14, 2021

Choose a reason for hiding this comment

ch-egli Sep 14, 2021

Choose a reason for hiding this comment

ilozano2 commented Oct 19, 2021

ponelat commented Oct 19, 2021

ilozano2 commented Jan 19, 2022

char0n commented Jan 19, 2022 • edited

ponelat commented Jan 25, 2022

char0n left a comment

Choose a reason for hiding this comment

chrklin commented Mar 24, 2022

chrklin commented Mar 24, 2022

ch-egli commented Jul 28, 2021 •

edited

Messhias commented Sep 14, 2021 •

edited

char0n commented Jan 19, 2022 •

edited