diff --git a/docs/oidc-resource-server.md b/docs/oidc-resource-server.md index 8397cff6b8..4a1ec18741 100644 --- a/docs/oidc-resource-server.md +++ b/docs/oidc-resource-server.md @@ -147,10 +147,7 @@ customer in this order. 6. One identity per customer: when the customer is already linked to another `sub`, the new subject is refused (403). One identity per customer is enforced by the link's identifier, which is derived from the customer, so two concurrent requests with different subjects cannot both link the same customer. - Links do not record the provider instance, and a `sub` is unique only within one provider. Before switching - `Authority` to another provider (or realm), remove the `ExternalAuth.OpenIdConnect` links: a subject of the new - provider equal to a stored one would select the old customer, and customers linked by the previous provider would be - refused as linked to another identity. + A `sub` is unique only within one provider, so the links belong to the provider they were made with (section 3.6). 7. With `RegisterCustomersPerStore` enabled an account of another store is refused (403) and lookups are store-scoped. **Per-store identity.** With `RegisterCustomersPerStore` enabled the same person can have a separate customer in each @@ -215,6 +212,42 @@ on unless every endpoint the resource covers scopes its data. * The Web API module itself is off by default: set `FeatureManagement:Grand.Module.Api = true` (environment variable `FeatureManagement__Grand.Module.Api`). Without it `/api/...` answers with the storefront's 404 page. +### 3.6 Managing links, and changing the provider + +**One link.** *Customers > Customers >* a customer *> Info* lists the customer's external sign-ins (Admin and Store +panels, within the panel's scope). Deleting the OpenID Connect row removes the link and ends the customer's sessions +started at the provider; the next provider sign-in connects the account again by the rules above. Use it when a person +was deleted and created again at the provider ("already connected to another identity provider account"). The row is +listed even when only the resource server is used, without the sign-in plugin. Removing a link does not bring back +tokens refused by a back-channel logout: revocations are kept apart from the links. + +**Changing `Authority`.** The store records the provider the links were made with (`OidcLinkSettings`; trailing slash +and case do not matter). While no OpenID Connect link exists, the configured `Authority` is adopted at once, so a fresh +installation, or a provider changed before anyone connected, needs nothing. When links exist and `Authority` differs +from the recorded provider, or links exist and no provider was recorded yet (links made before this version: the +provider may have changed with the same upgrade), a subject of the configured provider equal to a stored one could +select another person's account, so until an administrator decides: + +* provider sign-in is refused ("Signing in with the identity provider is paused…"), access tokens get 403, and no link + is made; +* sessions started at the previous provider are not used (the browser signs in again) and are not confirmed at the new + one; a back-channel logout ends no session, it only refuses the subject's older tokens. With `SsoOnly`, accounts + keep their password meanwhile; +* the log gets an error (at most once in five minutes) naming both addresses; +* *Configuration > External authentication* shows the change in the *OpenID Connect account links* card. + +Decide in that card (Admin panel): + +* **Remove all links** when it is another provider or realm: every OpenID Connect link is removed and every session + started at the provider ends. Accounts, orders and permissions stay; people connect again at their next sign-in, by + the rules of a first sign-in (in `ProviderOnly` by the verified e-mail, so the e-mails must match; with + `AutoProvision` an unknown e-mail gets a new account). The same button removes all links at any time. +* **Keep the links** only when the new address belongs to the same provider with the same users (a new host name for + the same Keycloak realm, for example). A user of another provider could otherwise sign in to someone else's account. + +A copy of the production database connected to a test provider stops the same way, instead of matching test users to +production accounts. + ## 4. Errors, metadata, routing ### 4.1 Errors @@ -856,7 +889,8 @@ rules of section 11.2 and the session limits of section 12.2 apply. * **No RP-initiated logout in `Mixed` mode.** Signing out of GrandNode does not sign out of the provider (section 11.6). Only the provider's back-channel logout reaches GrandNode. `ProviderOnly` also ends the provider session (section 13.7). * Ended sessions are deleted by the scheduled task **Delete OpenID Connect sessions** (daily, enabled by default). - Records are kept for 7 days after they end. Most sessions end with the browser, without a sign-out: a session not + Records are kept for 7 days after they end. The same task deletes token revocations (back-channel logout, a refusal + at the provider) older than 7 days, longer than an access token lives. Most sessions end with the browser, without a sign-out: a session not confirmed at the provider for 30 days is deleted with its refresh token. A new provider sign-in of the same account in the same browser ends the session it replaces. * With several instances, one refresh of a session runs at a time on all of them (a lease on the session record), so a @@ -1020,7 +1054,8 @@ step for that: sign in once in the browser first; the access token then uses tha | Failure page about an unverified e-mail | The provider did not report `email_verified` as true; verify the e-mail at the provider (sections 11.4 and 11.5). | | The provider shows an error after sign-out | The store URL plus `/` is not registered as a post-logout redirect URI (section 13.7). | | A user keeps the old name or e-mail | Names are copied at the next provider sign-in; for the e-mail see section 13.5. | -| Failure page "already connected to another identity provider account" (the user was deleted and created again at the provider) | The account is still linked to the old provider identity and the user cannot remove the connection in this mode. The Admin panel only lists the link (customer > *Info*), so an administrator deletes it in the database: the document in the `ExternalAuthentication` collection with the customer's `CustomerId` and `ProviderSystemName` `ExternalAuth.OpenIdConnect`. The user then signs in again and the account is linked to the new identity. | +| Failure page "already connected to another identity provider account" (the user was deleted and created again at the provider) | The account is still linked to the old provider identity and the user cannot remove the connection in this mode. An administrator deletes the OpenID Connect row in *Customers > Customers >* the customer *> Info* (section 3.6). The user then signs in again and the account is linked to the new identity. | +| Failure page "Signing in with the identity provider is paused…", access tokens get 403 | `Authority` changed since the links were made: remove or keep the links in *Configuration > External authentication* (section 3.6). | Sources used for provider specifics (checked 2026-10-08): Keycloak 26.8 MCP guide and release notes, Zitadel v4.19.4 claims, scopes, Actions v2 and dynamic client registration documentation; MCP authorization 2026-07-28; RFC 9728, diff --git a/src/Business/Grand.Business.Authentication/Services/CookieAuthenticationService.cs b/src/Business/Grand.Business.Authentication/Services/CookieAuthenticationService.cs index aaccb7ea18..c9b1754708 100644 --- a/src/Business/Grand.Business.Authentication/Services/CookieAuthenticationService.cs +++ b/src/Business/Grand.Business.Authentication/Services/CookieAuthenticationService.cs @@ -29,6 +29,7 @@ public class CookieAuthenticationService : IGrandAuthenticationService /// SecurityConfig /// Server-side sessions of provider sign-ins /// Accounts that must sign in through an external provider + /// Whether the provider sessions belong to the configured identity provider /// Logger public CookieAuthenticationService( CustomerSettings customerSettings, @@ -39,6 +40,7 @@ public CookieAuthenticationService( SecurityConfig securityConfig, IOidcSessionService oidcSessionService, IExternalSignInPolicy externalSignInPolicy, + IOidcLinkService oidcLinkService, ILogger logger) { _customerSettings = customerSettings; @@ -49,6 +51,7 @@ public CookieAuthenticationService( _securityConfig = securityConfig; _oidcSessionService = oidcSessionService; _externalSignInPolicy = externalSignInPolicy; + _oidcLinkService = oidcLinkService; _logger = logger; } @@ -75,6 +78,7 @@ public CookieAuthenticationService( private readonly SecurityConfig _securityConfig; private readonly IOidcSessionService _oidcSessionService; private readonly IExternalSignInPolicy _externalSignInPolicy; + private readonly IOidcLinkService _oidcLinkService; private readonly ILogger _logger; #endregion @@ -293,7 +297,10 @@ private async Task IsValidCustomer(Customer customer, ClaimsPrincipal prin //a provider sign-in: the server-side session decides (ended, idle, refused by the provider) var oidcSessionId = OidcSessionId(principal); if (!string.IsNullOrEmpty(oidcSessionId)) - return await _oidcSessionService.Validate(oidcSessionId, customer); + //a session of the previous provider is not confirmed at the configured one (its refresh token would go + //there, and a refusal would revoke a subject of another provider): it is not used until the links are reviewed + return await _oidcLinkService.LinksMatchProvider() && + await _oidcSessionService.Validate(oidcSessionId, customer); //a cookie without a session (issued before the account was connected or before the provider became required) //would escape revalidation and back-channel logout: an account that must use the provider signs in again there; diff --git a/src/Business/Grand.Business.Authentication/Services/OidcCustomerResolver.cs b/src/Business/Grand.Business.Authentication/Services/OidcCustomerResolver.cs index 9ae0e05dc7..6d4c3871be 100644 --- a/src/Business/Grand.Business.Authentication/Services/OidcCustomerResolver.cs +++ b/src/Business/Grand.Business.Authentication/Services/OidcCustomerResolver.cs @@ -51,6 +51,7 @@ public class OidcCustomerResolver : IOidcCustomerResolver private readonly OidcLoginConfig _loginConfig; private readonly CustomerConfig _customerConfig; private readonly ILogger _logger; + private readonly IOidcLinkService _linkService; public OidcCustomerResolver( IRepository externalAuthenticationRepository, @@ -63,8 +64,10 @@ public OidcCustomerResolver( OidcResourceServerConfig config, OidcLoginConfig loginConfig, CustomerConfig customerConfig, - ILogger logger) + ILogger logger, + IOidcLinkService linkService) { + _linkService = linkService; _externalAuthenticationRepository = externalAuthenticationRepository; _customerService = customerService; _customerManagerService = customerManagerService; @@ -86,6 +89,10 @@ public virtual async Task Resolve(ClaimsPrincipal principal, if (string.IsNullOrEmpty(sub)) return OidcCustomerResult.InvalidToken("The token has no subject"); + //the links belong to another provider: one of its subjects could equal a stored one + if (!await _linkService.LinksMatchProvider()) + return OidcCustomerResult.Forbidden("The identity provider changed; the administrator must review the account links"); + var link = await GetLink(sub, storeId); if (link != null) { @@ -144,6 +151,9 @@ public virtual async Task ResolveForLogin(string sub, string em if (string.IsNullOrEmpty(sub)) return OidcLoginResult.Refused(OidcLoginRefusal.InvalidIdentity); + if (!await _linkService.LinksMatchProvider()) + return OidcLoginResult.Refused(OidcLoginRefusal.ProviderChanged); + var link = await GetLink(sub, storeId); if (link != null) { @@ -209,8 +219,12 @@ public virtual async Task ResolveForLogin(string sub, string em }; } - public virtual Task LinkIdentity(Customer customer, string sub, string email, string storeId) => - Link(customer, sub, email, storeId, false); + public virtual async Task LinkIdentity(Customer customer, string sub, string email, string storeId) + { + if (!await _linkService.LinksMatchProvider()) + return OidcLinkResult.ProviderChanged; + return await Link(customer, sub, email, storeId, false); + } private async Task Link(Customer customer, string sub, string email, string storeId, bool byEmail) { diff --git a/src/Business/Grand.Business.Authentication/Services/OidcLinkService.cs b/src/Business/Grand.Business.Authentication/Services/OidcLinkService.cs new file mode 100644 index 0000000000..5358b9647f --- /dev/null +++ b/src/Business/Grand.Business.Authentication/Services/OidcLinkService.cs @@ -0,0 +1,131 @@ +using Grand.Business.Core.Interfaces.Authentication; +using Grand.Business.Core.Interfaces.Common.Configuration; +using Grand.Data; +using Grand.Domain.Customers; +using Grand.Infrastructure.Configuration; +using Microsoft.Extensions.Logging; + +namespace Grand.Business.Authentication.Services; + +/// +/// Ties the stored OpenID Connect links to the provider they were made with (OidcLinkSettings, store-independent). A +/// subject is unique only within one provider: after OidcResourceServer:Authority changes, a subject of the new provider +/// equal to a stored one would select another person's account, so no link is used or made until an administrator +/// removes the links (another provider) or keeps them (the same provider at a new address) +/// +public class OidcLinkService( + ISettingService settingService, + OidcResourceServerConfig config, + IRepository externalAuthenticationRepository, + IOidcSessionService oidcSessionService, + ILogger logger) : IOidcLinkService +{ + //every refused request would log it otherwise (access tokens are checked on each request) + private static long _lastMismatchLogTicks; + + //concurrent first requests of one instance record the provider once (each save also clears the cache) + private static readonly SemaphoreSlim AdoptLock = new(1, 1); + + public async Task LinksMatchProvider() + { + var settings = await settingService.LoadSetting(); + if (!string.IsNullOrWhiteSpace(settings?.Authority) && OidcAuthority.SameProvider(settings.Authority, config.Authority)) + return true; + + //no link a subject could select: the configured provider is adopted (first use, or a provider changed before + //anyone connected). Links of an unrecorded provider are not adopted - the provider may have changed together + //with the upgrade that started recording it + if (!await HasLinks()) + { + await Adopt(); + return true; + } + + LogMismatch(settings?.Authority); + return false; + } + + public async Task GetState() + { + var settings = await settingService.LoadSetting(); + var count = await externalAuthenticationRepository.CountAsync(OidcLinks()); + return new OidcLinkState(config.Authority, settings?.Authority, count); + } + + public async Task RemoveAll() + { + var count = await externalAuthenticationRepository.CountAsync(OidcLinks()); + await externalAuthenticationRepository.DeleteManyAsync(x => + x.ProviderSystemName == OidcCustomerResolver.ProviderSystemName); + //their refresh tokens belong to the previous provider + var sessions = await oidcSessionService.EndAllOpen(); + await Adopt(); + logger.LogWarning("All {Count} OpenID Connect links removed and {Sessions} provider sessions ended; links now " + + "belong to {Authority}", count, sessions, config.Authority); + return count; + } + + public async Task KeepAll() + { + await Adopt(); + logger.LogWarning("OpenID Connect links kept for {Authority} (the same provider at a new address)", config.Authority); + } + + public async Task Remove(Customer customer, string linkId) + { + ArgumentNullException.ThrowIfNull(customer); + if (string.IsNullOrEmpty(linkId)) + return false; + + //only a link of this customer + var link = await externalAuthenticationRepository.GetOneAsync(x => x.Id == linkId && x.CustomerId == customer.Id); + if (link == null) + return false; + + await externalAuthenticationRepository.DeleteAsync(link); + if (link.ProviderSystemName == OidcCustomerResolver.ProviderSystemName) + await oidcSessionService.EndByCustomer(customer.Id); + logger.LogInformation("External link {Provider} of customer {CustomerId} removed", link.ProviderSystemName, customer.Id); + return true; + } + + private async Task Adopt() + { + var authority = OidcAuthority.Normalize(config.Authority); + //nothing to record while the resource server has no provider + if (string.IsNullOrEmpty(authority)) + return; + + await AdoptLock.WaitAsync(); + try + { + //another request may have recorded it while this one waited + var settings = await settingService.LoadSetting(); + if (OidcAuthority.SameProvider(settings?.Authority, authority) && !string.IsNullOrEmpty(settings?.Authority)) + return; + await settingService.SaveSetting(new OidcLinkSettings { Authority = authority }); + } + finally + { + AdoptLock.Release(); + } + } + + private IQueryable OidcLinks() => + externalAuthenticationRepository.Table.Where(x => x.ProviderSystemName == OidcCustomerResolver.ProviderSystemName); + + private Task HasLinks() => externalAuthenticationRepository.AnyAsync(OidcLinks()); + + private void LogMismatch(string linkedAuthority) + { + var now = DateTime.UtcNow.Ticks; + var last = Interlocked.Read(ref _lastMismatchLogTicks); + if (now - last < TimeSpan.FromMinutes(5).Ticks || + Interlocked.CompareExchange(ref _lastMismatchLogTicks, now, last) != last) + return; + logger.LogError("The OpenID Connect links belong to {Linked}, not to OidcResourceServer:Authority {Configured}: " + + "OpenID Connect sign-in and access tokens are refused until the links are removed or kept (Admin > " + + "Configuration > External authentication)", + string.IsNullOrEmpty(linkedAuthority) ? "a provider that was not recorded" : linkedAuthority, config.Authority); + } +} diff --git a/src/Business/Grand.Business.Authentication/Services/OidcSessionService.cs b/src/Business/Grand.Business.Authentication/Services/OidcSessionService.cs index 0f27e4c48e..0e9a716344 100644 --- a/src/Business/Grand.Business.Authentication/Services/OidcSessionService.cs +++ b/src/Business/Grand.Business.Authentication/Services/OidcSessionService.cs @@ -268,6 +268,28 @@ public virtual async Task> EndBySubject(string subject) _sessionRepository.Table.Where(x => x.Subject == subject && x.EndedUtc == null))); } + public virtual async Task> EndByCustomer(string customerId) + { + ArgumentException.ThrowIfNullOrEmpty(customerId); + return await EndAll(await _sessionRepository.ToListAsync( + _sessionRepository.Table.Where(x => x.CustomerId == customerId && x.EndedUtc == null)), + OidcSessionEndReason.LinkRemoved); + } + + public virtual async Task EndAllOpen() + { + //one update for every session (there may be many): the count is read first, a session started meanwhile + //is ended as well + var count = await _sessionRepository.CountAsync(_sessionRepository.Table.Where(x => x.EndedUtc == null)); + await _sessionRepository.UpdateManyAsync(x => x.EndedUtc == null, + UpdateBuilder.Create().Set(x => x.EndedUtc, DateTime.UtcNow) + .Set(x => x.EndReason, OidcSessionEndReason.LinkRemoved).Set(x => x.ProtectedRefreshToken, null)); + await _cache.RemoveByPrefix(CacheKey.OIDC_SESSION_PATTERN_KEY); + _logger.LogInformation("All {Count} open OpenID Connect sessions ended: {Reason}", count, + OidcSessionEndReason.LinkRemoved); + return count; + } + public virtual async Task DeleteExpired() { var now = DateTime.UtcNow; @@ -284,10 +306,11 @@ public virtual async Task DeleteExpired() await _sessionRepository.DeleteManyAsync(x => x.EndedUtc == null && x.LastValidatedUtc < abandoned); } - private async Task> EndAll(IList sessions) + private async Task> EndAll(IList sessions, + OidcSessionEndReason reason = OidcSessionEndReason.BackChannel) { foreach (var session in sessions) - await End(session.Id, OidcSessionEndReason.BackChannel); + await End(session.Id, reason); return sessions; } diff --git a/src/Business/Grand.Business.Authentication/Services/OidcSsoSignInPolicy.cs b/src/Business/Grand.Business.Authentication/Services/OidcSsoSignInPolicy.cs index 8c6b94d830..3a0f8457e0 100644 --- a/src/Business/Grand.Business.Authentication/Services/OidcSsoSignInPolicy.cs +++ b/src/Business/Grand.Business.Authentication/Services/OidcSsoSignInPolicy.cs @@ -25,6 +25,7 @@ public class OidcSsoSignInPolicy( OidcLoginConfig config, OidcResourceServerConfig resourceServerConfig, IMemoryCache memoryCache, + IOidcLinkService linkService, ILogger logger) : IExternalSignInPolicy { private const string UnavailableLogKey = "Grand.OidcSignInMode.Unavailable"; @@ -40,7 +41,8 @@ private async Task RequiredProvider(Customer cu var confirmed = await externalAuthenticationRepository.GetOneAsync(x => x.ProviderSystemName == OidcCustomerResolver.ProviderSystemName && x.CustomerId == customer.Id && !x.LinkedByEmail); - return confirmed != null ? provider : null; + //while the links belong to another provider the provider sign-in is refused: the account keeps its password + return confirmed != null && await linkService.LinksMatchProvider() ? provider : null; } public async Task PasswordSignIn(Customer customer, SignInSurface surface) diff --git a/src/Business/Grand.Business.Authentication/Services/OidcTokenRevocationService.cs b/src/Business/Grand.Business.Authentication/Services/OidcTokenRevocationService.cs index 5eaddd766c..1ccd8c3964 100644 --- a/src/Business/Grand.Business.Authentication/Services/OidcTokenRevocationService.cs +++ b/src/Business/Grand.Business.Authentication/Services/OidcTokenRevocationService.cs @@ -1,30 +1,69 @@ using Grand.Business.Core.Interfaces.Authentication; +using Grand.Business.Core.Utilities.Authentication; using Grand.Data; using Grand.Domain.Customers; +using System.Security.Cryptography; +using System.Text; namespace Grand.Business.Authentication.Services; /// -/// Keeps the revocation time on the subject's links (one per store with per-store identity). Not cached: the check -/// must hold at once on every instance, and the lookup uses the provider + identifier index +/// Keeps the revocation time of a subject in its own record, not on the subject's links: a link removed in the panels +/// (or all of them, after the provider changed) must not bring the revoked tokens back. Not cached: the check must +/// hold at once on every instance, and the record is read by its key /// -public class OidcTokenRevocationService(IRepository externalAuthenticationRepository) +public class OidcTokenRevocationService(IRepository revocationRepository) : IOidcTokenRevocationService { - public Task RevokeTokens(string subject) + public async Task RevokeTokens(string subject) { ArgumentException.ThrowIfNullOrEmpty(subject); - return externalAuthenticationRepository.UpdateManyAsync( - x => x.ProviderSystemName == OidcCustomerResolver.ProviderSystemName && x.ExternalIdentifier == subject, - UpdateBuilder.Create().Set(x => x.TokensRevokedBeforeUtc, DateTime.UtcNow)); + var id = RevocationId(subject); + var now = DateTime.UtcNow; + + if (await revocationRepository.GetByIdAsync(id) == null) + try + { + await revocationRepository.InsertAsync(new OidcTokenRevocation { + Id = id, Subject = subject, RevokedBeforeUtc = now + }); + return; + } + catch (Exception) + { + //a concurrent revocation stored the record first: move its time forward below; any other failure is + //not swallowed, the caller (back-channel logout) answers with an error and the provider retries + if (await revocationRepository.GetByIdAsync(id) == null) + throw; + } + + //only forward: a later revocation already stored is never undone + await revocationRepository.UpdateOneAsync(x => x.Id == id && x.RevokedBeforeUtc < now, + UpdateBuilder.Create().Set(x => x.RevokedBeforeUtc, now)); } public async Task IsRevoked(string subject, DateTime issuedUtc) { if (string.IsNullOrEmpty(subject)) return false; - return await externalAuthenticationRepository.GetOneAsync(x => - x.ProviderSystemName == OidcCustomerResolver.ProviderSystemName && x.ExternalIdentifier == subject && - x.TokensRevokedBeforeUtc != null && x.TokensRevokedBeforeUtc > issuedUtc) != null; + var revocation = await revocationRepository.GetByIdAsync(RevocationId(subject)); + return revocation != null && revocation.RevokedBeforeUtc > issuedUtc; + } + + public Task DeleteExpired() + { + var expired = DateTime.UtcNow - OidcSessionDefaults.RevocationRetention; + return revocationRepository.DeleteManyAsync(x => x.RevokedBeforeUtc < expired); + } + + /// + /// The first 12 bytes of SHA-256 over "OidcTokenRevocation:" + subject, as 24 lower-case hex characters (shaped + /// like an ObjectId); subjects are case-sensitive + /// + public static string RevocationId(string subject) + { + ArgumentException.ThrowIfNullOrEmpty(subject); + var hash = SHA256.HashData(Encoding.UTF8.GetBytes("OidcTokenRevocation:" + subject)); + return Convert.ToHexStringLower(hash, 0, 12); } } diff --git a/src/Business/Grand.Business.Authentication/Startup/StartupApplication.cs b/src/Business/Grand.Business.Authentication/Startup/StartupApplication.cs index 60990f696e..cebea41ce4 100644 --- a/src/Business/Grand.Business.Authentication/Startup/StartupApplication.cs +++ b/src/Business/Grand.Business.Authentication/Startup/StartupApplication.cs @@ -28,6 +28,7 @@ public void ConfigureServices(IServiceCollection services, IConfiguration config services.AddScoped(); services.AddScoped(); services.AddScoped(); + services.AddScoped(); services.AddScoped(); } diff --git a/src/Business/Grand.Business.Core/Interfaces/Authentication/IOidcLinkService.cs b/src/Business/Grand.Business.Core/Interfaces/Authentication/IOidcLinkService.cs new file mode 100644 index 0000000000..77a9e29e71 --- /dev/null +++ b/src/Business/Grand.Business.Core/Interfaces/Authentication/IOidcLinkService.cs @@ -0,0 +1,51 @@ +using Grand.Domain.Customers; + +namespace Grand.Business.Core.Interfaces.Authentication; + +/// +/// The stored links between customers and identities at the OpenID Connect provider: whether they belong to the +/// configured provider, and their removal +/// +public interface IOidcLinkService +{ + /// + /// False when OidcResourceServer:Authority differs from the provider the links were made with, or links exist and + /// their provider was never recorded: no link may then be used or made until an administrator removes or keeps + /// them. Without any link the configured provider is adopted + /// + Task LinksMatchProvider(); + + /// The configured provider, the provider of the links and how many links exist + Task GetState(); + + /// Removes every OpenID Connect link, ends the open provider sessions and adopts the configured provider + Task RemoveAll(); + + /// Keeps the links for the configured provider (the same provider at a new address) + Task KeepAll(); + + /// Removes one external link of the customer; an OpenID Connect link also ends the customer's provider sessions + Task Remove(Customer customer, string linkId); +} + +/// OidcResourceServer:Authority +/// The provider the links were made with (empty before the first use) +/// Stored OpenID Connect links +public sealed record OidcLinkState(string ConfiguredAuthority, string LinkedAuthority, long LinkCount) +{ + /// Links exist and their provider differs from the configured one, or was never recorded + public bool ProviderChanged => LinkCount > 0 && + (ProviderUnknown || !OidcAuthority.SameProvider(LinkedAuthority, ConfiguredAuthority)); + + /// Links exist that were made before their provider was recorded + public bool ProviderUnknown => LinkCount > 0 && string.IsNullOrEmpty(LinkedAuthority); +} + +public static class OidcAuthority +{ + /// Authorities compared without a trailing slash and without regard to case + public static bool SameProvider(string a, string b) => + string.Equals(Normalize(a), Normalize(b), StringComparison.OrdinalIgnoreCase); + + public static string Normalize(string authority) => (authority ?? "").Trim().TrimEnd('/'); +} diff --git a/src/Business/Grand.Business.Core/Interfaces/Authentication/IOidcSessionService.cs b/src/Business/Grand.Business.Core/Interfaces/Authentication/IOidcSessionService.cs index 23142d3a59..aec229e1fb 100644 --- a/src/Business/Grand.Business.Core/Interfaces/Authentication/IOidcSessionService.cs +++ b/src/Business/Grand.Business.Core/Interfaces/Authentication/IOidcSessionService.cs @@ -27,6 +27,12 @@ public interface IOidcSessionService /// Ends every open session of the subject; returns them Task> EndBySubject(string subject); + /// Ends every open session of the customer (its provider link was removed); returns them + Task> EndByCustomer(string customerId); + + /// Ends every open session (the links to the provider were removed); returns how many ended + Task EndAllOpen(); + /// Deletes ended sessions and sessions past their lifetime, after the retention period Task DeleteExpired(); } diff --git a/src/Business/Grand.Business.Core/Interfaces/Authentication/IOidcTokenRevocationService.cs b/src/Business/Grand.Business.Core/Interfaces/Authentication/IOidcTokenRevocationService.cs index e00e5386ec..692e515b82 100644 --- a/src/Business/Grand.Business.Core/Interfaces/Authentication/IOidcTokenRevocationService.cs +++ b/src/Business/Grand.Business.Core/Interfaces/Authentication/IOidcTokenRevocationService.cs @@ -11,4 +11,7 @@ public interface IOidcTokenRevocationService /// The token of the subject issued at is refused Task IsRevoked(string subject, DateTime issuedUtc); + + /// Deletes revocations older than any access token they could still refuse + Task DeleteExpired(); } diff --git a/src/Business/Grand.Business.Core/Utilities/Authentication/OidcLoginResult.cs b/src/Business/Grand.Business.Core/Utilities/Authentication/OidcLoginResult.cs index 76867a2dd2..c3bea558e4 100644 --- a/src/Business/Grand.Business.Core/Utilities/Authentication/OidcLoginResult.cs +++ b/src/Business/Grand.Business.Core/Utilities/Authentication/OidcLoginResult.cs @@ -21,14 +21,16 @@ public enum OidcLoginRefusal AccountUnusable, LinkedToOtherIdentity, RegistrationDisabled, - EmailNotVerified + EmailNotVerified, + ProviderChanged } public enum OidcLinkResult { Linked, SubjectLinkedToOtherCustomer, - CustomerLinkedToOtherSubject + CustomerLinkedToOtherSubject, + ProviderChanged } /// diff --git a/src/Business/Grand.Business.Core/Utilities/Authentication/OidcSessionDefaults.cs b/src/Business/Grand.Business.Core/Utilities/Authentication/OidcSessionDefaults.cs index 478e63468f..8191ca5b7b 100644 --- a/src/Business/Grand.Business.Core/Utilities/Authentication/OidcSessionDefaults.cs +++ b/src/Business/Grand.Business.Core/Utilities/Authentication/OidcSessionDefaults.cs @@ -16,4 +16,10 @@ public static class OidcSessionDefaults /// providers drop an unused offline session after about as long) /// public static readonly TimeSpan AbandonedAfter = TimeSpan.FromDays(30); + + /// + /// How long a token revocation is kept: longer than an identity provider's access token lives, so no token it + /// refused becomes valid again + /// + public static readonly TimeSpan RevocationRetention = TimeSpan.FromDays(7); } diff --git a/src/Core/Grand.Domain/Customers/ExternalAuthentication.cs b/src/Core/Grand.Domain/Customers/ExternalAuthentication.cs index 1fb8169a56..d657c2c5ce 100644 --- a/src/Core/Grand.Domain/Customers/ExternalAuthentication.cs +++ b/src/Core/Grand.Domain/Customers/ExternalAuthentication.cs @@ -44,10 +44,4 @@ public class ExternalAuthentication : BaseEntity /// Gets or sets a value indicating whether the link was made by the e-mail address alone, without the account's password /// public bool LinkedByEmail { get; set; } - - /// - /// Gets or sets the time before which the identity provider's access tokens of this subject are refused (provider - /// sign-out, or the provider refused the account); empty = never - /// - public DateTime? TokensRevokedBeforeUtc { get; set; } } \ No newline at end of file diff --git a/src/Core/Grand.Domain/Customers/OidcLinkSettings.cs b/src/Core/Grand.Domain/Customers/OidcLinkSettings.cs new file mode 100644 index 0000000000..a5f07b648c --- /dev/null +++ b/src/Core/Grand.Domain/Customers/OidcLinkSettings.cs @@ -0,0 +1,13 @@ +using Grand.Domain.Configuration; + +namespace Grand.Domain.Customers; + +/// +/// The identity provider the stored OpenID Connect links belong to (store-independent). A subject is unique only within +/// one provider, so links are not used once OidcResourceServer:Authority points elsewhere +/// +public class OidcLinkSettings : ISettings +{ + /// Authority the links were made with; empty until the first use, which adopts the configured one + public string Authority { get; set; } +} diff --git a/src/Core/Grand.Domain/Customers/OidcSession.cs b/src/Core/Grand.Domain/Customers/OidcSession.cs index 73d46eb325..5a2d631e41 100644 --- a/src/Core/Grand.Domain/Customers/OidcSession.cs +++ b/src/Core/Grand.Domain/Customers/OidcSession.cs @@ -44,5 +44,6 @@ public enum OidcSessionEndReason MaxLifetime = 4, RevalidationRefused = 5, RevalidationUnavailable = 6, - NoRefreshToken = 7 + NoRefreshToken = 7, + LinkRemoved = 8 } diff --git a/src/Core/Grand.Domain/Customers/OidcTokenRevocation.cs b/src/Core/Grand.Domain/Customers/OidcTokenRevocation.cs new file mode 100644 index 0000000000..7956a18462 --- /dev/null +++ b/src/Core/Grand.Domain/Customers/OidcTokenRevocation.cs @@ -0,0 +1,19 @@ +namespace Grand.Domain.Customers; + +/// +/// The time before which the identity provider's access tokens of a subject are refused (provider sign-out, or the +/// provider refused the account). Kept apart from the subject's links, so removing a link does not bring its revoked +/// tokens back; the identifier is derived from the subject (one record per subject, read by its key) +/// +public class OidcTokenRevocation : BaseEntity +{ + /// + /// Gets or sets the subject (sub claim) at the identity provider + /// + public string Subject { get; set; } + + /// + /// Gets or sets the time before which the subject's access tokens are refused + /// + public DateTime RevokedBeforeUtc { get; set; } +} diff --git a/src/Modules/Grand.Module.ScheduledTasks/BackgroundServices/DeleteOidcSessionsScheduleTask.cs b/src/Modules/Grand.Module.ScheduledTasks/BackgroundServices/DeleteOidcSessionsScheduleTask.cs index c004f06685..250e6705e0 100644 --- a/src/Modules/Grand.Module.ScheduledTasks/BackgroundServices/DeleteOidcSessionsScheduleTask.cs +++ b/src/Modules/Grand.Module.ScheduledTasks/BackgroundServices/DeleteOidcSessionsScheduleTask.cs @@ -4,9 +4,16 @@ namespace Grand.Module.ScheduledTasks.BackgroundServices; /// -/// Deletes the records of ended OpenID Connect sessions and of sessions past their lifetime +/// Deletes the records of ended OpenID Connect sessions and of sessions past their lifetime, and token revocations +/// older than any token they could still refuse /// -public class DeleteOidcSessionsScheduleTask(IOidcSessionService oidcSessionService) : IScheduleTask +public class DeleteOidcSessionsScheduleTask( + IOidcSessionService oidcSessionService, + IOidcTokenRevocationService oidcTokenRevocationService) : IScheduleTask { - public Task Execute() => oidcSessionService.DeleteExpired(); + public async Task Execute() + { + await oidcSessionService.DeleteExpired(); + await oidcTokenRevocationService.DeleteExpired(); + } } diff --git a/src/Plugins/Authentication.OpenIdConnect/Controllers/OidcLoginController.cs b/src/Plugins/Authentication.OpenIdConnect/Controllers/OidcLoginController.cs index f1f632e823..338642a0a7 100644 --- a/src/Plugins/Authentication.OpenIdConnect/Controllers/OidcLoginController.cs +++ b/src/Plugins/Authentication.OpenIdConnect/Controllers/OidcLoginController.cs @@ -232,6 +232,7 @@ public async Task LinkComplete(string returnUrl) OidcLinkResult.Linked => RedirectToRoute("OidcLoginLogin", new { returnUrl }), //the provider identity, not this account, is the one already connected elsewhere OidcLinkResult.SubjectLinkedToOtherCustomer => RedirectToRoute("OidcLoginFailed", new { taken = true }), + OidcLinkResult.ProviderChanged => RedirectToRoute("OidcLoginFailed", new { reason = (int)OidcLoginRefusal.ProviderChanged }), _ => RedirectToRoute("OidcLoginFailed", new { reason = (int)OidcLoginRefusal.LinkedToOtherIdentity }) }; } diff --git a/src/Plugins/Authentication.OpenIdConnect/Infrastructure/OidcBackChannelLogoutEndpoint.cs b/src/Plugins/Authentication.OpenIdConnect/Infrastructure/OidcBackChannelLogoutEndpoint.cs index ca94d31bfb..fb09a2d526 100644 --- a/src/Plugins/Authentication.OpenIdConnect/Infrastructure/OidcBackChannelLogoutEndpoint.cs +++ b/src/Plugins/Authentication.OpenIdConnect/Infrastructure/OidcBackChannelLogoutEndpoint.cs @@ -1,4 +1,5 @@ using Grand.Business.Core.Interfaces.Authentication; +using Grand.Domain.Customers; using Grand.Infrastructure.Configuration; using Microsoft.AspNetCore.Authentication; using Microsoft.AspNetCore.Authentication.OpenIdConnect; @@ -69,10 +70,20 @@ public static async Task Handle(HttpContext context) try { - var sessionService = services.GetRequiredService(); - var ended = logout.Sid != null - ? await sessionService.EndBySid(logout.Subject, logout.Sid) - : await sessionService.EndBySubject(logout.Subject); + //the sessions were started at the provider the links belong to: while that is not the configured one, a + //subject or sid of the configured provider may name another person's session - the subject's tokens are + //still refused (a token is checked against the subject of its own issuer) + IList ended = []; + if (await services.GetRequiredService().LinksMatchProvider()) + { + var sessionService = services.GetRequiredService(); + ended = logout.Sid != null + ? await sessionService.EndBySid(logout.Subject, logout.Sid) + : await sessionService.EndBySubject(logout.Subject); + } + else + logger.LogWarning("Back-channel logout: no session ended, the identity provider changed and the account " + + "links are not reviewed yet"); //a logout token with sid only names the subject through the sessions it ended var subjects = logout.Subject != null diff --git a/src/Plugins/Authentication.OpenIdConnect/OidcLoginPlugin.cs b/src/Plugins/Authentication.OpenIdConnect/OidcLoginPlugin.cs index 088b5c8c26..b3d3947f3d 100644 --- a/src/Plugins/Authentication.OpenIdConnect/OidcLoginPlugin.cs +++ b/src/Plugins/Authentication.OpenIdConnect/OidcLoginPlugin.cs @@ -37,6 +37,8 @@ private static readonly (string Key, string Value)[] Resources = [ "Signing in with the identity provider is not set up completely. Use another sign-in method, or ask the administrator."), ("Failed.EmailNotVerified", "Your identity provider has not verified your e-mail address. Verify it there, then sign in again."), + ("Failed.ProviderChanged", + "Signing in with the identity provider is paused while the administrator updates the store's connection to it. Try again later."), ("DisplayOrder", "Display order"), ("ClientId", "Client ID"), ("ClientSecret", "Client secret"), diff --git a/src/Plugins/Authentication.OpenIdConnect/Views/OidcLogin/Failed.cshtml b/src/Plugins/Authentication.OpenIdConnect/Views/OidcLogin/Failed.cshtml index f1d4558dee..8c48673fbd 100644 --- a/src/Plugins/Authentication.OpenIdConnect/Views/OidcLogin/Failed.cshtml +++ b/src/Plugins/Authentication.OpenIdConnect/Views/OidcLogin/Failed.cshtml @@ -15,6 +15,7 @@ OidcLoginRefusal.RegistrationDisabled => "Plugins.ExternalAuth.OpenIdConnect.Failed.RegistrationDisabled", OidcLoginRefusal.LinkedToOtherIdentity => "Plugins.ExternalAuth.OpenIdConnect.Failed.LinkedToOtherIdentity", OidcLoginRefusal.EmailNotVerified => "Plugins.ExternalAuth.OpenIdConnect.Failed.EmailNotVerified", + OidcLoginRefusal.ProviderChanged => "Plugins.ExternalAuth.OpenIdConnect.Failed.ProviderChanged", _ => "Plugins.ExternalAuth.OpenIdConnect.Failed.Generic" }; } diff --git a/src/Tests/Grand.Business.Authentication.Tests/Services/CookieAuthenticationServiceTests.cs b/src/Tests/Grand.Business.Authentication.Tests/Services/CookieAuthenticationServiceTests.cs index 85066d7631..f264057997 100644 --- a/src/Tests/Grand.Business.Authentication.Tests/Services/CookieAuthenticationServiceTests.cs +++ b/src/Tests/Grand.Business.Authentication.Tests/Services/CookieAuthenticationServiceTests.cs @@ -30,6 +30,7 @@ public class CookieAuthenticationServiceTests private CookieOptionsFactory _cookieOptionsFactory; private Mock _oidcSessionService; private Mock _signInPolicy; + private Mock _linkService; [TestInitialize] public void Init() @@ -46,6 +47,8 @@ public void Init() _cookieOptionsFactory = new CookieOptionsFactory(_config); _oidcSessionService = new Mock(); _signInPolicy = new Mock(); + _linkService = new Mock(); + _linkService.Setup(x => x.LinksMatchProvider()).ReturnsAsync(true); _signInPolicy.Setup(x => x.PasswordSignIn(It.IsAny(), It.IsAny())) .ReturnsAsync(PasswordSignInDecision.Password); _cookieAuthService = NewService(); @@ -205,7 +208,8 @@ public async Task SignIn_WithOidcSession_AddsClaim() private CookieAuthenticationService NewService() => new(_customerSettings, _customerServiceMock.Object, _groupServiceMock.Object, _httpAccessorMock.Object, _cookieOptionsFactory, _config, - _oidcSessionService.Object, _signInPolicy.Object, NullLogger.Instance); + _oidcSessionService.Object, _signInPolicy.Object, _linkService.Object, + NullLogger.Instance); [TestMethod] public async Task GetAuthenticatedCustomer_NoSessionClaim_SessionServiceNotAsked() @@ -285,6 +289,19 @@ public async Task GetAuthenticatedCustomer_ValidSession_ReturnCustomer() Assert.AreSame(customer, await _cookieAuthService.GetAuthenticatedCustomer()); } + //the session was started at the provider the links belong to, not at the configured one: never confirmed there + [TestMethod] + public async Task GetAuthenticatedCustomer_ProviderChanged_SessionNotUsed() + { + var customer = RegisteredCustomer(); + _authServiceMock.Setup(x => x.AuthenticateAsync(It.IsAny(), It.IsAny())) + .ReturnsAsync(Ticket(new Claim(OidcSessionDefaults.SessionIdClaimType, "session-1", ClaimValueTypes.String, "grandnode"))); + _oidcSessionService.Setup(x => x.Validate("session-1", customer)).ReturnsAsync(true); + _linkService.Setup(x => x.LinksMatchProvider()).ReturnsAsync(false); + Assert.IsNull(await _cookieAuthService.GetAuthenticatedCustomer()); + _oidcSessionService.Verify(x => x.Validate(It.IsAny(), It.IsAny()), Times.Never); + } + //a claim not issued by the cookie (another issuer) is not a session reference [TestMethod] public async Task GetAuthenticatedCustomer_SessionClaimOfOtherIssuer_Ignored() diff --git a/src/Tests/Grand.Business.Authentication.Tests/Services/OidcCustomerResolverLoginTests.cs b/src/Tests/Grand.Business.Authentication.Tests/Services/OidcCustomerResolverLoginTests.cs index d262863268..8b5e5529bd 100644 --- a/src/Tests/Grand.Business.Authentication.Tests/Services/OidcCustomerResolverLoginTests.cs +++ b/src/Tests/Grand.Business.Authentication.Tests/Services/OidcCustomerResolverLoginTests.cs @@ -1,4 +1,5 @@ using Grand.Business.Authentication.Services; +using Grand.Business.Core.Interfaces.Authentication; using Grand.Business.Core.Events.Customers; using Grand.Business.Core.Interfaces.Common.Configuration; using Grand.Business.Core.Interfaces.Common.Directory; @@ -36,9 +37,13 @@ public class OidcCustomerResolverLoginTests private CustomerConfig _customerConfig; private CustomerSettings _customerSettings; + private Mock _linkService; + [TestInitialize] public void Init() { + _linkService = new Mock(); + _linkService.Setup(x => x.LinksMatchProvider()).ReturnsAsync(true); _links = new MongoDBRepositoryTest(); _customerService = new Mock(); _customerManagerService = new Mock(); @@ -58,7 +63,7 @@ public void Init() private OidcCustomerResolver Resolver(IRepository links = null) => new(links ?? _links, _customerService.Object, _customerManagerService.Object, _settingService.Object, - _groupService.Object, _permissionService.Object, _mediator.Object, _config, _loginConfig, _customerConfig, NullLogger.Instance); + _groupService.Object, _permissionService.Object, _mediator.Object, _config, _loginConfig, _customerConfig, NullLogger.Instance, _linkService.Object); private Task Link(string customerId, string sub = "sub-1", string provider = OidcCustomerResolver.ProviderSystemName, string storeId = null, bool byEmail = false) => @@ -77,6 +82,27 @@ private List LinksOf(string sub) => public async Task NoSubject_Refused_InvalidIdentity() => Assert.AreEqual(OidcLoginRefusal.InvalidIdentity, (await Resolver().ResolveForLogin(null, "a@b.c", true, Store)).Refusal); + [TestMethod] + public async Task ProviderChanged_Linked_RefusedNothingLoaded() + { + _linkService.Setup(x => x.LinksMatchProvider()).ReturnsAsync(false); + await Link("c1"); + + Assert.AreEqual(OidcLoginRefusal.ProviderChanged, + (await Resolver().ResolveForLogin("sub-1", "a@b.c", true, Store)).Refusal); + _customerService.Verify(x => x.GetCustomerById(It.IsAny()), Times.Never); + } + + [TestMethod] + public async Task ProviderChanged_LinkIdentity_NothingStored() + { + _linkService.Setup(x => x.LinksMatchProvider()).ReturnsAsync(false); + + Assert.AreEqual(OidcLinkResult.ProviderChanged, + await Resolver().LinkIdentity(new Customer { Id = "c1" }, "sub-1", "a@b.c", Store)); + Assert.AreEqual(0, LinksOf("sub-1").Count); + } + [TestMethod] public async Task Linked_SignIn_WithoutEmail() { diff --git a/src/Tests/Grand.Business.Authentication.Tests/Services/OidcCustomerResolverTests.cs b/src/Tests/Grand.Business.Authentication.Tests/Services/OidcCustomerResolverTests.cs index f96c4296ab..af7f22008d 100644 --- a/src/Tests/Grand.Business.Authentication.Tests/Services/OidcCustomerResolverTests.cs +++ b/src/Tests/Grand.Business.Authentication.Tests/Services/OidcCustomerResolverTests.cs @@ -1,4 +1,5 @@ using Grand.Business.Authentication.Services; +using Grand.Business.Core.Interfaces.Authentication; using Grand.Business.Core.Events.Customers; using Grand.Business.Core.Interfaces.Common.Configuration; using Grand.Business.Core.Interfaces.Common.Directory; @@ -36,9 +37,13 @@ public class OidcCustomerResolverTests private CustomerConfig _customerConfig; private CustomerSettings _customerSettings; + private Mock _linkService; + [TestInitialize] public void Init() { + _linkService = new Mock(); + _linkService.Setup(x => x.LinksMatchProvider()).ReturnsAsync(true); _links = new MongoDBRepositoryTest(); _customerService = new Mock(); _customerManagerService = new Mock(); @@ -58,7 +63,7 @@ public void Init() private OidcCustomerResolver Resolver(IRepository links = null) => new(links ?? _links, _customerService.Object, _customerManagerService.Object, _settingService.Object, - _groupService.Object, _permissionService.Object, _mediator.Object, _config, _loginConfig, _customerConfig, NullLogger.Instance); + _groupService.Object, _permissionService.Object, _mediator.Object, _config, _loginConfig, _customerConfig, NullLogger.Instance, _linkService.Object); private static ClaimsPrincipal Principal(string sub = "sub-1", string email = "John@Example.com", string emailVerified = "true") @@ -84,6 +89,18 @@ private List LinksOf(string sub) => public async Task NoSubject_InvalidToken() => Assert.AreEqual(OidcFailureKind.InvalidToken, (await Resolver().Resolve(Principal(sub: null), "store1")).Failure); + [TestMethod] + public async Task ProviderChanged_LinkedSubject_Forbidden_CustomerNotLoaded() + { + _linkService.Setup(x => x.LinksMatchProvider()).ReturnsAsync(false); + await Link("c1"); + + var result = await Resolver().Resolve(Principal(), "store1"); + + Assert.AreEqual(OidcFailureKind.Forbidden, result.Failure); + _customerService.Verify(x => x.GetCustomerById(It.IsAny()), Times.Never); + } + [TestMethod] public async Task CustomSubjectClaim_IsUsed() { diff --git a/src/Tests/Grand.Business.Authentication.Tests/Services/OidcLinkServiceTests.cs b/src/Tests/Grand.Business.Authentication.Tests/Services/OidcLinkServiceTests.cs new file mode 100644 index 0000000000..979f6aee0f --- /dev/null +++ b/src/Tests/Grand.Business.Authentication.Tests/Services/OidcLinkServiceTests.cs @@ -0,0 +1,206 @@ +using Grand.Business.Authentication.Services; +using Grand.Business.Core.Interfaces.Authentication; +using Grand.Business.Core.Interfaces.Common.Configuration; +using Grand.Data; +using Grand.Data.Tests.MongoDb; +using Grand.Domain.Customers; +using Grand.Infrastructure.Configuration; +using Microsoft.Extensions.Logging.Abstractions; +using Microsoft.VisualStudio.TestTools.UnitTesting; +using Moq; + +namespace Grand.Business.Authentication.Tests.Services; + +[TestClass] +public class OidcLinkServiceTests +{ + private const string Oidc = OidcCustomerResolver.ProviderSystemName; + + private IRepository _links; + private Mock _settings; + private Mock _sessions; + private OidcLinkSettings _stored; + private OidcResourceServerConfig _config; + + [TestInitialize] + public void Init() + { + _links = new MongoDBRepositoryTest(); + _stored = new OidcLinkSettings(); + _settings = new Mock(); + _settings.Setup(x => x.LoadSetting(It.IsAny())).ReturnsAsync(() => _stored); + _settings.Setup(x => x.SaveSetting(It.IsAny(), It.IsAny())) + .Callback((OidcLinkSettings s, string _) => _stored = s).Returns(Task.CompletedTask); + _sessions = new Mock(); + _sessions.Setup(x => x.EndAllOpen()).ReturnsAsync(3); + _sessions.Setup(x => x.EndByCustomer(It.IsAny())).ReturnsAsync(new List()); + _config = new OidcResourceServerConfig { Enabled = true, Authority = "https://idp.example/realms/shop/" }; + } + + private OidcLinkService Service() => + new(_settings.Object, _config, _links, _sessions.Object, NullLogger.Instance); + + private async Task Link(string customerId, string provider = Oidc, string sub = "sub-1") + { + var link = new ExternalAuthentication { CustomerId = customerId, ProviderSystemName = provider, ExternalIdentifier = sub }; + await _links.InsertAsync(link); + return link; + } + + [TestMethod] + public async Task FirstUse_AdoptsTheConfiguredProvider() + { + Assert.IsTrue(await Service().LinksMatchProvider()); + Assert.AreEqual("https://idp.example/realms/shop", _stored.Authority); + } + + [TestMethod] + [DataRow("https://idp.example/realms/shop")] + [DataRow("https://IDP.example/realms/shop/")] + public async Task SameProvider_TrailingSlashAndCase_Match(string stored) + { + _stored.Authority = stored; + Assert.IsTrue(await Service().LinksMatchProvider()); + } + + [TestMethod] + public async Task OtherProvider_DoesNotMatch_NothingAdopted() + { + _stored.Authority = "https://old-idp.example"; + await Link("c1"); + Assert.IsFalse(await Service().LinksMatchProvider()); + Assert.AreEqual("https://old-idp.example", _stored.Authority); + } + + //nobody connected yet: no subject could select another person's account + [TestMethod] + public async Task OtherProvider_NoLinks_AdoptsTheConfiguredProvider() + { + _stored.Authority = "https://old-idp.example"; + await Link("c1", "ExternalAuth.Google", "g-1"); + Assert.IsTrue(await Service().LinksMatchProvider()); + Assert.AreEqual("https://idp.example/realms/shop", _stored.Authority); + } + + //links made before the provider was recorded: the provider may have changed with the same upgrade + [TestMethod] + public async Task LinksOfAnUnrecordedProvider_DoNotMatch_NothingAdopted() + { + await Link("c1"); + Assert.IsFalse(await Service().LinksMatchProvider()); + Assert.IsTrue(string.IsNullOrEmpty(_stored.Authority)); + + var state = await Service().GetState(); + Assert.IsTrue(state.ProviderUnknown); + Assert.IsTrue(state.ProviderChanged); + } + + //concurrent first requests record the provider once (each save clears the whole cache) + [TestMethod] + public async Task FirstUse_ConcurrentRequests_RecordTheProviderOnce() + { + _settings.Setup(x => x.SaveSetting(It.IsAny(), It.IsAny())) + .Callback((OidcLinkSettings s, string _) => _stored = s).Returns(() => Task.Delay(50)); + + var results = await Task.WhenAll(Enumerable.Range(0, 5).Select(_ => Service().LinksMatchProvider())); + + Assert.IsTrue(results.All(x => x)); + _settings.Verify(x => x.SaveSetting(It.IsAny(), It.IsAny()), Times.Once); + } + + [TestMethod] + public async Task NoConfiguredProvider_NothingRecorded() + { + _config.Authority = ""; + await Service().KeepAll(); + _settings.Verify(x => x.SaveSetting(It.IsAny(), It.IsAny()), Times.Never); + } + + [TestMethod] + public async Task GetState_CountsOpenIdConnectLinks_ReportsTheChange() + { + _stored.Authority = "https://old-idp.example"; + await Link("c1"); + await Link("c2", sub: "sub-2"); + await Link("c1", "ExternalAuth.Google", "g-1"); + + var state = await Service().GetState(); + + Assert.AreEqual(2, state.LinkCount); + Assert.IsTrue(state.ProviderChanged); + Assert.AreEqual("https://old-idp.example", state.LinkedAuthority); + } + + [TestMethod] + public async Task GetState_BeforeFirstUse_NotChanged() + { + Assert.IsFalse((await Service().GetState()).ProviderChanged); + } + + [TestMethod] + public async Task GetState_OtherProviderWithoutLinks_NotChanged() + { + _stored.Authority = "https://old-idp.example"; + Assert.IsFalse((await Service().GetState()).ProviderChanged); + } + + [TestMethod] + public async Task RemoveAll_OnlyOpenIdConnectLinks_EndsSessions_AdoptsTheProvider() + { + _stored.Authority = "https://old-idp.example"; + await Link("c1"); + await Link("c2", sub: "sub-2"); + var google = await Link("c1", "ExternalAuth.Google", "g-1"); + + Assert.AreEqual(2, await Service().RemoveAll()); + + Assert.AreEqual(google.Id, _links.Table.Single().Id); + _sessions.Verify(x => x.EndAllOpen(), Times.Once); + Assert.IsTrue(await Service().LinksMatchProvider()); + } + + [TestMethod] + public async Task KeepAll_LinksStay_ProviderAdopted() + { + _stored.Authority = "https://old-idp.example"; + await Link("c1"); + + await Service().KeepAll(); + + Assert.AreEqual(1, _links.Table.Count()); + Assert.IsTrue(await Service().LinksMatchProvider()); + _sessions.Verify(x => x.EndAllOpen(), Times.Never); + } + + [TestMethod] + public async Task Remove_OpenIdConnectLinkOfTheCustomer_EndsItsSessions() + { + var link = await Link("c1"); + + Assert.IsTrue(await Service().Remove(new Customer { Id = "c1" }, link.Id)); + + Assert.AreEqual(0, _links.Table.Count()); + _sessions.Verify(x => x.EndByCustomer("c1"), Times.Once); + } + + [TestMethod] + public async Task Remove_LinkOfAnotherCustomer_Refused() + { + var link = await Link("c2"); + + Assert.IsFalse(await Service().Remove(new Customer { Id = "c1" }, link.Id)); + + Assert.AreEqual(1, _links.Table.Count()); + _sessions.Verify(x => x.EndByCustomer(It.IsAny()), Times.Never); + } + + [TestMethod] + public async Task Remove_OtherProviderLink_NoProviderSessionsEnded() + { + var link = await Link("c1", "ExternalAuth.Google", "g-1"); + + Assert.IsTrue(await Service().Remove(new Customer { Id = "c1" }, link.Id)); + + _sessions.Verify(x => x.EndByCustomer(It.IsAny()), Times.Never); + } +} diff --git a/src/Tests/Grand.Business.Authentication.Tests/Services/OidcSessionServiceTests.cs b/src/Tests/Grand.Business.Authentication.Tests/Services/OidcSessionServiceTests.cs index 52ac0220c4..d66faca925 100644 --- a/src/Tests/Grand.Business.Authentication.Tests/Services/OidcSessionServiceTests.cs +++ b/src/Tests/Grand.Business.Authentication.Tests/Services/OidcSessionServiceTests.cs @@ -1,4 +1,4 @@ -using Grand.Business.Authentication.Services; +using Grand.Business.Authentication.Services; using Grand.Business.Core.Interfaces.Authentication; using Grand.Business.Core.Interfaces.Common.Security; using Grand.Business.Core.Utilities.Authentication; @@ -168,6 +168,33 @@ public async Task EndBySubject_AllOpenSessionsOfSubject() Assert.IsNull(Stored(other.Id).EndedUtc); } + [TestMethod] + public async Task EndByCustomer_OnlyOpenSessionsOfTheCustomer() + { + var a = await Session(sid: "sid-1"); + var b = await Session(sid: "sid-2", subject: "sub-2"); + var other = await Session(customerId: "c2"); + var ended = await Service().EndByCustomer("c1"); + CollectionAssert.AreEquivalent(new[] { a.Id, b.Id }, ended.Select(x => x.Id).ToArray()); + Assert.AreEqual(OidcSessionEndReason.LinkRemoved, Stored(a.Id).EndReason); + Assert.IsNull(Stored(other.Id).EndedUtc); + } + + [TestMethod] + public async Task EndAllOpen_EndsEveryOpenSession() + { + await Session(); + await Session(customerId: "c2"); + var alreadyEnded = await Session(); + await _sessions.UpdateField(alreadyEnded.Id, x => x.EndedUtc, DateTime.UtcNow.AddMinutes(-5)); + Assert.AreEqual(2, await Service().EndAllOpen()); + Assert.IsFalse(_sessions.Table.Any(x => x.EndedUtc == null)); + //one update, not one per session: every ended session drops its refresh token, the cached sessions go at once + Assert.AreEqual(2, _sessions.Table.Count(x => x.EndReason == OidcSessionEndReason.LinkRemoved)); + Assert.IsFalse(_sessions.Table.Any(x => x.EndReason == OidcSessionEndReason.LinkRemoved && x.ProtectedRefreshToken != null)); + _cache.Verify(x => x.RemoveByPrefix(CacheKey.OIDC_SESSION_PATTERN_KEY, true), Times.Once); + } + [TestMethod] public async Task DeleteExpired_KeepsRecentAndOpen() { diff --git a/src/Tests/Grand.Business.Authentication.Tests/Services/OidcSsoSignInPolicyTests.cs b/src/Tests/Grand.Business.Authentication.Tests/Services/OidcSsoSignInPolicyTests.cs index c65ea19460..407ef08801 100644 --- a/src/Tests/Grand.Business.Authentication.Tests/Services/OidcSsoSignInPolicyTests.cs +++ b/src/Tests/Grand.Business.Authentication.Tests/Services/OidcSsoSignInPolicyTests.cs @@ -21,6 +21,7 @@ public class OidcSsoSignInPolicyTests private OidcLoginConfig _config; private OidcResourceServerConfig _resourceServer; private IMemoryCache _cache; + private Mock _linkService; private readonly Customer _customer = new() { Id = "c1" }; [TestInitialize] @@ -28,6 +29,8 @@ public void Init() { _links = new MongoDBRepositoryTest(); _cache = new MemoryCache(new MemoryCacheOptions()); + _linkService = new Mock(); + _linkService.Setup(x => x.LinksMatchProvider()).ReturnsAsync(true); _provider = new Mock(); _provider.Setup(x => x.SystemName).Returns(OidcCustomerResolver.ProviderSystemName); _provider.Setup(x => x.FriendlyName).Returns("OpenID Connect"); @@ -38,7 +41,7 @@ public void Init() private OidcSsoSignInPolicy Policy(params IExternalAuthenticationProvider[] providers) => new(_links, providers.Length > 0 ? providers : [_provider.Object], _settings, _config, _resourceServer, - _cache, NullLogger.Instance); + _cache, _linkService.Object, NullLogger.Instance); private Task Link(bool byEmail) => _links.InsertAsync(new ExternalAuthentication { CustomerId = "c1", ProviderSystemName = OidcCustomerResolver.ProviderSystemName, ExternalIdentifier = "sub-1", @@ -57,6 +60,15 @@ public async Task ConfirmedLink_ProviderRequired_WhateverTheAccount(bool staff) Assert.AreSame(_provider.Object, decision.Provider); } + //the provider sign-in is refused until the links are reviewed: requiring it would lock the account out + [TestMethod] + public async Task ConfirmedLink_ProviderChanged_PasswordAllowed() + { + await Link(byEmail: false); + _linkService.Setup(x => x.LinksMatchProvider()).ReturnsAsync(false); + Assert.IsTrue((await Policy().PasswordSignIn(_customer, SignInSurface.Storefront)).Allowed); + } + //break-glass: an account never connected to the provider keeps its password [TestMethod] public async Task WithoutLink_PasswordAllowed() => Assert.IsTrue((await Policy().PasswordSignIn(_customer, SignInSurface.Storefront)).Allowed); @@ -90,7 +102,7 @@ public async Task PluginNotInstalled_PasswordAllowed() { await Link(byEmail: false); Assert.IsTrue((await new OidcSsoSignInPolicy(_links, [], _settings, _config, _resourceServer, - _cache, NullLogger.Instance).PasswordSignIn(_customer, SignInSurface.Storefront)).Allowed); + _cache, _linkService.Object, NullLogger.Instance).PasswordSignIn(_customer, SignInSurface.Storefront)).Allowed); } [TestMethod] @@ -166,7 +178,7 @@ public async Task ProviderOnly_PluginMissing_RefusedWithoutProvider() { ProviderOnly(); var decision = await new OidcSsoSignInPolicy(_links, [], _settings, _config, _resourceServer, - _cache, NullLogger.Instance).PasswordSignIn(_customer, SignInSurface.Storefront); + _cache, _linkService.Object, NullLogger.Instance).PasswordSignIn(_customer, SignInSurface.Storefront); Assert.IsFalse(decision.Allowed); Assert.IsNull(decision.Provider); } @@ -177,7 +189,7 @@ public async Task ProviderOnly_PluginMissing_LoggedOncePerWindow() { ProviderOnly(); var logger = new Mock>(); - var policy = new OidcSsoSignInPolicy(_links, [], _settings, _config, _resourceServer, _cache, logger.Object); + var policy = new OidcSsoSignInPolicy(_links, [], _settings, _config, _resourceServer, _cache, _linkService.Object, logger.Object); await policy.PasswordSignIn(_customer, SignInSurface.Storefront); await policy.PasswordSignIn(_customer, SignInSurface.Storefront); logger.Verify(x => x.Log(LogLevel.Error, It.IsAny(), It.IsAny(), null, diff --git a/src/Tests/Grand.Business.Authentication.Tests/Services/OidcTokenRevocationServiceTests.cs b/src/Tests/Grand.Business.Authentication.Tests/Services/OidcTokenRevocationServiceTests.cs index 8b9e23cbcb..49d7d0becf 100644 --- a/src/Tests/Grand.Business.Authentication.Tests/Services/OidcTokenRevocationServiceTests.cs +++ b/src/Tests/Grand.Business.Authentication.Tests/Services/OidcTokenRevocationServiceTests.cs @@ -1,4 +1,5 @@ using Grand.Business.Authentication.Services; +using Grand.Business.Core.Utilities.Authentication; using Grand.Data; using Grand.Data.Tests.MongoDb; using Grand.Domain.Customers; @@ -9,55 +10,75 @@ namespace Grand.Business.Authentication.Tests.Services; [TestClass] public class OidcTokenRevocationServiceTests { - private IRepository _links; + private IRepository _revocations; private OidcTokenRevocationService _service; [TestInitialize] public void Init() { - _links = new MongoDBRepositoryTest(); - _service = new OidcTokenRevocationService(_links); + _revocations = new MongoDBRepositoryTest(); + _service = new OidcTokenRevocationService(_revocations); } - private Task Link(string customerId, string sub, string provider = OidcCustomerResolver.ProviderSystemName, string storeId = "") => - _links.InsertAsync(new ExternalAuthentication { - CustomerId = customerId, ExternalIdentifier = sub, ProviderSystemName = provider, StoreId = storeId - }); - [TestMethod] public async Task NeverRevoked_NotRevoked() { - await Link("c1", "sub-1"); Assert.IsFalse(await _service.IsRevoked("sub-1", DateTime.UtcNow.AddHours(-1))); } [TestMethod] public async Task Revoke_OlderTokenRevoked_NewerAccepted() { - await Link("c1", "sub-1"); await _service.RevokeTokens("sub-1"); Assert.IsTrue(await _service.IsRevoked("sub-1", DateTime.UtcNow.AddMinutes(-1))); Assert.IsFalse(await _service.IsRevoked("sub-1", DateTime.UtcNow.AddMinutes(1))); + Assert.IsFalse(await _service.IsRevoked("sub-2", DateTime.UtcNow.AddMinutes(-1))); } - //with per-store identity a subject has one link per store: all of them stop the old tokens + //a subject without a link yet (API tokens only) is revoked as well: a link made later does not bring the tokens back [TestMethod] - public async Task Revoke_EveryLinkOfTheSubject_OnlyOpenIdConnect() + public async Task Revoke_KeptApartFromLinks_OneRecordPerSubject() { - await Link("c1", "sub-1", storeId: "s1"); - await Link("c2", "sub-1", storeId: "s2"); - await Link("c3", "sub-1", provider: "ExternalAuth.Google"); - await Link("c4", "sub-2"); + await _service.RevokeTokens("sub-1"); await _service.RevokeTokens("sub-1"); - var revoked = _links.Table.Where(x => x.TokensRevokedBeforeUtc != null).Select(x => x.CustomerId).ToList(); - CollectionAssert.AreEquivalent(new[] { "c1", "c2" }, revoked); + Assert.AreEqual(1, _revocations.Table.Count(x => x.Subject == "sub-1")); + Assert.AreEqual(OidcTokenRevocationService.RevocationId("sub-1"), _revocations.Table.Single().Id); } [TestMethod] - public async Task UnknownSubject_NotRevoked_RevokeDoesNothing() + public async Task RevokeAgain_MovesTheTimeForward() { - await _service.RevokeTokens("nobody"); - Assert.IsFalse(await _service.IsRevoked("nobody", DateTime.UtcNow.AddHours(-1))); + var id = OidcTokenRevocationService.RevocationId("sub-1"); + await _revocations.InsertAsync(new OidcTokenRevocation { + Id = id, Subject = "sub-1", RevokedBeforeUtc = DateTime.UtcNow.AddHours(-2) + }); + Assert.IsFalse(await _service.IsRevoked("sub-1", DateTime.UtcNow.AddHours(-1))); + + await _service.RevokeTokens("sub-1"); + + Assert.IsTrue(await _service.IsRevoked("sub-1", DateTime.UtcNow.AddHours(-1))); + } + + //subjects are case-sensitive + [TestMethod] + public async Task Revoke_SubjectCaseMatters() + { + await _service.RevokeTokens("Sub-1"); + Assert.IsFalse(await _service.IsRevoked("sub-1", DateTime.UtcNow.AddMinutes(-1))); + } + + [TestMethod] + public async Task DeleteExpired_KeepsRecentRevocations() + { + await _revocations.InsertAsync(new OidcTokenRevocation { + Id = OidcTokenRevocationService.RevocationId("old"), Subject = "old", + RevokedBeforeUtc = DateTime.UtcNow - OidcSessionDefaults.RevocationRetention - TimeSpan.FromHours(1) + }); + await _service.RevokeTokens("recent"); + + await _service.DeleteExpired(); + + CollectionAssert.AreEqual(new[] { "recent" }, _revocations.Table.Select(x => x.Subject).ToList()); } } diff --git a/src/Tests/Grand.Modules.Tests/Plugins/OpenIdConnect/OidcBackChannelLogoutEndpointTests.cs b/src/Tests/Grand.Modules.Tests/Plugins/OpenIdConnect/OidcBackChannelLogoutEndpointTests.cs index e82326130d..f68377397b 100644 --- a/src/Tests/Grand.Modules.Tests/Plugins/OpenIdConnect/OidcBackChannelLogoutEndpointTests.cs +++ b/src/Tests/Grand.Modules.Tests/Plugins/OpenIdConnect/OidcBackChannelLogoutEndpointTests.cs @@ -28,6 +28,7 @@ public class OidcBackChannelLogoutEndpointTests private static readonly RsaSecurityKey Key = NewKey(); private Mock _sessions; private Mock _revocation; + private Mock _links; private OpenIdConnectOptions _options; private bool _schemeRegistered; @@ -44,6 +45,8 @@ public void Init() _sessions.Setup(x => x.EndBySid(It.IsAny(), It.IsAny())).ReturnsAsync([]); _sessions.Setup(x => x.EndBySubject(It.IsAny())).ReturnsAsync([]); _revocation = new Mock(); + _links = new Mock(); + _links.Setup(x => x.LinksMatchProvider()).ReturnsAsync(true); var configuration = new OpenIdConnectConfiguration { Issuer = Issuer }; configuration.SigningKeys.Add(Key); _options = new OpenIdConnectOptions { @@ -80,6 +83,7 @@ private IServiceProvider Services() .AddSingleton(new OidcLogoutTokenValidator(new MemoryCache(new MemoryCacheOptions()))) .AddSingleton(_sessions.Object) .AddSingleton(_revocation.Object) + .AddSingleton(_links.Object) .AddSingleton(new OidcLoginConfig { ClientId = "shop" }) .AddSingleton(new OidcResourceServerConfig { ClockSkewSeconds = 60 }) .AddSingleton(NullLoggerFactory.Instance) @@ -121,6 +125,19 @@ public async Task SidOnly_RevokesSubjectsOfEndedSessions() _revocation.Verify(x => x.RevokeTokens("sub-2"), Times.Once); } + //the sessions belong to the previous provider: its subject may be another person - the tokens of this subject (of + //the configured provider) are still refused + [TestMethod] + public async Task ProviderChanged_EndsNoSession_RevokesSubject() + { + _links.Setup(x => x.LinksMatchProvider()).ReturnsAsync(false); + var context = await Post(Token()); + Assert.AreEqual(200, context.Response.StatusCode); + _sessions.Verify(x => x.EndBySid(It.IsAny(), It.IsAny()), Times.Never); + _sessions.Verify(x => x.EndBySubject(It.IsAny()), Times.Never); + _revocation.Verify(x => x.RevokeTokens("sub-1"), Times.Once); + } + [TestMethod] public async Task SubOnly_EndsAllSessionsOfSubject() { diff --git a/src/Tests/Grand.Modules.Tests/Services/BackgroundService/DeleteOidcSessionsScheduleTaskTests.cs b/src/Tests/Grand.Modules.Tests/Services/BackgroundService/DeleteOidcSessionsScheduleTaskTests.cs index 951817977c..bcab0b408a 100644 --- a/src/Tests/Grand.Modules.Tests/Services/BackgroundService/DeleteOidcSessionsScheduleTaskTests.cs +++ b/src/Tests/Grand.Modules.Tests/Services/BackgroundService/DeleteOidcSessionsScheduleTaskTests.cs @@ -9,10 +9,12 @@ namespace Grand.Modules.Tests.Services.BackgroundService; public class DeleteOidcSessionsScheduleTaskTests { [TestMethod] - public async Task Execute_DeletesExpiredSessions() + public async Task Execute_DeletesExpiredSessionsAndRevocations() { var sessions = new Mock(); - await new DeleteOidcSessionsScheduleTask(sessions.Object).Execute(); + var revocations = new Mock(); + await new DeleteOidcSessionsScheduleTask(sessions.Object, revocations.Object).Execute(); sessions.Verify(x => x.DeleteExpired(), Times.Once); + revocations.Verify(x => x.DeleteExpired(), Times.Once); } } diff --git a/src/Tests/Grand.Web.Admin.Tests/Controllers/BaseCustomerControllerTests.cs b/src/Tests/Grand.Web.Admin.Tests/Controllers/BaseCustomerControllerTests.cs index cda632b193..7e991cee59 100644 --- a/src/Tests/Grand.Web.Admin.Tests/Controllers/BaseCustomerControllerTests.cs +++ b/src/Tests/Grand.Web.Admin.Tests/Controllers/BaseCustomerControllerTests.cs @@ -669,4 +669,31 @@ public async Task OutOfStockSubscriptionList_GlobalScope_NoOwnershipCheck() Assert.IsInstanceOfType(result, typeof(JsonResult)); CustomerServiceMock.Verify(s => s.GetCustomerById(It.IsAny()), Times.Never); } + + [TestMethod] + public async Task ExternalAuthenticationDelete_CustomerInScope_RemovesTheLink() + { + var customer = new Customer { Id = "c1" }; + CustomerServiceMock.Setup(s => s.GetCustomerById("c1")).ReturnsAsync(customer); + ScopeMock.Setup(s => s.HasAccess(customer)).ReturnsAsync(true); + var links = new Mock(); + links.Setup(l => l.Remove(customer, "link-1")).ReturnsAsync(true); + + var result = await Controller.ExternalAuthenticationDelete("link-1", "c1", links.Object); + + Assert.IsInstanceOfType(result, typeof(JsonResult)); + links.Verify(l => l.Remove(customer, "link-1"), Times.Once); + } + + [TestMethod] + public async Task ExternalAuthenticationDelete_CustomerOutOfScope_Throws_NothingRemoved() + { + var customer = new Customer { Id = "c1" }; + CustomerServiceMock.Setup(s => s.GetCustomerById("c1")).ReturnsAsync(customer); + ScopeMock.Setup(s => s.HasAccess(customer)).ReturnsAsync(false); + var links = new Mock(); + + await Assert.ThrowsAsync(() => Controller.ExternalAuthenticationDelete("link-1", "c1", links.Object)); + links.Verify(l => l.Remove(It.IsAny(), It.IsAny()), Times.Never); + } } diff --git a/src/Web/Grand.Web.Admin/Areas/Admin/Views/ExternalAuthentication/Methods.cshtml b/src/Web/Grand.Web.Admin/Areas/Admin/Views/ExternalAuthentication/Methods.cshtml index 9781c965f2..08d01c76d0 100644 --- a/src/Web/Grand.Web.Admin/Areas/Admin/Views/ExternalAuthentication/Methods.cshtml +++ b/src/Web/Grand.Web.Admin/Areas/Admin/Views/ExternalAuthentication/Methods.cshtml @@ -1,4 +1,5 @@ -@{ +@model OidcLinksModel +@{ //page title ViewBag.Title = Loc["Admin.Plugins.ExternalAuthenticationMethods"]; } @@ -36,4 +37,46 @@ + + @* the OpenID Connect account links belong to one identity provider: a subject is unique only there *@ + @if (Model != null) + { + + @if (Model.ProviderUnknown) + { +
+ @string.Format(Loc["Admin.Plugins.ExternalAuthenticationMethods.OidcLinks.ProviderUnknown"].Value, + Model.ConfiguredAuthority) +
+ } + else if (Model.ProviderChanged) + { +
+ @string.Format(Loc["Admin.Plugins.ExternalAuthenticationMethods.OidcLinks.ProviderChanged"].Value, + Model.LinkedAuthority, Model.ConfiguredAuthority) +
+ } +

+ @string.Format(Loc["Admin.Plugins.ExternalAuthenticationMethods.OidcLinks.Info"].Value, + Model.ConfiguredAuthority, Model.LinkCount) +

+
+ + @if (Model.ProviderChanged) + { + + } +
+ + } diff --git a/src/Web/Grand.Web.Admin/Controllers/ExternalAuthenticationController.cs b/src/Web/Grand.Web.Admin/Controllers/ExternalAuthenticationController.cs index 66de5cb3ed..d8f6f20202 100644 --- a/src/Web/Grand.Web.Admin/Controllers/ExternalAuthenticationController.cs +++ b/src/Web/Grand.Web.Admin/Controllers/ExternalAuthenticationController.cs @@ -3,6 +3,8 @@ using Grand.Business.Core.Interfaces.Common.Configuration; using Grand.Domain.Permissions; using Grand.Domain.Customers; +using Grand.Infrastructure.Configuration; +using Grand.Business.Core.Interfaces.Common.Localization; using Grand.Infrastructure.Plugins; using Grand.Web.AdminShared.Extensions.Mapping; using Grand.Web.AdminShared.Models.ExternalAuthentication; @@ -41,11 +43,56 @@ public ExternalAuthenticationController(IExternalAuthenticationService openAuthe #region Methods - public IActionResult Methods() + public async Task Methods([FromServices] OidcResourceServerConfig resourceServerConfig, + [FromServices] IOidcLinkService linkService) { - return View(); + //the account links of the OpenID Connect provider, only while it is configured + if (!ResourceServerConfigured(resourceServerConfig)) + return View(); + + var state = await linkService.GetState(); + return View(new OidcLinksModel { + ConfiguredAuthority = state.ConfiguredAuthority, + LinkedAuthority = state.LinkedAuthority, + LinkCount = state.LinkCount, + ProviderChanged = state.ProviderChanged, + ProviderUnknown = state.ProviderUnknown + }); } + /// Another identity provider: every OpenID Connect link is removed, people connect again at their next sign-in + [HttpPost] + [AutoValidateAntiforgeryToken] + public async Task RemoveOidcLinks([FromServices] IOidcLinkService linkService, + [FromServices] ITranslationService translationService, [FromServices] OidcResourceServerConfig resourceServerConfig) + { + //the links are recorded for the configured provider: without one there is nothing to record them for + if (!ResourceServerConfigured(resourceServerConfig)) + return RedirectToAction("Methods"); + + var count = await linkService.RemoveAll(); + Success(string.Format(translationService.GetResource("Admin.Plugins.ExternalAuthenticationMethods.OidcLinks.Removed"), + count)); + return RedirectToAction("Methods"); + } + + /// The same identity provider at a new address: the links are kept for it + [HttpPost] + [AutoValidateAntiforgeryToken] + public async Task KeepOidcLinks([FromServices] IOidcLinkService linkService, + [FromServices] ITranslationService translationService, [FromServices] OidcResourceServerConfig resourceServerConfig) + { + if (!ResourceServerConfigured(resourceServerConfig)) + return RedirectToAction("Methods"); + + await linkService.KeepAll(); + Success(translationService.GetResource("Admin.Plugins.ExternalAuthenticationMethods.OidcLinks.Kept")); + return RedirectToAction("Methods"); + } + + private static bool ResourceServerConfigured(OidcResourceServerConfig config) => + config.Enabled && !string.IsNullOrWhiteSpace(config.Authority); + [HttpPost] public IActionResult Methods(DataSourceRequest command) { diff --git a/src/Web/Grand.Web.AdminShared/Controllers/BaseCustomerController.cs b/src/Web/Grand.Web.AdminShared/Controllers/BaseCustomerController.cs index 4ece7e829c..3e43ec32c9 100644 --- a/src/Web/Grand.Web.AdminShared/Controllers/BaseCustomerController.cs +++ b/src/Web/Grand.Web.AdminShared/Controllers/BaseCustomerController.cs @@ -1,5 +1,6 @@ #nullable enable +using Grand.Business.Core.Interfaces.Authentication; using Grand.Business.Core.Interfaces.Catalog.Products; using Grand.Business.Core.Interfaces.Common.Addresses; using Grand.Business.Core.Interfaces.Common.Directory; @@ -465,6 +466,23 @@ public async Task AddressesSelect(string customerId, DataSourceRe return Json(gridModel); } + /// + /// Removes a link to an external sign-in (e.g. after the person was deleted and created again at the identity + /// provider); the next provider sign-in connects the account again by the rules of a first sign-in + /// + [PermissionAuthorizeAction(PermissionActionName.Edit)] + [HttpPost] + public async Task ExternalAuthenticationDelete(string id, string customerId, + [FromServices] IOidcLinkService linkService) + { + var (customer, _) = await LoadAuthorizedCustomer(customerId); + if (customer is null) throw new ArgumentException("No customer found with the specified id", nameof(customerId)); + + if (!await linkService.Remove(customer, id)) + return ErrorForKendoGridJson("No external sign-in found with the specified id"); + return new JsonResult(""); + } + [PermissionAuthorizeAction(PermissionActionName.Edit)] [HttpPost] public async Task AddressDelete(string id, string customerId) diff --git a/src/Web/Grand.Web.AdminShared/Models/ExternalAuthentication/OidcLinksModel.cs b/src/Web/Grand.Web.AdminShared/Models/ExternalAuthentication/OidcLinksModel.cs new file mode 100644 index 0000000000..4aa57d70bf --- /dev/null +++ b/src/Web/Grand.Web.AdminShared/Models/ExternalAuthentication/OidcLinksModel.cs @@ -0,0 +1,11 @@ +namespace Grand.Web.AdminShared.Models.ExternalAuthentication; + +/// The stored OpenID Connect account links against the configured identity provider (resource server enabled) +public class OidcLinksModel +{ + public string ConfiguredAuthority { get; set; } + public string LinkedAuthority { get; set; } + public long LinkCount { get; set; } + public bool ProviderChanged { get; set; } + public bool ProviderUnknown { get; set; } +} diff --git a/src/Web/Grand.Web.AdminShared/Services/CustomerViewModelService.cs b/src/Web/Grand.Web.AdminShared/Services/CustomerViewModelService.cs index ae9f0f111d..6f39b69a61 100644 --- a/src/Web/Grand.Web.AdminShared/Services/CustomerViewModelService.cs +++ b/src/Web/Grand.Web.AdminShared/Services/CustomerViewModelService.cs @@ -33,6 +33,7 @@ using Grand.Web.Common.Extensions; using Grand.Web.Common.Localization; using Grand.Web.Common.Models; +using Grand.Web.Common.Security.Oidc; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc.Rendering; using Microsoft.Extensions.DependencyInjection; @@ -1262,14 +1263,17 @@ protected virtual string GetCustomerGroupsNames(IList customerGro foreach (var record in await openAuthenticationService.GetExternalIdentifiers(customer)) { var method = openAuthenticationService.LoadAuthenticationProviderBySystemName(record.ProviderSystemName); - if (method == null) + //the resource server makes OpenID Connect links without the sign-in plugin: they are listed (and can be + //removed) all the same + var isOidcLink = record.ProviderSystemName == OidcResourceServerDefaults.ProviderSystemName; + if (method == null && !isOidcLink) continue; result.Add(new CustomerModel.AssociatedExternalAuthModel { Id = record.Id, Email = record.Email, ExternalIdentifier = record.ExternalIdentifier, - AuthMethodName = method.FriendlyName + AuthMethodName = method?.FriendlyName ?? "OpenID Connect" }); } diff --git a/src/Web/Grand.Web.AdminShared/Views/AdminShared/Customer/Partials/CreateOrUpdate.TabInfo.cshtml b/src/Web/Grand.Web.AdminShared/Views/AdminShared/Customer/Partials/CreateOrUpdate.TabInfo.cshtml index ee8d4bae1e..513a1d317b 100644 --- a/src/Web/Grand.Web.AdminShared/Views/AdminShared/Customer/Partials/CreateOrUpdate.TabInfo.cshtml +++ b/src/Web/Grand.Web.AdminShared/Views/AdminShared/Customer/Partials/CreateOrUpdate.TabInfo.cshtml @@ -74,11 +74,15 @@ {
+ @* removing a link lets the person connect again, e.g. after being created anew at the identity provider *@ + data="Model.AssociatedExternalAuthRecords.Select(x => new { x.Id, x.AuthMethodName, x.Email, x.ExternalIdentifier })" + destroy-url="@Url.Action("ExternalAuthenticationDelete", "Customer", new { customerId = Model.Id, area = ViewContext.RouteData.Values["area"] })" + confirm-destroy="true"> +
diff --git a/src/Web/Grand.Web/App_Data/Resources/DefaultLanguage.xml b/src/Web/Grand.Web/App_Data/Resources/DefaultLanguage.xml index 17a1362638..6ca35bf38a 100644 Binary files a/src/Web/Grand.Web/App_Data/Resources/DefaultLanguage.xml and b/src/Web/Grand.Web/App_Data/Resources/DefaultLanguage.xml differ diff --git a/src/Web/Grand.Web/App_Data/Resources/Upgrade/en_250.xml b/src/Web/Grand.Web/App_Data/Resources/Upgrade/en_250.xml index 255679933f..0d79b1a7da 100644 --- a/src/Web/Grand.Web/App_Data/Resources/Upgrade/en_250.xml +++ b/src/Web/Grand.Web/App_Data/Resources/Upgrade/en_250.xml @@ -30,4 +30,34 @@ Sign in with {0} + + OpenID Connect account links + + + Identity provider: {0}. Accounts connected to it: {1}. + + + The identity provider changed from {0} to {1}. Sign-in through the provider and its access tokens are refused until you decide: remove the links when this is another provider (people connect again at their next sign-in), or keep them when it is the same provider at a new address. + + + The OpenID Connect links were made before the store recorded their identity provider, so it cannot tell whether {0} is the same provider. Sign-in through the provider and its access tokens are refused until you decide: keep the links when they were made with {0}, or remove them (people connect again at their next sign-in). + + + Remove all links + + + Every account loses its connection to the identity provider and every session started there ends. People connect again at their next sign-in, by the rules of a first sign-in. Continue? + + + {0} account links removed. + + + Keep the links (same provider) + + + Keep the links only when the new address belongs to the same provider, with the same users. A user of another provider could otherwise sign in to someone else's account. Continue? + + + The account links now belong to the configured identity provider. +