fix: add client_id and client_secret to form when type is request-body

[scottohara]

In an OAuth2 password flow, when the type is request-body, the client_id and client_secret were being passed in an Authorization header and not in the request body.

Description

Refactored the authorizePassword function to properly handle the request-body case.

Motivation and Context

Fixes #4192

How Has This Been Tested?

1. Load the following sample API definition into Swagger UI: http://flask-restplus-example-server.herokuapp.com/api/v1/swagger.json
2. Click the Authorize button
3. Enter a username/password/client ID/client secret
4. Ensure that the type is set to Request Body
5. Click the Authorize button (note: the actual authorization will fail, but for the purposes of this PR it is not an issue)
6. Open the DevTools Network panel and check the form data in the request includes the username/password/client ID/client secret (see screenshot below)

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.

[heldersepu]

This logic could break some of the previous cases...
We had separate conditions for clientId & clientSecret, now it forces both

[scottohara]

Is it a valid use case to only supply one of those values, though?

I ask only because the branch of code that was previously defaulting to putting these in the Authorization header didn't include similar checks.

So in the case where where type was request-body, and only a client_id value was supplied, it previously would have set the header to Basic " + btoa("the_client_id:undefined"), which sounds like another lurking bug.

Happy to add back in the separate conditions, but I wanted to double check that supplying one or other value (id or secret, but not both) is something we're expecting people could be doing on purpose?

[shockey]

On hold pending @webron ☎️

[ThomHurks]

Why on earth is this not merged yet?

[heldersepu]

@ThomHurks Maybe you can offer some constructive details...

• Why is this important to you?
• How is this helpful to the community?
• Can you help adding some unit tests?

[ThomHurks]

I was the one who reported the bug, and it's a pretty serious one too. Because of this bug nobody can use the Oauth2 password grant authentication flow. So, that's a pretty big feature not to have, right? This would make swagger-ui unusable for quite a big number of people. There's a patch made, nobody got back to Scott's questions and then the patch was put on hold for 2 months, that doesn't seem like a good way to deal with this problem.

[heldersepu]

@ThomHurks
Can you help adding some unit tests?

[heldersepu]

Hey @ThomHurks we have not heard back from you ...

One of the things you mentioned:

nobody got back to Scott's questions

Is it a valid use case to only supply one of those values (clientId & clientSecret) , though?

I do not know enough about Oauth2 to give a precise answer...
But if it was separate conditions before I would keep it that way to avoid regressions.

[Ivan-L]

Hi @scottohara @heldersepu @ThomHurks,

We're just hitting this issue now after upgrading to SwaggerUI v3 (using Swashbuckle.AspNetCore v3).
It is a valid use case to only supply the client_id parameter and not the client_secret parameter, therefore the check should be changed. Prior versions of SwaggerUI required both parameters, so the older version of Swashbuckle.AspNetCore ended up sending a "na" string as the client secret when we did not supply one.

Irrespective of whether the client_id/client_secret check in this PR needs to be updated, without the contents of this PR in place, we cannot use the OAuth password grant flow through SwaggerUI either, whereas it did actually work in prior versions of SwaggerUI.

Is there any chance that this PR can be taken over the goal line?

[heldersepu]

@Ivan-L thanks for the report, this PR has been dormant for a while.
Your report confirms my initial comment the logic break some of the previous cases.

We still waiting for @ThomHurks to add some unit tests...
@ThomHurks Where on earth are those unit tests?

[scottohara]

To help progress this old PR, I've extracted the code for setting client_id and client_secret into a separate setClientIdAndSecret() function.

The logic has been modified so that either field (or both) can be supplied, and only the supplied values will be set in the target object (query or form).

Hopefully this is sufficient to get this over the line.

[shockey]

Thanks @scottohara - I'll take this off of ice now 😄

[enobrev]

I confirmed this PR on one of my own projects, and will be using it for the time being. I needed the client_id to be sent for a password grant-type.

The steps I took to confirm:

$ yarn add --dev swagger-api/swagger-ui#4213/head
$ cd node_modules/swagger-ui
$ yarn install
$ yarn run build

Then, in the interface, I tried authentication using the password grant-type and setting my client_id and it worked just fine, which was not working prior to these steps.

[shockey]

thanks @scottohara!