Public Access: Honour custom IMemberGroupService in backoffice dialog (closes #22580)

[AndyButland]

Description

Issue #22580 describes a situation where a custom IMemberGroupService has been created, and notes that the public access dialog doesn't use this.

This is because of two issues:

• PublicAccessPresentationFactory: reads from IMemberRoleManager.Roles + IEntityService.GetAll(...) to get the list of roles to pick from
• ItemMemberGroupItemController (GET /umbraco/management/api/v1/item/member-group): uses IEntityService.GetAll(UmbracoObjectTypes.MemberGroup, ...) to get the picked items.

Both of these now use IMemberGroupService where a new method allowing bulk lookup by keys has been introduced.

Known gap

The member-group tree controllers under src/Umbraco.Cms.Api.Management/Controllers/MemberGroup/Tree/ still go through IEntityService via the shared NamedEntityTreeControllerBase. Rewiring that path requires a larger change to shared tree infrastructure, and would also break consistency with the shared entity service use of other trees, so hasn't been tackled here.

It's likely reasonable in that if someone does have their own implementation of the member services, they aren't looking to manage members via the backoffice anyway.

Testing

Automated

New tests have been added to verify PublicAccessPresentationFactory and MemberGroupService.

Manual

Using a custom implementation of IMemberGroupService verify that the public access dialog works as expected.

FixedMemberGroupService .cs

using Umbraco.Cms.Core;
using Umbraco.Cms.Core.Composing;
using Umbraco.Cms.Core.Models;
using Umbraco.Cms.Core.Services;
using Umbraco.Cms.Core.Services.OperationStatus;

namespace Umbraco.Cms.Web.UI.Custom.MemberGroups;

public sealed class FixedMemberGroupService : IMemberGroupService
{
    private static readonly IReadOnlyList<IMemberGroup> Groups =
    [
        CreateGroup(9001, Guid.Parse("11111111-1111-1111-1111-111111111111"), "External Readers"),
        CreateGroup(9002, Guid.Parse("22222222-2222-2222-2222-222222222222"), "External Editors"),
        CreateGroup(9003, Guid.Parse("33333333-3333-3333-3333-333333333333"), "External VIPs"),
    ];

    public IEnumerable<IMemberGroup> GetAll() => Groups;

    public IMemberGroup? GetById(int id) => Groups.FirstOrDefault(x => x.Id == id);

    public IMemberGroup? GetById(Guid id) => Groups.FirstOrDefault(x => x.Key == id);

    public IEnumerable<IMemberGroup> GetByIds(IEnumerable<int> ids)
    {
        var set = ids.ToHashSet();
        return Groups.Where(x => set.Contains(x.Id));
    }

    public IMemberGroup? GetByName(string? name) =>
        name is null ? null : Groups.FirstOrDefault(x => string.Equals(x.Name, name, StringComparison.OrdinalIgnoreCase));

    public Task<IMemberGroup?> GetByNameAsync(string name) => Task.FromResult(GetByName(name));

    public Task<IMemberGroup?> GetAsync(Guid key) => Task.FromResult(GetById(key));

    public Task<IEnumerable<IMemberGroup>> GetAllAsync() => Task.FromResult<IEnumerable<IMemberGroup>>(Groups);

    public Task<IEnumerable<IMemberGroup>> GetByIdsAsync(IEnumerable<int> ids) => Task.FromResult(GetByIds(ids));

    public void Save(IMemberGroup memberGroup) => throw new NotSupportedException("FixedMemberGroupService is read-only.");

    public void Delete(IMemberGroup memberGroup) => throw new NotSupportedException("FixedMemberGroupService is read-only.");

    public Task<Attempt<IMemberGroup?, MemberGroupOperationStatus>> CreateAsync(IMemberGroup memberGroup) =>
        throw new NotSupportedException("FixedMemberGroupService is read-only.");

    public Task<Attempt<IMemberGroup?, MemberGroupOperationStatus>> DeleteAsync(Guid key) =>
        throw new NotSupportedException("FixedMemberGroupService is read-only.");

    public Task<Attempt<IMemberGroup?, MemberGroupOperationStatus>> UpdateAsync(IMemberGroup memberGroup) =>
        throw new NotSupportedException("FixedMemberGroupService is read-only.");

    private static MemberGroup CreateGroup(int id, Guid key, string name) =>
        new() { Id = id, Key = key, Name = name };
}

public sealed class FixedMemberGroupServiceComposer : IComposer
{
    public void Compose(IUmbracoBuilder builder)
        => builder.Services.AddUnique<IMemberGroupService, FixedMemberGroupService>();
}

[claude]

Claude finished @AndyButland's task in 5m 51s —— View job

---

PR Review

Target: origin/main · Based on commit: b251205f

Fixes the Public Access dialog not honouring custom IMemberGroupService implementations. Adds a new bulk-key lookup (GetAsync(IEnumerable<Guid>)) to the interface with a default in-memory implementation, wires ItemMemberGroupItemController and PublicAccessPresentationFactory through the service layer instead of going direct to IEntityService/IMemberRoleManager, and adds the matching IMemberGroup → MemberGroupItemResponseModel mapper.

• Modified public API: IMemberGroupService (new method GetAsync(IEnumerable<Guid>) with default impl), PublicAccessPresentationFactory (new 6-param primary constructor), ItemMemberGroupItemController (new 3-param primary constructor)
• Affected implementations (outside this PR): Any third-party IMemberGroupService implementation — covered by the default interface implementation (Pattern 3), no action required by consumers

---

Suggestions

• src/Umbraco.Cms.Api.Management/Controllers/MemberGroup/Item/ItemMemberGroupItemController.cs:30: The [ActivatorUtilitiesConstructor] primary constructor accepts IEntityService entityService but never assigns or uses it — DI injects this service on every controller instantiation for no effect. Since [ActivatorUtilitiesConstructor] already unambiguously selects this constructor for DI, entityService could be dropped from the primary constructor; the obsolete constructor can simply ignore it:

  [ActivatorUtilitiesConstructor]
  public ItemMemberGroupItemController(IUmbracoMapper mapper, IMemberGroupService memberGroupService)
  {
      _mapper = mapper;
      _memberGroupService = memberGroupService;
  }
  
  [Obsolete("Please use the constructor with all parameters. Scheduled for removal in Umbraco 19.")]
  public ItemMemberGroupItemController(IEntityService entityService, IUmbracoMapper mapper)
      : this(mapper, StaticServiceProvider.Instance.GetRequiredService<IMemberGroupService>())
  {
  }

  (Note: PublicAccessPresentationFactory does need to keep IMemberRoleManager in its primary constructor so the 6-param signature wins DI selection over the 5-param obsolete one — that situation is different and correct as-is.)

• src/Umbraco.Core/Services/IMemberGroupService.cs:86: Minor — the TODO comment format deviates slightly from the documented convention. Consider: // TODO (V19): Remove the default implementation — gives custom implementors until v18 to provide an efficient override.

---

Approved with Suggestions for improvement

Good to go, but please carefully consider the importance of the suggestions.

The breaking-change mitigations (Pattern 1 on both constructors, Pattern 3 on the interface) are applied correctly and the removal target version (v19 = 17 + 2) is right. The new MemberGroupService.GetAsync correctly follows the MemberService.GetByKeysAsync pattern. Test coverage is solid (unit + integration). Nice clean fix.

[Copilot]

Pull request overview

This PR fixes Public Access UI/API behavior to honor custom IMemberGroupService implementations (e.g., external user stores) when resolving member groups, addressing #22580.

Changes:

• Public Access dialog group resolution now uses IMemberGroupService.GetByName(...) instead of querying roles/entities directly.
• Added IMemberGroupService.GetAsync(IEnumerable<Guid> keys) bulk lookup, with an efficient override in MemberGroupService.
• Updated management API member-group item endpoint and mapping to work with IMemberGroup sources; adjusted/added tests accordingly.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
tests/Umbraco.Tests.UnitTests/Umbraco.Cms.Api.Management/Factories/PublicAccessPresentationFactoryTests.cs	Updates unit tests to mock IMemberGroupService for group resolution.
tests/Umbraco.Tests.Integration/Umbraco.Core/Services/MemberGroupServiceTests.cs	Adds integration test validating bulk lookup by keys.
src/Umbraco.Core/Services/MemberGroupService.cs	Implements efficient GetAsync(IEnumerable<Guid>) using repository query.
src/Umbraco.Core/Services/IMemberGroupService.cs	Introduces new bulk GetAsync(IEnumerable<Guid>) with a default fallback implementation.
src/Umbraco.Cms.Api.Management/Mapping/Item/ItemTypeMapDefinition.cs	Adds mapper definition from IMemberGroup to MemberGroupItemResponseModel.
src/Umbraco.Cms.Api.Management/Factories/PublicAccessPresentationFactory.cs	Switches group resolution to IMemberGroupService and keeps obsolete ctor for compatibility.
src/Umbraco.Cms.Api.Management/Controllers/MemberGroup/Item/ItemMemberGroupItemController.cs	Uses IMemberGroupService.GetAsync(ids) instead of IEntityService.GetAll(...) for item hydration.

[kjac]

Code looks good and it works as advertised.

I have also double checked login from the frontend, using a member group protection setup with group(s) from the custom member group service.

So far, so good.

Now, after testing I swapped back to the core member group service, and I noticed something I had not expected:

For some reason, the groups from the custom member group service were added to the DB 🤔

The top two member groups were assigned to my test member during the test. Only one of them were assigned to the content protection configuration. An early conclusion suggests that the member editing explicitly creates groups it does not recognize when saving members. Is this expected?

[AndyButland]

I think it is. If you trace through from MemberEditingService.UpdateRoles you'll see we eventually end up in MemberGroupRepository which does a direct write to umbracoNode for new member groups. This is necessary as cmsMember2MemberGroup has a foreign key to this table.

Importantly though, this goes via IMemberService, so I think it's reasonable to expect that if you are going to provide a custom IMemberGroupService, you'll need to provide a custom IMemberService alongside it. That's what the person who raised the issue this PR addresses is doing, and they had it working as needed in Umbraco 13.

[kjac]

In that case I'm happy with this 👍