From f2c9e7b2347e4892a355c232d93dd2129d026410 Mon Sep 17 00:00:00 2001 From: Tim Van Holder Date: Sat, 30 Dec 2023 14:45:08 +0100 Subject: [PATCH 1/2] Add PKCE support Note that this breaks the API of the `CreateAuthorizationRequest` and `GetBearerToken[Async]` methods (due to additional parameters). This also slightly tweaks the bodies of requests, putting each form parameter on a separate line (mainly results in more readable traces). --- MetaBrainz.MusicBrainz.sln.DotSettings | 1 + MetaBrainz.MusicBrainz/OAuth2.cs | 70 +++++++++++++------ .../MetaBrainz.MusicBrainz.net6.0.cs.md | 6 +- .../MetaBrainz.MusicBrainz.net8.0.cs.md | 6 +- 4 files changed, 57 insertions(+), 26 deletions(-) diff --git a/MetaBrainz.MusicBrainz.sln.DotSettings b/MetaBrainz.MusicBrainz.sln.DotSettings index e3be2ca..4add54e 100644 --- a/MetaBrainz.MusicBrainz.sln.DotSettings +++ b/MetaBrainz.MusicBrainz.sln.DotSettings @@ -102,6 +102,7 @@ True True True + True True True True diff --git a/MetaBrainz.MusicBrainz/OAuth2.cs b/MetaBrainz.MusicBrainz/OAuth2.cs index 254aed3..34059ce 100644 --- a/MetaBrainz.MusicBrainz/OAuth2.cs +++ b/MetaBrainz.MusicBrainz/OAuth2.cs @@ -179,15 +179,26 @@ public string UrlScheme { /// The URI that should receive the authorization code; use for out-of-band requests. /// /// The authorization scopes that should be included in the authorization code. - /// An optional string that will be included in the response sent to . + /// + /// Any string the application wants passed back after authorization; it will be included in the response sent to + /// . For example, this can be a CSRF token from your application. This parameter is optional, but + /// strongly recommended. + /// + /// + /// MusicBrainz supports the use of "Proof Key for Code Exchange" (PKCE) by clients. This is strongly recommended to avoid + /// authorization code interception attacks.
+ /// See RFC 7636 for the process of generating a + /// code_verifier and then a code_challenge (passed here) based on that. + /// + /// Either "S256" (recommended) or "plain" (the default if not specified). /// Requests offline use (a refresh token will be provided alongside the access token). /// /// If , the user will be required to confirm authorization even if the requested scopes have already been /// granted. /// /// The generated URI. - public Uri CreateAuthorizationRequest(Uri redirectUri, AuthorizationScope scope, string? state = null, bool offlineAccess = false, - bool forcePrompt = false) { + public Uri CreateAuthorizationRequest(Uri redirectUri, AuthorizationScope scope, string? state = null, string? challenge = null, + string? challengeMethod = null, bool offlineAccess = false, bool forcePrompt = false) { if (scope == AuthorizationScope.None) { throw new ArgumentException("At least one authorization scope must be selected.", nameof(scope)); } @@ -195,7 +206,7 @@ public Uri CreateAuthorizationRequest(Uri redirectUri, AuthorizationScope scope, query.Append("?response_type=code"); query.Append("&client_id=").Append(Uri.EscapeDataString(this.ClientId)); query.Append("&redirect_uri=").Append(Uri.EscapeDataString(redirectUri.ToString())); - query.Append("&scope=").Append(string.Join("+", OAuth2.ScopeStrings(scope))); + query.Append("&scope=").Append(string.Join('+', OAuth2.ScopeStrings(scope))); if (state is not null) { query.Append("&state=").Append(Uri.EscapeDataString(state)); } @@ -205,6 +216,12 @@ public Uri CreateAuthorizationRequest(Uri redirectUri, AuthorizationScope scope, if (forcePrompt) { query.Append("&approval_prompt=force"); } + if (challenge is not null) { + query.Append("&code_challenge=").Append(Uri.EscapeDataString(challenge)); + if (challengeMethod is not null) { + query.Append("&code_challenge_method=").Append(Uri.EscapeDataString(challengeMethod)); + } + } return new UriBuilder(this.UrlScheme, this.Server, this.Port, OAuth2.AuthorizationEndPoint, query.ToString()).Uri; } @@ -215,9 +232,14 @@ public Uri CreateAuthorizationRequest(Uri redirectUri, AuthorizationScope scope, /// The URI to redirect to (or for out-of-band requests); must match the request URI used to obtain /// . /// + /// + /// If you're using PKCE, pass the code_verifier here. The request will be rejected if it doesn't agree with the challenge + /// and challenge method used for the authorization request as generated via . The process + /// is described in detail by RFC 7636. + /// /// The obtained bearer token. - public IAuthorizationToken GetBearerToken(string code, string clientSecret, Uri redirectUri) - => AsyncUtils.ResultOf(this.GetBearerTokenAsync(code, clientSecret, redirectUri)); + public IAuthorizationToken GetBearerToken(string code, string clientSecret, Uri redirectUri, string? verifier = null) + => AsyncUtils.ResultOf(this.GetBearerTokenAsync(code, clientSecret, redirectUri, verifier)); /// Exchanges an authorization code for a bearer token. /// The authorization code to be used. If the request succeeds, this code will be invalidated. @@ -226,11 +248,16 @@ public IAuthorizationToken GetBearerToken(string code, string clientSecret, Uri /// The URI to redirect to (or for out-of-band requests); must match the request URI used to obtain /// . /// + /// + /// If you're using PKCE, pass the code_verifier here. The request will be rejected if it doesn't agree with the challenge + /// and challenge method used for the authorization request as generated via . The process + /// is described in detail by RFC 7636. + /// /// The cancellation token to cancel the operation. /// The obtained bearer token. - public Task GetBearerTokenAsync(string code, string clientSecret, Uri redirectUri, + public Task GetBearerTokenAsync(string code, string clientSecret, Uri redirectUri, string? verifier = null, CancellationToken cancellationToken = default) - => this.RequestTokenAsync("bearer", code, clientSecret, redirectUri, cancellationToken); + => this.RequestTokenAsync("bearer", code, clientSecret, redirectUri, verifier, cancellationToken); /// Gets information about the user associated with an access token. /// The access token. @@ -456,8 +483,8 @@ private async Task PostAsync(string endPoint, HttpContent? content, Cancel private async Task PostRevocationRequestAsync(string token, string clientSecret, CancellationToken cancellationToken) { var body = new StringBuilder(); body.Append("token=").Append(Uri.EscapeDataString(token)); - body.Append("&client_id=").Append(Uri.EscapeDataString(this.ClientId)); - body.Append("&client_secret=").Append(Uri.EscapeDataString(clientSecret)); + body.Append("&\nclient_id=").Append(Uri.EscapeDataString(this.ClientId)); + body.Append("&\nclient_secret=").Append(Uri.EscapeDataString(clientSecret)); var content = new StringContent(body.ToString(), Encoding.UTF8, OAuth2.TokenRequestBodyType); await this.PerformRequestAsync(HttpMethod.Post, OAuth2.RevokeEndPoint, content, null, cancellationToken).ConfigureAwait(false); } @@ -475,22 +502,25 @@ private Task RefreshTokenAsync(string type, string codeOrTo CancellationToken cancellationToken) { var body = new StringBuilder(); body.Append("client_id=").Append(Uri.EscapeDataString(this.ClientId)); - body.Append("&client_secret=").Append(Uri.EscapeDataString(clientSecret)); - body.Append("&token_type=").Append(Uri.EscapeDataString(type)); - body.Append("&grant_type=refresh_token"); - body.Append("&refresh_token=").Append(Uri.EscapeDataString(codeOrToken)); + body.Append("&\nclient_secret=").Append(Uri.EscapeDataString(clientSecret)); + body.Append("&\ntoken_type=").Append(Uri.EscapeDataString(type)); + body.Append("&\ngrant_type=refresh_token"); + body.Append("&\nrefresh_token=").Append(Uri.EscapeDataString(codeOrToken)); return this.PostTokenRequestAsync(type, body.ToString(), cancellationToken); } private Task RequestTokenAsync(string type, string codeOrToken, string clientSecret, Uri redirectUri, - CancellationToken cancellationToken) { + string? verifier, CancellationToken cancellationToken) { var body = new StringBuilder(); body.Append("client_id=").Append(Uri.EscapeDataString(this.ClientId)); - body.Append("&client_secret=").Append(Uri.EscapeDataString(clientSecret)); - body.Append("&token_type=").Append(Uri.EscapeDataString(type)); - body.Append("&grant_type=authorization_code"); - body.Append("&code=").Append(Uri.EscapeDataString(codeOrToken)); - body.Append("&redirect_uri=").Append(Uri.EscapeDataString(redirectUri.ToString())); + body.Append("&\nclient_secret=").Append(Uri.EscapeDataString(clientSecret)); + body.Append("&\ntoken_type=").Append(Uri.EscapeDataString(type)); + body.Append("&\ngrant_type=authorization_code"); + body.Append("&\ncode=").Append(Uri.EscapeDataString(codeOrToken)); + body.Append("&\nredirect_uri=").Append(Uri.EscapeDataString(redirectUri.ToString())); + if (verifier is not null) { + body.Append("&\ncode_verifier=").Append(Uri.EscapeDataString(verifier)); + } return this.PostTokenRequestAsync(type, body.ToString(), cancellationToken); } diff --git a/public-api/MetaBrainz.MusicBrainz.net6.0.cs.md b/public-api/MetaBrainz.MusicBrainz.net6.0.cs.md index 1936f5b..319b756 100644 --- a/public-api/MetaBrainz.MusicBrainz.net6.0.cs.md +++ b/public-api/MetaBrainz.MusicBrainz.net6.0.cs.md @@ -167,15 +167,15 @@ public sealed class OAuth2 : System.IDisposable { public void ConfigureClientCreation(System.Func? code); - public System.Uri CreateAuthorizationRequest(System.Uri redirectUri, AuthorizationScope scope, string? state = null, bool offlineAccess = false, bool forcePrompt = false); + public System.Uri CreateAuthorizationRequest(System.Uri redirectUri, AuthorizationScope scope, string? state = null, string? challenge = null, string? challengeMethod = null, bool offlineAccess = false, bool forcePrompt = false); public sealed override void Dispose(); protected override void Finalize(); - public MetaBrainz.MusicBrainz.Interfaces.IAuthorizationToken GetBearerToken(string code, string clientSecret, System.Uri redirectUri); + public MetaBrainz.MusicBrainz.Interfaces.IAuthorizationToken GetBearerToken(string code, string clientSecret, System.Uri redirectUri, string? verifier = null); - public System.Threading.Tasks.Task GetBearerTokenAsync(string code, string clientSecret, System.Uri redirectUri, System.Threading.CancellationToken cancellationToken = default); + public System.Threading.Tasks.Task GetBearerTokenAsync(string code, string clientSecret, System.Uri redirectUri, string? verifier = null, System.Threading.CancellationToken cancellationToken = default); public MetaBrainz.MusicBrainz.Interfaces.IUserInfo GetUserInfo(string token); diff --git a/public-api/MetaBrainz.MusicBrainz.net8.0.cs.md b/public-api/MetaBrainz.MusicBrainz.net8.0.cs.md index a1b1e46..93e3926 100644 --- a/public-api/MetaBrainz.MusicBrainz.net8.0.cs.md +++ b/public-api/MetaBrainz.MusicBrainz.net8.0.cs.md @@ -167,15 +167,15 @@ public sealed class OAuth2 : System.IDisposable { public void ConfigureClientCreation(System.Func? code); - public System.Uri CreateAuthorizationRequest(System.Uri redirectUri, AuthorizationScope scope, string? state = null, bool offlineAccess = false, bool forcePrompt = false); + public System.Uri CreateAuthorizationRequest(System.Uri redirectUri, AuthorizationScope scope, string? state = null, string? challenge = null, string? challengeMethod = null, bool offlineAccess = false, bool forcePrompt = false); public sealed override void Dispose(); protected override void Finalize(); - public MetaBrainz.MusicBrainz.Interfaces.IAuthorizationToken GetBearerToken(string code, string clientSecret, System.Uri redirectUri); + public MetaBrainz.MusicBrainz.Interfaces.IAuthorizationToken GetBearerToken(string code, string clientSecret, System.Uri redirectUri, string? verifier = null); - public System.Threading.Tasks.Task GetBearerTokenAsync(string code, string clientSecret, System.Uri redirectUri, System.Threading.CancellationToken cancellationToken = default); + public System.Threading.Tasks.Task GetBearerTokenAsync(string code, string clientSecret, System.Uri redirectUri, string? verifier = null, System.Threading.CancellationToken cancellationToken = default); public MetaBrainz.MusicBrainz.Interfaces.IUserInfo GetUserInfo(string token); From 1d5451951edf15686360e37fa6286376bbc34d61 Mon Sep 17 00:00:00 2001 From: Tim Van Holder Date: Sat, 30 Dec 2023 14:55:04 +0100 Subject: [PATCH 2/2] Update user info retrieval The email address in the user info is now considered optional (because it is only returned when the `email` scope has been granted). The documentation for `GetUserInfo[Async]` now mentions this, as well as the requirement for the `profile` scope to have been granted. This also renames `AuthorizationScope.EMail` to `AuthorizationScope.Email`. --- MetaBrainz.MusicBrainz/AuthorizationScope.cs | 4 ++-- MetaBrainz.MusicBrainz/Interfaces/IUserInfo.cs | 8 +++++--- MetaBrainz.MusicBrainz/Json/Readers/UserInfoReader.cs | 6 ++---- MetaBrainz.MusicBrainz/OAuth2.cs | 10 +++++++++- MetaBrainz.MusicBrainz/Objects/UserInfo.cs | 5 ++--- public-api/MetaBrainz.MusicBrainz.net6.0.cs.md | 5 +++-- public-api/MetaBrainz.MusicBrainz.net8.0.cs.md | 5 +++-- 7 files changed, 26 insertions(+), 17 deletions(-) diff --git a/MetaBrainz.MusicBrainz/AuthorizationScope.cs b/MetaBrainz.MusicBrainz/AuthorizationScope.cs index bc175bb..026f4f0 100644 --- a/MetaBrainz.MusicBrainz/AuthorizationScope.cs +++ b/MetaBrainz.MusicBrainz/AuthorizationScope.cs @@ -15,11 +15,11 @@ public enum AuthorizationScope { /// Request all available permissions (not recommended). Everything = -1, - /// View the user's public profile information (username, age, country, homepage). + /// View the user's public profile information (e.g. username and time zone). Profile = 1 << 0, /// View the user's email address. - EMail = 1 << 1, + Email = 1 << 1, /// View and modify the user's private tags. Tag = 1 << 2, diff --git a/MetaBrainz.MusicBrainz/Interfaces/IUserInfo.cs b/MetaBrainz.MusicBrainz/Interfaces/IUserInfo.cs index 2d444c6..d8179a0 100644 --- a/MetaBrainz.MusicBrainz/Interfaces/IUserInfo.cs +++ b/MetaBrainz.MusicBrainz/Interfaces/IUserInfo.cs @@ -4,11 +4,13 @@ namespace MetaBrainz.MusicBrainz.Interfaces; -/// Information about a user as returned by OAuth2. +/// +/// Information about a user as returned by OAuth2 (if has been granted). +/// public interface IUserInfo : IJsonBasedObject { - /// The user's email address. - string Email { get; } + /// The user's email address. Will only be provided if has been granted. + string? Email { get; init; } /// The user's gender. string? Gender { get; } diff --git a/MetaBrainz.MusicBrainz/Json/Readers/UserInfoReader.cs b/MetaBrainz.MusicBrainz/Json/Readers/UserInfoReader.cs index 0db602e..dc64eef 100644 --- a/MetaBrainz.MusicBrainz/Json/Readers/UserInfoReader.cs +++ b/MetaBrainz.MusicBrainz/Json/Readers/UserInfoReader.cs @@ -68,13 +68,11 @@ protected override UserInfo ReadObjectContents(ref Utf8JsonReader reader, JsonSe if (name is null) { throw new JsonException("Expected user name not found or null."); } - if (email is null) { - throw new JsonException("Expected email address not found or null."); - } if (profile is null) { throw new JsonException("Expected profile URI not found or null."); } - return new UserInfo(id.Value, name, email, profile) { + return new UserInfo(id.Value, name, profile) { + Email = email, Gender = gender, TimeZone = timeZone, UnhandledProperties = rest, diff --git a/MetaBrainz.MusicBrainz/OAuth2.cs b/MetaBrainz.MusicBrainz/OAuth2.cs index 34059ce..ab2b7c6 100644 --- a/MetaBrainz.MusicBrainz/OAuth2.cs +++ b/MetaBrainz.MusicBrainz/OAuth2.cs @@ -262,12 +262,20 @@ public Task GetBearerTokenAsync(string code, string clientS /// Gets information about the user associated with an access token. /// The access token. /// Information about the user associated with the access token. + /// + /// If the permission has not been granted, this request will fail. In addition, it will + /// only include the user's email address if the permission has been granted. + /// public IUserInfo GetUserInfo(string token) => AsyncUtils.ResultOf(this.GetUserInfoAsync(token)); /// Gets information about the user associated with an access token. /// The access token. /// The cancellation token to cancel the operation. /// Information about the user associated with the access token. + /// + /// If the permission has not been granted, this request will fail. In addition, it will + /// only include the user's email address if the permission has been granted. + /// public Task GetUserInfoAsync(string token, CancellationToken cancellationToken = default) => this.GetAsync(OAuth2.UserInfoEndPoint, token, cancellationToken); @@ -528,7 +536,7 @@ private static IEnumerable ScopeStrings(AuthorizationScope scope) { if ((scope & AuthorizationScope.Collection) != 0) { yield return "collection"; } - if ((scope & AuthorizationScope.EMail) != 0) { + if ((scope & AuthorizationScope.Email) != 0) { yield return "email"; } if ((scope & AuthorizationScope.Profile) != 0) { diff --git a/MetaBrainz.MusicBrainz/Objects/UserInfo.cs b/MetaBrainz.MusicBrainz/Objects/UserInfo.cs index 42d92ad..f0d883b 100644 --- a/MetaBrainz.MusicBrainz/Objects/UserInfo.cs +++ b/MetaBrainz.MusicBrainz/Objects/UserInfo.cs @@ -7,14 +7,13 @@ namespace MetaBrainz.MusicBrainz.Objects; internal sealed class UserInfo : JsonBasedObject, IUserInfo { - public UserInfo(int userId, string name, string email, Uri profile) { - this.Email = email; + public UserInfo(int userId, string name, Uri profile) { this.Name = name; this.Profile = profile; this.UserId = userId; } - public string Email { get; } + public string? Email { get; init; } public string? Gender { get; init; } diff --git a/public-api/MetaBrainz.MusicBrainz.net6.0.cs.md b/public-api/MetaBrainz.MusicBrainz.net6.0.cs.md index 319b756..0147e67 100644 --- a/public-api/MetaBrainz.MusicBrainz.net6.0.cs.md +++ b/public-api/MetaBrainz.MusicBrainz.net6.0.cs.md @@ -16,7 +16,7 @@ public enum AuthorizationScope { Collection = 16, - EMail = 2, + Email = 2, Everything = -1, None = 0, Profile = 1, @@ -1892,8 +1892,9 @@ public interface IStreamingQueryResults : System.Collections.Generic. ```cs public interface IUserInfo : MetaBrainz.Common.Json.IJsonBasedObject { - string Email { + string? Email { public abstract get; + public abstract init; } string? Gender { diff --git a/public-api/MetaBrainz.MusicBrainz.net8.0.cs.md b/public-api/MetaBrainz.MusicBrainz.net8.0.cs.md index 93e3926..d0fa240 100644 --- a/public-api/MetaBrainz.MusicBrainz.net8.0.cs.md +++ b/public-api/MetaBrainz.MusicBrainz.net8.0.cs.md @@ -16,7 +16,7 @@ public enum AuthorizationScope { Collection = 16, - EMail = 2, + Email = 2, Everything = -1, None = 0, Profile = 1, @@ -1892,8 +1892,9 @@ public interface IStreamingQueryResults : System.Collections.Generic. ```cs public interface IUserInfo : MetaBrainz.Common.Json.IJsonBasedObject { - string Email { + string? Email { public abstract get; + public abstract init; } string? Gender {