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

Fix form sending regression after StackExchange.Utils.Http upgrade to 0.3.41 #426

Merged
merged 2 commits into from
Aug 30, 2023

Conversation

dionrhys
Copy link
Contributor

@dionrhys dionrhys commented Aug 29, 2023

Problem

Commit cae2dd9 updated StackExchange.Utils.Http from 0.3.2 to 0.3.41. Between these versions, a breaking change landed in the package that changed SendForm from sending application/x-www-form-urlencoded requests to sending multipart/form-data requests instead. This appears to be the commit that introduced that change: StackExchange/StackExchange.Utils@30151e8

It looks like that change broke retrieving the OIDC access tokens in Opserver, because the RFC requires application/x-www-form-urlencoded requests for these, and providers like Okta enforce this requirement. Here's the error that Opserver raises when trying to authenticate over OIDC with Okta:

Error: failed to exchange authorization code for access token. BadRequest - {"errorCode":"E0000021","errorSummary":"Bad request. Accept and/or Content-Type headers likely do not match supported values.","errorLink":"E0000021","errorId":"~redacted~","errorCauses":[]}

Here's an Okta team member confirming they only allow application/x-www-form-urlencoded: https://devforum.okta.com/t/password-grant-bad-request-accept-and-or-content-type-headers-likely-do-not-match-supported-values/2346/2

Steps to reproduce

  1. Sign up for a Free tier Okta Developer account - https://developer.okta.com/signup/
  2. Create two groups opserver-view and opserver-admin in Okta, and assign your user to them
  3. Set up a new OIDC application for Opserver
  4. Configure Opserver with the following settings:
    "Security": {
      "provider": "OIDC",
      "viewEverythingGroups": "opserver-view",
      "adminEverythingGroups": "opserver-admin",
      "scopes": [ "openid", "groups", "profile" ],
      "clientId": "{clientid}",
      "clientSecret": "{clientsecret}",
      "authorizationUrl": "https://{orgsubdomain}.okta.com/oauth2/v1/authorize",
      "accessTokenUrl": "https://{orgsubdomain}.okta.com/oauth2/v1/token",
      "userInfoUrl": "https://{orgsubdomain}.okta.com/oauth2/v1/userinfo",
      "nameClaim": "preferred_username",
      "groupsClaim": "groups"
    }
  5. Run Opserver and attempt to login

With Opserver running on commit 1c4a755 (one before the StackExchange.Utils.Http upgrade in commit cae2dd9), sign in will work.

With Opserver running on commit cae2dd9 or the current main branch, sign in will fail.

With Opserver running with the changes in PR, sign in should work again.

Solution

This PR updates StackExchange.Utils.Http to 0.3.48 and makes use of a new SendFormUrlEncoded extension method that restores the previous behaviour of sending application/x-www-form-urlencoded form requests, everywhere that SendForm is used in this codebase.

The main fix I'm interested in is for OIDC, but this might be affecting the CloudFlare API and HAProxy web UI form requests as well. I don't have those available for testing but I figured I should revert the behaviour on those as well to be safe.

@NickCraver
Copy link
Member

Should this instead be an extension method in the Utils package, that we consume here?

@dionrhys
Copy link
Contributor Author

Sure thing, I'll look into adding that. I just need to track down how we publish a new Utils packages to nuget.org - it's been a while!

@NickCraver
Copy link
Member

@dionrhys If it's through MyGet, I can probably promote it still (as can @deanward81), trying to sync with the team on the long-term there.

NickCraver pushed a commit to StackExchange/StackExchange.Utils that referenced this pull request Aug 29, 2023
…oded form data (#27)

In #9, the `SendForm` extension method was changed from sending a `application/x-www-form-urlencoded` request to sending one as `multipart/form-data` instead. This is a breaking change in some cases if sending to an endpoint that doesn't expect form data to come in with `multipart/form-data` encoding (OIDC access token requests for example, see opserver/Opserver#426).

This PR re-adds the old behaviour as a new extension method so consuming libraries don't need to add it back themselves.

I expect `application/x-www-form-urlencoded` is the more common media type that consumers will want (when size limitations or file uploads aren't a concern), but I didn't want to revert `SendForm` back to its original behaviour to avoid breaking dependent projects again.

I could also obsolete `SendForm` and provide `SendMultipartForm` / `SendFormUrlEncoded` but I don't think it's important enough to inconvenience consumers with changing their code (and getting build errors using `TreatWarningsAsErrors`).
@NickCraver
Copy link
Member

@dionrhys Validating on NuGet now, should be available for use here shortly: https://www.nuget.org/packages/StackExchange.Utils.Http/0.3.48

@dionrhys
Copy link
Contributor Author

dionrhys commented Aug 29, 2023

@NickCraver Thank you! I've amended the PR to make use of the new version

EDIT: Re-pushed last commit to fix author/signing and added repro steps to op

@dionrhys dionrhys force-pushed the dion/fix-sendform-regression branch from efd5312 to 7863a09 Compare August 30, 2023 08:49
Copy link
Member

@NickCraver NickCraver left a comment

Choose a reason for hiding this comment

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

Looking good, thank you!

@NickCraver NickCraver merged commit b01ef3a into opserver:main Aug 30, 2023
@dionrhys dionrhys deleted the dion/fix-sendform-regression branch August 31, 2023 10:00
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.

2 participants