Make it choosable where client id and secret are put and improve expi… #3970
Conversation
| isBrowsable: true, | ||
| isReadOnly: false, | ||
| label: 'Credentials location', | ||
| name: 'CredentialsLocation', |
There was a problem hiding this comment.
Note: in case you'd want to use the standard term, it's officially called "token endpoint client authentication method".
| items: [ | ||
| { | ||
| text: 'Authorization header', | ||
| value: 'header', |
There was a problem hiding this comment.
If you want to use the standard values, it's client_secret_basic for that.
| }, | ||
| { | ||
| text: 'Form data', | ||
| value: 'form', |
| @@ -67,6 +70,10 @@ public async Task<TokenResponse> GetToken(Secret secret, string? authCode, strin | |||
| { | |||
| var httpClient = _httpClientFactory.CreateClient(nameof(OAuth2TokenService)); | |||
| httpClient.DefaultRequestHeaders.Accept.Add(new MediaTypeWithQualityHeaderValue("application/json")); | |||
| if (credentialsLocation == "header") | |||
| { | |||
| httpClient.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue("Basic", Base64Encode(clientId + ":" + clientSecret)); | |||
There was a problem hiding this comment.
Note: while many servers implement it this way, your encoding mechanism is not standard: you must formURL-encode the client identifier and secret before base64-encoding them (i.e do Uri.EscapeDataString(value).Replace("%20", "+") on each value).
Depending on the OAuth 2.0 services you want to support, you may want to make that configurable.
There was a problem hiding this comment.
Also, you're not really supposed to use the Authorization header option when you don't have a client secret (in this case, you need to send the client_id alone in the form), but some servers (e.g Reddit) don't implement that correctly and require a quirk.
There was a problem hiding this comment.
I added escaping, and since it is standard, and seems to be working with google, I wouldn't make it configurable. That secret modal is getting a bit bloated already, so if someone requires it, they can expand upon this feature.
About your second comment, I guess the one who setups the credentials, would just choose the client_secret_post option and not set secret, wouldn't they?
There was a problem hiding this comment.
I guess so. In this case, you'd probably want to add an if to avoid sending an empty client_secret... just in case 😄
Also, Uri.EscapeDataString(clientSecret) will throw if the secret is null (not sure if public clients is a scenario you support).
Make it choosable where client id and secret are put and improve expiration check