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 authorization code flow to work with openiddict library #3951

Merged
merged 1 commit into from Apr 25, 2023
Merged

Fix authorization code flow to work with openiddict library #3951

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 6 additions & 4 deletions src/modules/secrets/Elsa.Secrets.Http/Services/OAuth2TokenService.cs
View file
Open in desktop
Expand Up @@ -48,23 +48,25 @@ public async Task<TokenResponse> GetToken(Secret secret, string? authCode, strin
{ "grant_type", secret.GetProperty("GrantType") },
{ "client_id", clientId },
{ "client_secret", clientSecret },
{ "scope", secret.GetProperty("Scope") }
{ "offline_access ", "true" }
Copy link

Choose a reason for hiding this comment

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

Hey 👋🏻

FYI, offline_access is not a standard parameter (and not used by OpenIddict unless you add custom logic to handle it). It also seems there's an extra space at the end of the parameter name so it's likely something you'll want to fix/remove 😃

Copy link
Author

Choose a reason for hiding this comment

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

Damn, you are right. "offline_access" is probably not required indeed, but was something I added when trying to get refresh tokens to work. I will create a new PR to remove this.

Copy link
Member

Choose a reason for hiding this comment

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

@tanelkuhi If you create a PR with the required corrections we may be able to include it in a 2.11.2 release soon

};

if (authCode != null)
{
content.Add("code", authCode);
}
else
{
content.Add("scope", secret.GetProperty("Scope"));
}
if (redirectUri != null)
{
content.Add("redirect_uri", redirectUri);
}

try
{
var httpClient = _httpClientFactory.CreateClient(nameof(OAuth2TokenService));
httpClient.DefaultRequestHeaders.Accept.Add(new MediaTypeWithQualityHeaderValue("application/json"));
httpClient.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue("Basic", Base64Encode(clientId + ":" + clientSecret));
Copy link

Choose a reason for hiding this comment

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

Note: OpenIddict supports sending the client credentials in the Authorization header (basic authentication) or in the request form, but as per the spec, it's illegal to send credentials using both methods (which is likely why you were getting an error).

That said, if this client is expected to be used with other vendors that don't support sending the credentials in the request form, this change might break them. You may want to add an option to control how the credentials are sent 😃

Copy link
Author

@tanelkuhi tanelkuhi Apr 25, 2023 •
edited

Choose a reason for hiding this comment

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

Yep, I indeed got an error, because the credentials were specified in 2 ways. Do you think there are vendors, that support only credentials in request form and not in headers?

Copy link

Choose a reason for hiding this comment

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

Do you think there are vendors, that support only credentials in request form and not in headers?

Yeah, a lot, sadly (even if supporting basic authentication is - in theory - mandatory for an OAuth 2.0 server: https://www.rfc-editor.org/rfc/rfc6749#section-2.3.1)

Copy link
Author

Choose a reason for hiding this comment

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

Made a new PR:
#3970


var response = await httpClient.PostAsync(secret.GetProperty("AccessTokenUrl"), new FormUrlEncodedContent(content));
var json = await response.Content.ReadAsStringAsync();
Expand Down Expand Up @@ -105,7 +107,7 @@ public async Task<TokenResponse> GetTokenByRefreshToken(Secret secret, string re
var json = await response.Content.ReadAsStringAsync();
var result = JsonConvert.DeserializeObject<TokenResponse>(json);

if (response.StatusCode == HttpStatusCode.Unauthorized || result?.Error == "access_denied")
if (response.StatusCode == HttpStatusCode.Unauthorized || !string.IsNullOrEmpty(result?.Error))
{
secret.RemoveProperty("Token");
await _secretsStore.UpdateAsync(secret);
Expand Down