From 3147ffd2bfe7cd7bde8a23c673b0e4912b482d63 Mon Sep 17 00:00:00 2001 From: Jordan Brown Date: Mon, 7 Dec 2020 14:06:43 -0500 Subject: [PATCH] Oauth cleanups --- README.md | 13 ++++++--- docs/API.dox | 8 +++--- .../Models/OAuthProvider.cs | 5 ++++ .../Models/OAuthProviderInfo.cs | 20 ++++++++++++++ .../Models/ServerInformation.cs | 4 +-- .../Configuration/OAuthConfiguration.cs | 11 ++++++-- .../Controllers/HomeController.cs | 2 +- .../Security/OAuth/BaseOAuthValidator.cs | 2 +- .../Security/OAuth/DiscordOAuthValidator.cs | 2 +- .../Security/OAuth/DiscordTokenRequest.cs | 27 ------------------- .../Security/OAuth/GenericOAuthValidator.cs | 8 +++++- .../Security/OAuth/GitHubOAuthValidator.cs | 12 +++++++-- .../Security/OAuth/IOAuthProviders.cs | 4 +-- .../Security/OAuth/IOAuthValidator.cs | 4 +-- .../Security/OAuth/OAuthProviders.cs | 6 ++--- .../Security/OAuth/OAuthTokenRequest.cs | 17 ++++++++++-- .../Security/OAuth/TGForumsOAuthValidator.cs | 15 ++++++++--- 17 files changed, 102 insertions(+), 58 deletions(-) create mode 100644 src/Tgstation.Server.Api/Models/OAuthProviderInfo.cs delete mode 100644 src/Tgstation.Server.Host/Security/OAuth/DiscordTokenRequest.cs diff --git a/README.md b/README.md index c2cb3d211d..26a84e81dd 100644 --- a/README.md +++ b/README.md @@ -133,15 +133,20 @@ Create an `appsettings.Production.json` file next to `appsettings.json`. This wi - `Security:OAuth`: Sets the OAuth client ID and secret for a given ``. The currently supported providers are `GitHub`, `Discord`, and `TGForums`. Setting these fields to `null` disables logins with the provider, but does not stop users from associating their accounts using the API. Sample Entry: ```json "GitHubOAuth":{ - "Url": "...", (Used with certain providers) "ClientId": "...", - "ClientSecret": "..." + "ClientSecret": "...", + "RedirectUrl": "...", (Used with certain providers) + "ServerUrl": "...", (Used with certain providers) } ``` +The following providers use the `RedirectUrl` setting: -The following providers use the `Url` setting: +- GitHub +- TGForums -- `TGForums`: Used as the OAuth redirect url. +The following providers use the `ServerUrl` setting: + +- None so far ### Database Configuration diff --git a/docs/API.dox b/docs/API.dox index c4cec21c15..5507fa7f83 100644 --- a/docs/API.dox +++ b/docs/API.dox @@ -63,7 +63,7 @@ TGS will only every return the response codes listed here - 204: No Content. Identical to 200 with no response body. - 400: Bad Request. The response body will contain an @ref Tgstation.Server.Api.Models.ErrorMessage model detailing the error - 401: Unauthorized. Invalid or expired credentials were provided. Check rights APIs for updates. See @ref api_auth for details -- 403: Forbidden. User tried to make a request they were not allowed to perform. +- 403: Forbidden. User tried to make a request they were not allowed to perform. - 404: Not found. A resource was requested that had never existed. In the case of retrieving a resource by ID, it could potentially exist in the future - 406: Not Acceptable. Consequence of failing to provide an Accept header - 408: Request Timeout. The client took to long to continue a request @@ -97,7 +97,7 @@ Other fields may be present in the Version model but should be ignored. See a de @section api_auth Authentication -Every request made to TGS requires authentication. It is provided in the form of the Authorization header. +Every request made to TGS requires authentication. It is provided in the form of the Authorization header. The first request made to TGS must be to login the user @@ -128,7 +128,7 @@ TGS4 supports OAuth 2.0 with select providers for authentication. The flow for this is as follows: -- Retrieve the @ref api_ver to find out available OAuth providers and their respective client IDs. +- Retrieve the @ref api_ver to find out available OAuth providers and their respective client ID and redirect URIs. - Send the user to the Authorization Request endpoint for the provider using the client ID from above. See https://tools.ietf.org/html/rfc6749#section-4.1.1. DO NOT specify a redirect URI, this should be configured in the provider. - Retrieve the authorization response code after successfully completing the authorize step above. - Perform the following request: @@ -391,7 +391,7 @@ If the server detects a set of @ref Tgstation.Server.Api.Models.TestMergeParamet @subsubsection api_repounsetauth Unsetting Authentication -The repository uses the @ref Tgstation.Server.Api.Models.Repository.AccessUser and @ref Tgstation.Server.Api.Models.Repository.AccessToken credentials to access the remote repository if these fields are set. To unset them you must set both of them to an empty string like so +The repository uses the @ref Tgstation.Server.Api.Models.Repository.AccessUser and @ref Tgstation.Server.Api.Models.Repository.AccessToken credentials to access the remote repository if these fields are set. To unset them you must set both of them to an empty string like so @code{.json} { diff --git a/src/Tgstation.Server.Api/Models/OAuthProvider.cs b/src/Tgstation.Server.Api/Models/OAuthProvider.cs index 8d5a9eb354..0031ed0f79 100644 --- a/src/Tgstation.Server.Api/Models/OAuthProvider.cs +++ b/src/Tgstation.Server.Api/Models/OAuthProvider.cs @@ -19,5 +19,10 @@ namespace Tgstation.Server.Api.Models /// https://tgstation13.org /// TGForums, + + /// + /// https://www.keycloak.org + /// + Keycloak, } } diff --git a/src/Tgstation.Server.Api/Models/OAuthProviderInfo.cs b/src/Tgstation.Server.Api/Models/OAuthProviderInfo.cs new file mode 100644 index 0000000000..10466c3204 --- /dev/null +++ b/src/Tgstation.Server.Api/Models/OAuthProviderInfo.cs @@ -0,0 +1,20 @@ +using System; + +namespace Tgstation.Server.Api.Models +{ + /// + /// Public information about a given . + /// + public sealed class OAuthProviderInfo + { + /// + /// The client ID. + /// + public string? ClientId { get; set; } + + /// + /// The redirect URL. + /// + public Uri? RedirectUri { get; set; } + } +} diff --git a/src/Tgstation.Server.Api/Models/ServerInformation.cs b/src/Tgstation.Server.Api/Models/ServerInformation.cs index a2d8958845..2111302237 100644 --- a/src/Tgstation.Server.Api/Models/ServerInformation.cs +++ b/src/Tgstation.Server.Api/Models/ServerInformation.cs @@ -24,8 +24,8 @@ namespace Tgstation.Server.Api.Models public Version? DMApiVersion { get; set; } /// - /// Map of to the server's associated client IDs for them. + /// Map of to the for them. /// - public IDictionary? OAuthProviderClientIds { get; set; } + public IDictionary? OAuthProviderInfos { get; set; } } } diff --git a/src/Tgstation.Server.Host/Configuration/OAuthConfiguration.cs b/src/Tgstation.Server.Host/Configuration/OAuthConfiguration.cs index 5d865c76ed..b12e239911 100644 --- a/src/Tgstation.Server.Host/Configuration/OAuthConfiguration.cs +++ b/src/Tgstation.Server.Host/Configuration/OAuthConfiguration.cs @@ -1,3 +1,5 @@ +using System; + namespace Tgstation.Server.Host.Configuration { /// @@ -6,8 +8,13 @@ namespace Tgstation.Server.Host.Configuration sealed class OAuthConfiguration : OAuthConfigurationBase { /// - /// The redirect or server URL. Not used by all providers. + /// The client redirect URL. Not used by all providers. /// - public string Url { get; set; } + public Uri ServerUrl { get; set; } + + /// + /// The authentication server URL. Not used by all providers. + /// + public Uri RedirectUrl { get; set; } } } diff --git a/src/Tgstation.Server.Host/Controllers/HomeController.cs b/src/Tgstation.Server.Host/Controllers/HomeController.cs index 9d09033105..fd4dd4d7f8 100644 --- a/src/Tgstation.Server.Host/Controllers/HomeController.cs +++ b/src/Tgstation.Server.Host/Controllers/HomeController.cs @@ -167,7 +167,7 @@ namespace Tgstation.Server.Host.Controllers InstanceLimit = generalConfiguration.InstanceLimit, UserLimit = generalConfiguration.UserLimit, ValidInstancePaths = generalConfiguration.ValidInstancePaths, - OAuthProviderClientIds = await oAuthProviders.ClientIds(cancellationToken).ConfigureAwait(false) + OAuthProviderInfos = await oAuthProviders.ProviderInfos(cancellationToken).ConfigureAwait(false) }); } diff --git a/src/Tgstation.Server.Host/Security/OAuth/BaseOAuthValidator.cs b/src/Tgstation.Server.Host/Security/OAuth/BaseOAuthValidator.cs index 68cc17cf23..4cb1995f59 100644 --- a/src/Tgstation.Server.Host/Security/OAuth/BaseOAuthValidator.cs +++ b/src/Tgstation.Server.Host/Security/OAuth/BaseOAuthValidator.cs @@ -73,7 +73,7 @@ namespace Tgstation.Server.Host.Security.OAuth } /// - public abstract Task GetClientId(CancellationToken cancellationToken); + public abstract Task GetProviderInfo(CancellationToken cancellationToken); /// public abstract Task ValidateResponseCode(string code, CancellationToken cancellationToken); diff --git a/src/Tgstation.Server.Host/Security/OAuth/DiscordOAuthValidator.cs b/src/Tgstation.Server.Host/Security/OAuth/DiscordOAuthValidator.cs index f0fe8591f5..37d055d08b 100644 --- a/src/Tgstation.Server.Host/Security/OAuth/DiscordOAuthValidator.cs +++ b/src/Tgstation.Server.Host/Security/OAuth/DiscordOAuthValidator.cs @@ -38,7 +38,7 @@ namespace Tgstation.Server.Host.Security.OAuth protected override Uri UserInformationUrl => new Uri("https://discord.com/api/users/@me"); /// - protected override OAuthTokenRequest CreateTokenRequest(string code) => new DiscordTokenRequest(OAuthConfiguration, code); + protected override OAuthTokenRequest CreateTokenRequest(string code) => new OAuthTokenRequest(OAuthConfiguration, code, "identify"); /// protected override string DecodeTokenPayload(dynamic responseJson) => responseJson.access_token; diff --git a/src/Tgstation.Server.Host/Security/OAuth/DiscordTokenRequest.cs b/src/Tgstation.Server.Host/Security/OAuth/DiscordTokenRequest.cs deleted file mode 100644 index cf477ba156..0000000000 --- a/src/Tgstation.Server.Host/Security/OAuth/DiscordTokenRequest.cs +++ /dev/null @@ -1,27 +0,0 @@ -using Tgstation.Server.Host.Configuration; - -namespace Tgstation.Server.Host.Security.OAuth -{ - /// - /// for Discord. - /// - /// See https://discord.com/developers/docs/topics/oauth2 - sealed class DiscordTokenRequest : OAuthTokenRequest - { - /// - /// The 'grant_type' field. - /// - public string GrantType { get; } - - /// - /// Initializes a new instance of the . - /// - /// The for the . - /// The OAuth code for the . - public DiscordTokenRequest(OAuthConfigurationBase oAuthConfiguration, string code) - : base(oAuthConfiguration, code, "identify") - { - GrantType = "authorization_code"; - } - } -} diff --git a/src/Tgstation.Server.Host/Security/OAuth/GenericOAuthValidator.cs b/src/Tgstation.Server.Host/Security/OAuth/GenericOAuthValidator.cs index 8deff22537..98294d46d6 100644 --- a/src/Tgstation.Server.Host/Security/OAuth/GenericOAuthValidator.cs +++ b/src/Tgstation.Server.Host/Security/OAuth/GenericOAuthValidator.cs @@ -8,6 +8,7 @@ using System.Net.Http.Headers; using System.Threading; using System.Threading.Tasks; using Tgstation.Server.Api; +using Tgstation.Server.Api.Models; using Tgstation.Server.Host.Configuration; using Tgstation.Server.Host.System; @@ -119,6 +120,11 @@ namespace Tgstation.Server.Host.Security.OAuth } /// - public override Task GetClientId(CancellationToken cancellationToken) => Task.FromResult(OAuthConfiguration.ClientId); + public override Task GetProviderInfo(CancellationToken cancellationToken) => Task.FromResult( + new OAuthProviderInfo + { + ClientId = OAuthConfiguration.ClientId, + RedirectUri = OAuthConfiguration.RedirectUrl + }); } } diff --git a/src/Tgstation.Server.Host/Security/OAuth/GitHubOAuthValidator.cs b/src/Tgstation.Server.Host/Security/OAuth/GitHubOAuthValidator.cs index b4ee1354e6..c678568ebe 100644 --- a/src/Tgstation.Server.Host/Security/OAuth/GitHubOAuthValidator.cs +++ b/src/Tgstation.Server.Host/Security/OAuth/GitHubOAuthValidator.cs @@ -65,7 +65,10 @@ namespace Tgstation.Server.Host.Security.OAuth new OauthTokenRequest( oAuthConfiguration.ClientId, oAuthConfiguration.ClientSecret, - code)) + code) + { + RedirectUri = oAuthConfiguration.RedirectUrl + }) .ConfigureAwait(false); var token = response.AccessToken; @@ -94,6 +97,11 @@ namespace Tgstation.Server.Host.Security.OAuth } /// - public Task GetClientId(CancellationToken cancellationToken) => Task.FromResult(oAuthConfiguration.ClientId); + public Task GetProviderInfo(CancellationToken cancellationToken) => Task.FromResult( + new OAuthProviderInfo + { + ClientId = oAuthConfiguration.ClientId, + RedirectUri = oAuthConfiguration.RedirectUrl + }); } } diff --git a/src/Tgstation.Server.Host/Security/OAuth/IOAuthProviders.cs b/src/Tgstation.Server.Host/Security/OAuth/IOAuthProviders.cs index 66af75b5af..06cae4e96e 100644 --- a/src/Tgstation.Server.Host/Security/OAuth/IOAuthProviders.cs +++ b/src/Tgstation.Server.Host/Security/OAuth/IOAuthProviders.cs @@ -21,7 +21,7 @@ namespace Tgstation.Server.Host.Security.OAuth /// Gets a of the provider client IDs. /// /// The for the operation. - /// A resulting in a anew of the active provider client IDs. - Task> ClientIds(CancellationToken cancellationToken); + /// A resulting in a anew of the active s. + Task> ProviderInfos(CancellationToken cancellationToken); } } diff --git a/src/Tgstation.Server.Host/Security/OAuth/IOAuthValidator.cs b/src/Tgstation.Server.Host/Security/OAuth/IOAuthValidator.cs index be33dcb992..be8df49788 100644 --- a/src/Tgstation.Server.Host/Security/OAuth/IOAuthValidator.cs +++ b/src/Tgstation.Server.Host/Security/OAuth/IOAuthValidator.cs @@ -15,11 +15,11 @@ namespace Tgstation.Server.Host.Security.OAuth OAuthProvider Provider { get; } /// - /// Gets the OAuth client ID of validator. + /// Gets the of validator. /// /// The for the operation. /// A resulting in the client ID of the validator on success, on failure. - Task GetClientId(CancellationToken cancellationToken); + Task GetProviderInfo(CancellationToken cancellationToken); /// /// Validate a given OAuth response . diff --git a/src/Tgstation.Server.Host/Security/OAuth/OAuthProviders.cs b/src/Tgstation.Server.Host/Security/OAuth/OAuthProviders.cs index 91be97f0e2..9087aad25b 100644 --- a/src/Tgstation.Server.Host/Security/OAuth/OAuthProviders.cs +++ b/src/Tgstation.Server.Host/Security/OAuth/OAuthProviders.cs @@ -58,7 +58,7 @@ namespace Tgstation.Server.Host.Security.OAuth loggerFactory.CreateLogger(), discordConfig)); - if(securityConfiguration.OAuth.TryGetValue(OAuthProvider.TGForums, out var tgConfig)) + if (securityConfiguration.OAuth.TryGetValue(OAuthProvider.TGForums, out var tgConfig)) validatorsBuilder.Add( new TGForumsOAuthValidator( httpClientFactory, @@ -73,11 +73,11 @@ namespace Tgstation.Server.Host.Security.OAuth public IOAuthValidator GetValidator(OAuthProvider oAuthProvider) => validators.FirstOrDefault(x => x.Provider == oAuthProvider); /// - public async Task> ClientIds(CancellationToken cancellationToken) + public async Task> ProviderInfos(CancellationToken cancellationToken) { var providersAndTasks = validators.ToDictionary( x => x.Provider, - x => x.GetClientId(cancellationToken)); + x => x.GetProviderInfo(cancellationToken)); await Task.WhenAll(providersAndTasks.Values).ConfigureAwait(false); diff --git a/src/Tgstation.Server.Host/Security/OAuth/OAuthTokenRequest.cs b/src/Tgstation.Server.Host/Security/OAuth/OAuthTokenRequest.cs index cad2885746..eb715953c6 100644 --- a/src/Tgstation.Server.Host/Security/OAuth/OAuthTokenRequest.cs +++ b/src/Tgstation.Server.Host/Security/OAuth/OAuthTokenRequest.cs @@ -6,7 +6,7 @@ namespace Tgstation.Server.Host.Security.OAuth /// /// Generic OAuth token request. /// - class OAuthTokenRequest : OAuthConfigurationBase + sealed class OAuthTokenRequest : OAuthConfigurationBase { /// /// The OAuth code received from the browser. @@ -18,17 +18,30 @@ namespace Tgstation.Server.Host.Security.OAuth /// public string Scope { get; } + /// + /// The OAuth redirect URI. + /// + public Uri RedirectUri { get; } + + /// + /// The OAuth grant type. + /// + public string GrantType { get; } + /// /// Initializes a new instance of the . /// /// The to build from. /// The value of . /// The value of - public OAuthTokenRequest(OAuthConfigurationBase oAuthConfiguration, string code, string scope) + public OAuthTokenRequest(OAuthConfiguration oAuthConfiguration, string code, string scope) : base(oAuthConfiguration) { Code = code ?? throw new ArgumentNullException(nameof(code)); Scope = scope ?? throw new ArgumentNullException(nameof(scope)); + + RedirectUri = oAuthConfiguration.RedirectUrl; + GrantType = "authorization_code"; } } } diff --git a/src/Tgstation.Server.Host/Security/OAuth/TGForumsOAuthValidator.cs b/src/Tgstation.Server.Host/Security/OAuth/TGForumsOAuthValidator.cs index 775551af97..4e6028987e 100644 --- a/src/Tgstation.Server.Host/Security/OAuth/TGForumsOAuthValidator.cs +++ b/src/Tgstation.Server.Host/Security/OAuth/TGForumsOAuthValidator.cs @@ -54,7 +54,7 @@ namespace Tgstation.Server.Host.Security.OAuth } /// - public override async Task GetClientId(CancellationToken cancellationToken) + public override async Task GetProviderInfo(CancellationToken cancellationToken) { var expiredSessions = sessions.RemoveAll(x => x.Item2.AddMinutes(SessionRetentionMinutes) < DateTimeOffset.Now); if (expiredSessions > 0) @@ -65,7 +65,7 @@ namespace Tgstation.Server.Host.Security.OAuth { UriBuilder builder = new UriBuilder("https://tgstation13.org/phpBB/oauth_create_session.php") { - Query = $"site_private_token={HttpUtility.UrlEncode(Convert.ToBase64String(Encoding.UTF8.GetBytes(OAuthConfiguration.ClientSecret)))}&return_uri={HttpUtility.UrlEncode(OAuthConfiguration.Url)}" + Query = $"site_private_token={HttpUtility.UrlEncode(Convert.ToBase64String(Encoding.UTF8.GetBytes(OAuthConfiguration.ClientSecret)))}&return_uri={HttpUtility.UrlEncode(OAuthConfiguration.RedirectUrl.ToString())}" }; using var request = new HttpRequestMessage(HttpMethod.Get, builder.Uri); @@ -83,8 +83,15 @@ namespace Tgstation.Server.Host.Security.OAuth return null; } - sessions.Add(Tuple.Create(newSession, DateTimeOffset.Now)); - return newSession.SessionPublicToken; + sessions.Add( + Tuple.Create( + newSession, + DateTimeOffset.Now)); + return new OAuthProviderInfo + { + ClientId = newSession.SessionPublicToken, + RedirectUri = OAuthConfiguration.RedirectUrl + }; } catch (Exception ex) {