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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 2 commits into from Aug 10, 2022
Merged

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

merged 2 commits into from Aug 10, 2022

Conversation

Risthart
Copy link
Contributor

@Risthart Risthart commented Aug 4, 2022

While using PKCE option (usePkceWithAuthorizationCodeGrant flag is set to true), client_secret field is hidden under non-auth_code authorization flows.

PKCE option hides client_secret field in every flow in auth component. In general PKCE should not be a replacement for a client secret (https://oauth.net/2/pkce/). So may be all the changes affecting client_secret should be reverted #7438?

Description

Made the flag usePkceWithAuthorizationCodeGrant affect only auth_code grant.

Motivation and Context

While using client credentials flow, I can't enter client secret into corresponding field (the field is hidden).

Screenshots (if appropriate):

There is no client_secret option for client credentials flow when usePkceWithAuthorizationCodeGrant is set to true.
image

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.

@tim-lai tim-lai merged commit c63737d into swagger-api:master Aug 10, 2022
@tim-lai
Copy link
Contributor

tim-lai commented Aug 10, 2022

@Risthart PR merged! Thanks for the fix and contribution!

@Pilchard123 Pilchard123 mentioned this pull request Aug 26, 2022
Closed
This was referenced Oct 31, 2022
Closed
Open
Phoosha added a commit to Phoosha/swagger-ui that referenced this pull request Oct 31, 2022
@Phoosha
fix: show client secret input for PKCE auth code flow
3d22ec1 
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/
@Phoosha Phoosha mentioned this pull request Oct 31, 2022
Merged
17 tasks
Phoosha added a commit to Phoosha/swagger-ui that referenced this pull request Nov 2, 2022
@Phoosha
fix: show client secret input for PKCE auth code flow
645ecd5 
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/
tim-lai pushed a commit that referenced this pull request Nov 4, 2022
@Phoosha
fix: show client secret input for PKCE auth code flow (#8268)
7b0ac1a 
* 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
@rwb196884
Copy link

rwb196884 commented Jan 3, 2023 •
edited

It's still broken in 6.4.0.

The client_credentials flow always requires client_secret, but code should not resp. should have a client_secret according to whether

    app.UseSwaggerUI(c =>
    {
        c.OAuthUsePkce(); // bug: determines wither client_secret field is either present or absent for all flows.
    });

However, turning on PKCE causes the client_secret field disappears from both cases.
Thereore at least one case cannot work.

Furthermore, it will generally be useful to set different default client id values (c.OAuthClientId(...)) for the different flows.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Risthart @tim-lai @rwb196884