opserver / Opserver

Stack Exchange's Monitoring System
https://opserver.github.io/Opserver/
MIT License
4.51k stars 828 forks source link

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

Closed dionrhys closed 1 year ago

dionrhys commented 1 year ago

Problem

Commit cae2dd9cb3b8f37802f60b10b1864583c1dfa0fc 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: https://github.com/StackExchange/StackExchange.Utils/commit/30151e875bdd8c243cb17c352cefe6133b6ca2a1

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 1c4a7558476d8005f48650b0969d6b2d34ee1d22 (one before the StackExchange.Utils.Http upgrade in commit cae2dd9cb3b8f37802f60b10b1864583c1dfa0fc), sign in will work.

With Opserver running on commit cae2dd9cb3b8f37802f60b10b1864583c1dfa0fc 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 commented 1 year ago

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

dionrhys commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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

dionrhys commented 1 year ago

@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