Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

V14: Move towards get guid #15889

Merged
merged 22 commits into from Apr 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -253,7 +253,6 @@ private async Task<IActionResult> AuthorizeExternal(OpenIddictRequest request)
private async Task<IActionResult> SignInBackOfficeUser(BackOfficeIdentityUser backOfficeUser, OpenIddictRequest request)
{
ClaimsPrincipal backOfficePrincipal = await _backOfficeSignInManager.CreateUserPrincipalAsync(backOfficeUser);
backOfficePrincipal.SetClaim(OpenIddictConstants.Claims.Subject, backOfficeUser.Key.ToString());

Claim[] backOfficeClaims = backOfficePrincipal.Claims.ToArray();
foreach (Claim backOfficeClaim in backOfficeClaims)
Expand Down
Expand Up @@ -33,7 +33,7 @@ public class SetTelemetryController : TelemetryControllerBase
return BadRequest(invalidModelProblem);
}

_metricsConsentService.SetConsentLevel(telemetryRepresentationBase.TelemetryLevel);
await _metricsConsentService.SetConsentLevelAsync(telemetryRepresentationBase.TelemetryLevel);
return await Task.FromResult(Ok());
}
}
Expand Up @@ -17,14 +17,16 @@
private readonly MediaFileManager _mediaFileManager;
private readonly IImageUrlGenerator _imageUrlGenerator;
private readonly IEntityService _entityService;
private readonly IUserIdKeyResolver _userIdKeyResolver;

public AuditLogPresentationFactory(IUserService userService, AppCaches appCaches, MediaFileManager mediaFileManager, IImageUrlGenerator imageUrlGenerator, IEntityService entityService)
public AuditLogPresentationFactory(IUserService userService, AppCaches appCaches, MediaFileManager mediaFileManager, IImageUrlGenerator imageUrlGenerator, IEntityService entityService, IUserIdKeyResolver userIdKeyResolver)
{
_userService = userService;
_appCaches = appCaches;
_mediaFileManager = mediaFileManager;
_imageUrlGenerator = imageUrlGenerator;
_entityService = entityService;
_userIdKeyResolver = userIdKeyResolver;

Check warning on line 29 in src/Umbraco.Cms.Api.Management/Factories/AuditLogPresentationFactory.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (v14/dev)

❌ New issue: Constructor Over-Injection

AuditLogPresentationFactory has 6 arguments, threshold = 5. This constructor has too many arguments, indicating an object with low cohesion or missing function argument abstraction. Avoid adding more arguments.
}

public IEnumerable<AuditLogResponseModel> CreateAuditLogViewModel(IEnumerable<IAuditItem> auditItems) => auditItems.Select(CreateAuditLogViewModel);
Expand All @@ -46,7 +48,8 @@
private T CreateResponseModel<T>(IAuditItem auditItem, out IUser user)
where T : AuditLogBaseModel, new()
{
user = _userService.GetUserById(auditItem.UserId)
Guid userKey = _userIdKeyResolver.GetAsync(auditItem.UserId).GetAwaiter().GetResult();
user = _userService.GetAsync(userKey).GetAwaiter().GetResult()
?? throw new ArgumentException($"Could not find user with id {auditItem.UserId}");

IEntitySlim? entitySlim = _entityService.Get(auditItem.Id);
Expand Down
Expand Up @@ -52,7 +52,7 @@ public async Task HandleAsync(UnattendedInstallNotification notification, Cancel
return;
}

IUser? admin = _userService.GetUserById(Constants.Security.SuperUserId);
IUser? admin = await _userService.GetAsync(Constants.Security.SuperUserKey);
if (admin == null)
{
throw new InvalidOperationException("Could not find the super user!");
Expand Down
8 changes: 7 additions & 1 deletion src/Umbraco.Core/Cache/DistributedCacheExtensions.cs
Expand Up @@ -32,7 +32,13 @@ public static void RefreshUserCache(this DistributedCache dc, int userId)
=> dc.Refresh(UserCacheRefresher.UniqueId, userId);

public static void RefreshUserCache(this DistributedCache dc, IEnumerable<IUser> users)
=> dc.Refresh(UserCacheRefresher.UniqueId, users.Select(x => x.Id).Distinct().ToArray());
{
foreach (IUser user in users)
{
dc.Refresh(UserCacheRefresher.UniqueId, user.Key);
dc.Refresh(UserCacheRefresher.UniqueId, user.Id);
}
}

public static void RefreshAllUserCache(this DistributedCache dc)
=> dc.RefreshAll(UserCacheRefresher.UniqueId);
Expand Down
Expand Up @@ -30,25 +30,19 @@ public override void RefreshAll()
base.RefreshAll();
}

public override void Refresh(int id)
{
Remove(id);
base.Refresh(id);
}

public override void Remove(int id)
public override void Refresh(Guid id)
Zeegaan marked this conversation as resolved.
Show resolved Hide resolved
{
Attempt<IAppPolicyCache?> userCache = AppCaches.IsolatedCaches.Get<IUser>();
if (userCache.Success)
{
userCache.Result?.Clear(RepositoryCacheKeys.GetKey<IUser, int>(id));
userCache.Result?.Clear(RepositoryCacheKeys.GetKey<IUser, Guid>(id));
userCache.Result?.ClearByKey(CacheKeys.UserContentStartNodePathsPrefix + id);
userCache.Result?.ClearByKey(CacheKeys.UserMediaStartNodePathsPrefix + id);
userCache.Result?.ClearByKey(CacheKeys.UserAllContentStartNodesPrefix + id);
userCache.Result?.ClearByKey(CacheKeys.UserAllMediaStartNodesPrefix + id);
}

base.Remove(id);
base.Refresh(id);
}

#endregion
Expand Down
5 changes: 5 additions & 0 deletions src/Umbraco.Core/Constants-Security.cs
Expand Up @@ -112,6 +112,11 @@ public static class Security
/// </summary>
public const string SecurityStampClaimType = "AspNet.Identity.SecurityStamp";

/// <summary>
/// The claim type for the mandatory OpenIdDict sub claim
/// </summary>
public const string OpenIdDictSubClaimType = "sub";

public const string AspNetCoreV3PasswordHashAlgorithmName = "PBKDF2.ASPNETCORE.V3";
public const string AspNetCoreV2PasswordHashAlgorithmName = "PBKDF2.ASPNETCORE.V2";
public const string AspNetUmbraco8PasswordHashAlgorithmName = "HMACSHA256";
Expand Down
2 changes: 1 addition & 1 deletion src/Umbraco.Core/Events/UserNotificationsHandler.cs
Expand Up @@ -197,7 +197,7 @@ public void Notify(IAction? action, params IContent[] entities)
_logger.LogDebug(
"There is no current Umbraco user logged in, the notifications will be sent from the administrator");
}
user = _userService.GetUserById(Constants.Security.SuperUserId);
user = _userService.GetAsync(Constants.Security.SuperUserKey).GetAwaiter().GetResult();
Zeegaan marked this conversation as resolved.
Show resolved Hide resolved
if (user == null)
{
_logger.LogWarning(
Expand Down
15 changes: 14 additions & 1 deletion src/Umbraco.Core/Extensions/ClaimsIdentityExtensions.cs
Expand Up @@ -193,6 +193,7 @@
/// </summary>
/// <param name="identity">this</param>
/// <param name="userId">The users Id</param>
/// <param name="userKey">The users key</param>
/// <param name="username">Username</param>
/// <param name="realName">Real name</param>
/// <param name="startContentNodes">Start content nodes</param>
Expand All @@ -201,20 +202,32 @@
/// <param name="securityStamp">Security stamp</param>
/// <param name="allowedApps">Allowed apps</param>
/// <param name="roles">Roles</param>
public static void AddRequiredClaims(this ClaimsIdentity identity, string userId, string username, string realName, IEnumerable<int>? startContentNodes, IEnumerable<int>? startMediaNodes, string culture, string securityStamp, IEnumerable<string> allowedApps, IEnumerable<string> roles)
public static void AddRequiredClaims(this ClaimsIdentity identity, string userId, Guid userKey, string username, string realName, IEnumerable<int>? startContentNodes, IEnumerable<int>? startMediaNodes, string culture, string securityStamp, IEnumerable<string> allowedApps, IEnumerable<string> roles)
{
// This is the id that 'identity' uses to check for the user id
if (identity.HasClaim(x => x.Type == ClaimTypes.NameIdentifier) == false)
{
identity.AddClaim(new Claim(
ClaimTypes.NameIdentifier,
userId,
ClaimValueTypes.String,
AuthenticationType,
AuthenticationType,
identity));
}

// This is the id that 'identity' uses to check for the user id
if (identity.HasClaim(x => x.Type == Constants.Security.OpenIdDictSubClaimType) == false)
{
identity.AddClaim(new Claim(
Constants.Security.OpenIdDictSubClaimType,
userKey.ToString(),
ClaimValueTypes.String,
AuthenticationType,

Check warning on line 226 in src/Umbraco.Core/Extensions/ClaimsIdentityExtensions.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (v14/dev)

❌ Getting worse: Complex Method

AddRequiredClaims increases in cyclomatic complexity from 18 to 19, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.

Check notice on line 226 in src/Umbraco.Core/Extensions/ClaimsIdentityExtensions.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (v14/dev)

ℹ Getting worse: Excess Number of Function Arguments

AddRequiredClaims increases from 10 to 11 arguments, threshold = 4. This function has too many arguments, indicating a lack of encapsulation. Avoid adding more arguments.
AuthenticationType,
identity));
}

if (identity.HasClaim(x => x.Type == ClaimTypes.Name) == false)
{
identity.AddClaim(new Claim(
Expand Down
2 changes: 1 addition & 1 deletion src/Umbraco.Core/Handlers/AuditNotificationsHandler.cs
Expand Up @@ -82,7 +82,7 @@ private IUser CurrentPerformingUser
get
{
IUser? identity = _backOfficeSecurityAccessor.BackOfficeSecurity?.CurrentUser;
IUser? user = identity == null ? null : _userService.GetUserById(Convert.ToInt32(identity.Id));
IUser? user = identity == null ? null : _userService.GetAsync(identity.Key).GetAwaiter().GetResult();
Zeegaan marked this conversation as resolved.
Show resolved Hide resolved
return user ?? UnknownUser(_globalSettings);
}
}
Expand Down
4 changes: 1 addition & 3 deletions src/Umbraco.Core/Persistence/Repositories/IUserRepository.cs
Expand Up @@ -4,7 +4,7 @@

namespace Umbraco.Cms.Core.Persistence.Repositories;

public interface IUserRepository : IReadWriteQueryRepository<int, IUser>
public interface IUserRepository : IReadWriteQueryRepository<Guid, IUser>
{
/// <summary>
/// Gets the count of items based on a complex query
Expand Down Expand Up @@ -141,6 +141,4 @@ public interface IUserRepository : IReadWriteQueryRepository<int, IUser>
int ClearLoginSessions(TimeSpan timespan);

void ClearLoginSession(Guid sessionId);

IEnumerable<IUser> GetNextUsers(int id, int count);
}
1 change: 1 addition & 0 deletions src/Umbraco.Core/Security/IBackofficeSecurity.cs
Expand Up @@ -21,6 +21,7 @@ public interface IBackOfficeSecurity
/// <returns>The current user's Id that has been authenticated for the request.</returns>
/// <remarks>If authentication hasn't taken place this will be unsuccessful.</remarks>
// TODO: This should just be an extension method on ClaimsIdentity
[Obsolete("Scheduled for removal in V15")]
Attempt<int> GetUserId();

/// <summary>
Expand Down
3 changes: 3 additions & 0 deletions src/Umbraco.Core/Services/IMetricsConsentService.cs
Expand Up @@ -6,5 +6,8 @@ public interface IMetricsConsentService
{
TelemetryLevel GetConsentLevel();

[Obsolete("Please use SetConsentLevelAsync instead, scheduled for removal in V15")]
void SetConsentLevel(TelemetryLevel telemetryLevel);

Task SetConsentLevelAsync(TelemetryLevel telemetryLevel);
}
2 changes: 0 additions & 2 deletions src/Umbraco.Core/Services/IUserService.cs
Expand Up @@ -322,8 +322,6 @@ public interface IUserService : IMembershipUserService
/// </returns>
IEnumerable<IUser> GetAllNotInGroup(int groupId);

IEnumerable<IUser> GetNextUsers(int id, int count);

#region User groups

/// <summary>
Expand Down
15 changes: 11 additions & 4 deletions src/Umbraco.Core/Services/MemberService.cs
@@ -1,4 +1,4 @@
using Microsoft.Extensions.Logging;

Check notice on line 1 in src/Umbraco.Core/Services/MemberService.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (v14/dev)

✅ Getting better: Primitive Obsession

The ratio of primitive types in function arguments decreases from 70.53% to 70.31%, threshold = 30.0%. The functions in this file have too many primitive types (e.g. int, double, float) in their function argument lists. Using many primitive types lead to the code smell Primitive Obsession. Avoid adding more primitive arguments.

Check notice on line 1 in src/Umbraco.Core/Services/MemberService.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (v14/dev)

✅ Getting better: String Heavy Function Arguments

The ratio of strings in function arguments decreases from 45.26% to 44.79%, threshold = 39.0%. The functions in this file have a high ratio of strings as arguments. Avoid adding more.
using Umbraco.Cms.Core.Events;
using Umbraco.Cms.Core.Models;
using Umbraco.Cms.Core.Models.Membership;
Expand All @@ -20,8 +20,8 @@
private readonly IMemberTypeRepository _memberTypeRepository;
private readonly IMemberGroupRepository _memberGroupRepository;
private readonly IAuditRepository _auditRepository;

private readonly IMemberGroupService _memberGroupService;
private readonly Lazy<IIdKeyMap> _idKeyMap;
bergmania marked this conversation as resolved.
Show resolved Hide resolved

#region Constructor

Expand All @@ -33,13 +33,15 @@
IMemberRepository memberRepository,
IMemberTypeRepository memberTypeRepository,
IMemberGroupRepository memberGroupRepository,
IAuditRepository auditRepository)
IAuditRepository auditRepository,
Lazy<IIdKeyMap> idKeyMap)
: base(provider, loggerFactory, eventMessagesFactory)
{
_memberRepository = memberRepository;
_memberTypeRepository = memberTypeRepository;
_memberGroupRepository = memberGroupRepository;
_auditRepository = auditRepository;
_idKeyMap = idKeyMap;

Check notice on line 44 in src/Umbraco.Core/Services/MemberService.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (v14/dev)

ℹ Getting worse: Constructor Over-Injection

MemberService increases from 8 to 9 arguments, threshold = 5. This constructor has too many arguments, indicating an object with low cohesion or missing function argument abstraction. Avoid adding more arguments.
_memberGroupService = memberGroupService ?? throw new ArgumentNullException(nameof(memberGroupService));
}

Expand Down Expand Up @@ -333,8 +335,7 @@
{
using ICoreScope scope = ScopeProvider.CreateCoreScope(autoComplete: true);
scope.ReadLock(Constants.Locks.MemberTree);
IQuery<IMember> query = Query<IMember>().Where(x => x.Key == id);
return _memberRepository.Get(query)?.FirstOrDefault();
return GetMemberFromRepository(id);
}

[Obsolete($"Use {nameof(GetById)}. Will be removed in V15.")]
Expand Down Expand Up @@ -1069,6 +1070,12 @@

private void Audit(AuditType type, int userId, int objectId, string? message = null) => _auditRepository.Save(new AuditItem(objectId, type, userId, ObjectTypes.GetName(UmbracoObjectTypes.Member), message));

private IMember? GetMemberFromRepository(Guid id)
=> _idKeyMap.Value.GetIdForKey(id, UmbracoObjectTypes.Member) switch
{
{ Success: false } => null,
{ Result: var intId } => _memberRepository.Get(intId),
};
#endregion

#region Membership
Expand Down
11 changes: 5 additions & 6 deletions src/Umbraco.Core/Services/MetricsConsentService.cs
Expand Up @@ -39,13 +39,12 @@ public TelemetryLevel GetConsentLevel()
return analyticsLevel;
}

public void SetConsentLevel(TelemetryLevel telemetryLevel)
[Obsolete("Please use SetConsentLevelAsync instead, scheduled for removal in V15")]
public void SetConsentLevel(TelemetryLevel telemetryLevel) => SetConsentLevelAsync(telemetryLevel).GetAwaiter().GetResult();

public async Task SetConsentLevelAsync(TelemetryLevel telemetryLevel)
{
IUser? currentUser = _backOfficeSecurityAccessor.BackOfficeSecurity?.CurrentUser;
if (currentUser is null)
{
currentUser = _userService.GetUserById(Constants.Security.SuperUserId);
}
IUser? currentUser = _backOfficeSecurityAccessor.BackOfficeSecurity?.CurrentUser ?? await _userService.GetAsync(Constants.Security.SuperUserKey);

_logger.LogInformation("Telemetry level set to {telemetryLevel} by {username}", telemetryLevel, currentUser?.Username);
_keyValueService.SetValue(Key, telemetryLevel.ToString());
Expand Down
65 changes: 27 additions & 38 deletions src/Umbraco.Core/Services/NotificationService.cs
Expand Up @@ -94,52 +94,41 @@
// lazily get versions
var prevVersionDictionary = new Dictionary<int, IContentBase?>();

// see notes above
var id = Constants.Security.SuperUserId;
const int pagesz = 400; // load batches of 400 users
do
var notifications = GetUsersNotifications(new List<int>(), action, Enumerable.Empty<int>(), Constants.ObjectTypes.Document)?.ToList();
if (notifications is null || notifications.Count == 0)
{
var notifications = GetUsersNotifications(new List<int>(), action, Enumerable.Empty<int>(), Constants.ObjectTypes.Document)?.ToList();
if (notifications is null || notifications.Count == 0)
{
break;
}
return;
}

// users are returned ordered by id, notifications are returned ordered by user id
var users = _userService.GetNextUsers(id, pagesz).Where(x => x.IsApproved).ToList();
foreach (IUser user in users)
IUser[] users = _userService.GetAll(0, int.MaxValue, out _).ToArray();
foreach (IUser user in users)
{
Notification[] userNotifications = notifications.Where(n => n.UserId == user.Id).ToArray();
foreach (Notification notification in userNotifications)
{
Notification[] userNotifications = notifications.Where(n => n.UserId == user.Id).ToArray();
foreach (Notification notification in userNotifications)
// notifications are inherited down the tree - find the topmost entity
// relevant to this notification (entity list is sorted by path)
IContent? entityForNotification = entitiesL
.FirstOrDefault(entity =>
pathsByEntityId.TryGetValue(entity.Id, out var path) &&
path.Contains(notification.EntityId));

if (entityForNotification == null)
{
// notifications are inherited down the tree - find the topmost entity
// relevant to this notification (entity list is sorted by path)
IContent? entityForNotification = entitiesL
.FirstOrDefault(entity =>
pathsByEntityId.TryGetValue(entity.Id, out var path) &&
path.Contains(notification.EntityId));

if (entityForNotification == null)
{
continue;
}

if (prevVersionDictionary.ContainsKey(entityForNotification.Id) == false)
{
prevVersionDictionary[entityForNotification.Id] = GetPreviousVersion(entityForNotification.Id);
}
continue;
}

// queue notification
NotificationRequest req = CreateNotificationRequest(operatingUser, user, entityForNotification, prevVersionDictionary[entityForNotification.Id], actionName, siteUri, createSubject, createBody);
Enqueue(req);
break;
if (prevVersionDictionary.ContainsKey(entityForNotification.Id) == false)
{
prevVersionDictionary[entityForNotification.Id] = GetPreviousVersion(entityForNotification.Id);
}
}

// load more users if any
id = users.Count == pagesz ? users.Last().Id + 1 : -1;
// queue notification
NotificationRequest req = CreateNotificationRequest(operatingUser, user, entityForNotification, prevVersionDictionary[entityForNotification.Id], actionName, siteUri, createSubject, createBody);
Enqueue(req);
break;
}
}

Check notice on line 131 in src/Umbraco.Core/Services/NotificationService.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (v14/dev)

✅ Getting better: Complex Method

SendNotifications decreases in cyclomatic complexity from 12 to 10, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.

Check notice on line 131 in src/Umbraco.Core/Services/NotificationService.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (v14/dev)

✅ No longer an issue: Bumpy Road Ahead

SendNotifications is no longer above the threshold for logical blocks with deeply nested code. The Bumpy Road code smell is a function that contains multiple chunks of nested conditional logic. The deeper the nesting and the more bumps, the lower the code health.

Check notice on line 131 in src/Umbraco.Core/Services/NotificationService.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (v14/dev)

✅ No longer an issue: Deep, Nested Complexity

SendNotifications is no longer above the threshold for nested complexity depth. This function contains deeply nested logic such as if statements and/or loops. The deeper the nesting, the lower the code health.
while (id > 0);
}

/// <summary>
Expand Down