SubPointSolutions / spmeta2

SharePoint artifact provision for .NET platform. Supports SharePoint Online, SharePoint 2019, 2016 and 2013 via CSOM/SSOM.
http://subpointsolutions.com/spmeta2
133 stars 56 forks source link

SecurityGroupDefinition.IsAssociatedXXXGroup is not processed SSOM #1108

Closed andreasblueher closed 5 years ago

andreasblueher commented 6 years ago

Hello,

Brief description

Please provide short description covering two areas:

SharePoint API

Which version of SharePoint runtime do you use? SharePoint 2013 SSOM

SPMeta2 API

Which version of SPMeta2 do you use? Use the following code snippet for your convenience. It shows all the details on SPMeta2 and SharePoint runtime used. 1.2.140

SPMeta2 model

Optionally, provide SPMeta2 model in which issue occurs. That helps to identify the issue and provide the solution as far as we can.

public static SecurityGroupDefinition LeaderDefinition = new SecurityGroupDefinition
{
    Name = "Leitung Serviceportal",
    Description = "Leitung Serviceportal",
    IsAssociatedOwnerGroup = true
}

After looking into validators and handlers, I'm not sure what those properties are for, because there not implemented and skipped in validators.


Outstanding work:

SubPointSupport commented 6 years ago

It actually works differently to what you described early.

SharePoint has built-in groups for every web - Owners, Members and Visitors. These groups might be pre-created by SharePoint or assigned by end users. Hence, such groups are rather "dynamic", we never know what they are.

The next bit of the problem is that sometime we want to use these groups, Owners, Members and Visitors, within provision. As we don't know what these groups are, SPMeta2 introduced three properties in SecurityGroupDefinition:

By default, these properties aren't set and SPMeta2 creates a new group or updates existing one. However, is one of these properties set, then SPMeta2 does not do any provision. Instead, a corresponding built-in groups is resolved - Owners, Members or Visitors - for further provision. It works similar to .AddHostList() where a built-in list is only resolved for further provision, same way here - we indicate which group to resolve, and then it is used later.

In your example, a built-in Owner group will be resolved and later used in further provision.

andreasblueher commented 6 years ago

Ok I understand your thoughts now much better. My first note would be, that it's not 100% certain that there are 3 associated groups on every web. Depending on which template you select or if you choose to create the groups automatically you might end up with no groups at all. In this case further provision will run into "object reference not found" exceptions as I did in my web (template STS#1). You can't select this in SPO anymore, but for OnPremises this is still an option.

My second note: Whenever I set a property for a definition I set a property on the corresponding SharePoint element. Sometimes it even changes another related SharePoin element like a SPView when I have AddToDefaultView selected. Here "IsAssociatedMemberGroup" is used to kind of ignore (not 100% sure on this part) the definition and use the AssociatedMemberGroup instead. Wouldn't it be nicer to have some other syntax or maybe BuiltInAssociatedGroups definitions?

In my specific case I wanted to make one of my newly created SPGroups the AssociatedOwnerGroup which as I understand now is not possible yet. Since it would technically be possible, could you imagine this functionality into SPMeta?

andreasblueher commented 6 years ago

@SubPointSupport adding some more thoughts here, so we can proceed to try to find a fitting solution for both sides. Although I might be repeating stuff from my last post here, I'll try to rephrase it.

SPWeb has properties "AssociateXXXGroup" which cannot be set using SPMeta (as far as I know). Haveing seen the "IsAssociatedMemberGroup" property on your SPGroupDefinition I assumed this to be the way to assign a group as designated member group for the web. What you explained is that this property is only used to rather use SPWebAssociatedMemberGroup instead of creating a new SPGroup with the given properties. I want to repeat my suggestion to integrate BuiltInAssociatedGroups so one you specifically use those instead of creating a custom group definition.

SubPointSupport commented 6 years ago

SPWeb has properties "AssociateXXXGroup" which cannot be set using SPMeta (as far as I know

No, they cannot be set by SPMeta2. These props are supposed to be on the target web site already.

Haveing seen the "IsAssociatedMemberGroup" property on your SPGroupDefinition I assumed this to be the way to assign a group as designated member group for the web. What you explained is that this property is only used to rather use SPWebAssociatedMemberGroup instead of creating a new SPGroup with the given properties

Yes, for lookup only. You are right. You cannot assign groups, only suggest SPMeta2 to lookup and use you own, or built-in, already existing group. As names of such groups on the web sites aren't known, that was and is the way to dynamically lookup these groups before the provision.

I want to repeat my suggestion to integrate BuiltInAssociatedGroups so one you specifically use those instead of creating a custom group definition.

So that way, you would like to have the following behavior:

Or this one:

Could we describe your idea in such steps? First workflow, second or we got it wrong and you'll describe it right?

andreasblueher commented 6 years ago

For my scenario the first option would be almost perfect:

This way the only thing missing would be a way to reference the AssociatedOwnerGroup without creating/setting it myself before.

SubPointSupport commented 6 years ago

Yep, so currently AssociatedOwnerGroup is created or set externally, via script or something.

We need to bring ability to set AssociatedXXXGroup on the target web via SPMeta2 definitions to complete the picture. We need to give a few thoughts on that, the question is where to place it, should it be a new .AddXXX() methods syntax or new properties in SecurityGroupDefinition (such as PromoteAsOwnerGroup or something after creation). Surely, should be implemented.

SubPointSupport commented 6 years ago

Pu a few comments into PR. Echoing here too:

@andreasblueher that might not work as expected.

Consider two cases: 1) AssociatedMemberGroup is NULL, we want to create/assign it, so that further provision would work 2) AssociatedMemberGroup was already created, NOT NULL. We don't know the name of the group, hence refer to it via "AssociatedMemberGroup" property while defining SecurityGroup

Simple saying, changing this behavior from "only lookup" to "update what might be already there" would break backward compatibility introducing new behavior.

IsAssociatedVisitorsGroup.AssociatedMemberGroup, AssociatedOwnerGroup and AssociatedVisitorsGroup house the following code:

public static SecurityGroupDefinition AssociatedMemberGroup = new SecurityGroupDefinition
        {
            IsAssociatedMemberGroup = true
        };

        public static SecurityGroupDefinition AssociatedOwnerGroup = new SecurityGroupDefinition
        {
            IsAssociatedOwnerGroup = true
        };

        public static SecurityGroupDefinition AssociatedVisitorsGroup = new SecurityGroupDefinition
        {
            IsAssociatedVisitorsGroup = true
        };

Same way, we use IsAssociatedXXXGroup flag toonly lookup existing group assuming it was pre-created.

All magic, however, happens in SecurityGroupLinkDefinition. There is a .AddSecurityGroupLink() method which processed SecurityGroupDefinition properties copying IsAssociatedXXXGroup -> into SecurityGroupLinkDefinition. Only then, SecurityGroupLinkModelHandler for CSOM/SSOM try to resolve these built-in groups from the webs.

WHat can be done, however, is adding new properties such as "SetAsAssociatedXXXGroup" within SecurityGroupDefinition which would do exactly what you wanted - update corresponding values with in SecurityGrouphandlers. That should work well.

If this sounds good, we'll reject this OR and implement "SetAsAssociatedXXXGroup" properties. What do you think, @andreasblueher ?

SubPointSupport commented 6 years ago

Looked into some implementations around this. All comes to the point where we need to update web's properties as following:

web.AssociatedOwnerGroup   = targetGroup;
web.AssociatedMemberGroup  = targetGroup;
web.AssociatedVisitorGroup = targetGroup;

That's a bit challenging to express with existing .AddXXX() syntax. We have a few methods right now:

With updating web's AssociatedOwnerGroup, AssociatedMemberGroup and AssociatedVisitorGroup, we simple don't have an appropriate syntax to describe the model. All ways to fit into current syntax and implementation don't seem to be appropriate.

Way forward could be the following:

Underneath, that would be a new definition and handler express by this particular .AddAssociatedXXXGroup() methods. Implementation for CSOM/SSOM would update the right properties for web setting corresponding properties.

Here is how it might look like at syntax level:

var securityGroupDefinition = ...;
var myWebDefinition = ...;

# create security group 
var siteModel = SPMeta2Model.NewSiteModel(site => {
    site.AddSecurityGroup(securityGroupDefinition);
});

# link security group with web, using one of these methods
var webModel = SPMeta2Model.NewWebModel(web=> {
    # one of these methods
    web.AddAssociatedOwnerGroup(securityGroupDefinition);
    web.AddAssociatedMemberGroup(securityGroupDefinition);
    web.AddAssociatedVisitorGroup(securityGroupDefinition);
});

Again, all other options seem to break existing workflow: the way model gets created and provisioned along with linking/resolving security groups. We need a new way to express SPWeb properties updates related to security groups, unfortunately, both semantically and at the model handler level.

Sharing this design here. @andreasblueher , looking for some ideas on how it might work. If all good, we'll implement this along with testing.

andreasblueher commented 6 years ago

I really like the suggested syntax. Having a "AddAssociatedOwnerGroup" method would render the somewhere else discussed "SetAssociatedOwnerGroup" property for the SecurityGroupDefinition rather useless.

On top of those last suggested changes I would like to add a small change to https://github.com/SubPointSolutions/spmeta2/blob/5d5121e61f45a59a95589f945047a84d9bc9ccd4/SPMeta2/SPMeta2.SSOM/ModelHandlers/SecurityGroupLinkModelHandler.cs#L58-L90

I find nullreference exceptions usually very annoying and therefor I would like to add additional null checks on web.AssociatedMemberGroup and so forth so this method can never return null and rather throw an exception.

SubPointSupport commented 6 years ago

Hey @andreasblueher, thanks for getting back on this.

Great! Glad we figured it out. So here is how we'll handle this:

1) We'll go with suggested AddAssociatedXXXGroup() syntax 2) Add more null-ref checking for ResolveSecurityGroup() method, you are right - https://github.com/SubPointSolutions/spmeta2/blob/5d5121e61f45a59a95589f945047a84d9bc9ccd4/SPMeta2/SPMeta2.SSOM/ModelHandlers/SecurityGroupLinkModelHandler.cs#L58-L90

Reflected in the ticket description as well. Working on this today.

SubPointSupport commented 6 years ago

In progress, 1-2 days to go so that 1.2.150-beta2 milestone can be closed. Renaming it into "1.2.150-beta1" as previous week didn't have much changes to push to NuGet.

andreasblueher commented 6 years ago

How are things going?