Adding Common part to User content type causes crash

[orchardbot]

planetClaire created: https://orchard.codeplex.com/workitem/18365

Adding the Common part to the User ContentType in the dashboard causes a stackoverflow on a new default install of the latest source code. The exception occurs here: Orchard.Core.Common.Handlers.CommonPartHandler.AssignCreatingOwner(Orchard.ContentManagement.Handlers.InitializingContentContext context, Orchard.Core.Common.Models.CommonPart part)

Some background info: http://orchard.codeplex.com/discussions/286349 My original intent was to create a list of users, but there were problems with adding the Containable part to the User content type. That might be a separate issue, please see the discussion.

[orchardbot]

@petehurst commented:

I see what's happening:: when CommonPart initializes the Owner is set, using _authenticationService.GetAuthenticatedUser() - which of course results in a call to content manager and OnInitialize is triggered again, ad infinitum.

There's another issue buried here, I think:

    protected void AssignCreatingOwner(InitializingContentContext context, CommonPart part) {
        // and use the current user as Owner
        if (part.Record.OwnerId == 0) {
            part.Owner = _authenticationService.GetAuthenticatedUser();
        }
    }

What if this happens:

• User A (administrator) adds CommonPart to a content type that didn't previously have it
• User B (not administrator) views an item of that type. They will be immediately assigned as the owner because it's the first time that item has been initialized since CommonPart was added...

[orchardbot]

@bleroy commented:

I think the original intent, adding Containable to make it possible to make lists of users, is not the right way of implementing your scenario. I would recommend instead using taxonomies.

[agriffard]

Linked to #5167

[JamesNK]

I ran into this bug :(

I want to make user's containable and place them in a list.

[bleroy]

I'm not saying that this shouldn't be fixed ;) But did you try to implement the scenario using taxonomies?

[JamesNK]

Users can create teams in Orchard. Teams have lots of data against them including a logo. A team then has a list of users. Using a taxonomy doesn't fit.

[bleroy]

Well, it could fit (taxonomy terms are content items, and their type can be extended), but the admin ui would admittedly be clunky in comparison with lists.

[mdameer]

I also ran into this bug when I upgraded orchard from 1.9.2 to 1.10, until I remove the common part from user type (after 2 days of researching), I added it to get the user item history (creation date, modified date, owner ... etc).

[Xceno]

Same here, i need to make OrganizationUnits and we should be able to move User into OU containers. Unfortunately I don't have much time at hand right now, but I gonna investigate it a bit. Maybe i find something and can at least provide a PR to prevent the stackoverflow.

[Xceno]

Okay thanks to @petehurst's comment above from 2012 the solution seems quite easy at first sight. It may not be super elegant, but works:

I would simply check if the current ContentType is a User and skip the code that causes the loop like this:

Original AssignCreatingOwner

Original GetUserName

Proposed fix

protected void AssignCreatingOwner(InitializingContentContext context, CommonPart part) {
  // and use the current user as Owner
  if (part.Record.OwnerId == 0) {
    if ( context.ContentType.Equals("User", StringComparison.OrdinalIgnoreCase)) {
      return;
    }

    part.Owner = _authenticationService.GetAuthenticatedUser();
  }
}

// .... Other code

private string GetUserNameSafe(CommonPart part) {
  if ( part.ContentItem.TypeDefinition.Name.Equals("User", StringComparison.OrdinalIgnoreCase) ) {
    return string.Empty;
  }

  var user = _authenticationService.GetAuthenticatedUser();
  return user == null ? string.Empty : user.UserName;
}

The question is, if we should assign a User to itself as an owner or not. If not, the CommonPartDriver will refuse to update, since the Owner field is required. As it seems, the OnInitializing hooks aren't suitable for retrieving the username though. So we'd need to figure out where and how to do this properly if wanted.

Anyway, for making users listable in Orchard.Lists the above changes are enough to make it work.

Edit: Some thoughts after a quick discussion with @MatteoPiovanelli-Laser on gitter:

• Having a user own itself will eventually lead to nasty infinite recursions
• The problem could also be solved by making lists / containers work without the CommonPart. This however wont be doable anytime soon without a major breaking change (And I don't think anybody wants this)

So there is only one question remaining: Should we apply a fix like the one above, to at least circumvent the stackoverflow or just let everything be as is?