Implementation of User domain model

[ypcode]

Hello guys,

I am kicking a new chat here about the User model implementation in the SDK.
I would like to collect your input about what and how you think should be done.

Different aspects are tackled here:

Inheriting IPrincpal

The SSOM/CSOM object (SP)User is inheriting the (SP)Principal class, but as far as I know, only a few properties are common to the Principal contract, not any specific methods. However since it's the base class for User and Group, it might be handy, when using the SDK, to refer to them as a common base type (e.g. a collection of users and groups, ...)

https://docs.microsoft.com/en-us/previous-versions/office/sharepoint-server/ee542393(v=office.15)

The current idea is to expose the IPrincipal contract but not implementing it as concrete type (rather only in User, Group, ... concrete types)

Do you have any suggestions about that, any functional or technical reason not to do it that way ?

Seamless unified User concept across MS Graph and SharePoint

A common need I am more and more facing in my day to day work, is to work with either Graph or SP specific data refering to the same user. It think it would be really nice, from the SDK to have this unified concept of a user and being able to grab data from either API seamlessly without the need of handling some custom mapping logic in each and every piece of consumer software we write.

Again, can you please provide any input you might have about this ?

(ping @jansenbe @PaoloPia @pkbullock @JarbasHorst )

[jansenbe]

Thanks for starting up this discussion @ypcode. Having a unified model that allows library users to deal with users in graph/rest neutral manner is important as we don't want that a library user to fully understand which type of API is called. Given that data returned by Graph or REST is different your approach of having another interface (your IPrincipal) that describes the user/group in an API agnostic way is a good one. For user collections we could introduce some methods that can return a list of IPrincipal. In order to allow for seamless switching between graph/rest calls we need to ensure that the needed Graph user metadata is set when a user/group is loaded via REST and the needed REST metadata is set for when a user/group is loaded via Graph. This logic needs to be added in the jsonserializer

[ypcode]

Hi friends,

Throwing some ideas and observations here:

1. An entity already works through Graph and exists in the namespace PnP.Core.Model.AzureActiveDirectory.
   Should we have a dedicated namespace for a unified entity since it won't really be a 1 to 1 mapping with the Graph entity?
   (not really a fundamental concern, but always better IMHO to organize well the code for the sake of quality)

2. A User in SharePoint is identified by a UPN or an ID but local to a particular Site, this ID is used to get SP specific properties or call SP Methods on a particular user. Thus we would need to key the entity with two different information in order to be able to call seamlessly MS Graph or SP REST. Do you have some ideas how we could handle this considering what's already in place?
   Just out the the wild, I was thinking of a "Complex type" or tuple or whatever to be the value of the common "Key" property.
   From there, we would have some specific mapping logic to populate the Id or GraphId tokens.

3. How should we handle the key information to be populated. should a get to a User entity call automatically both Graph and SP Rest ? If called SP Rest first, I think we can rely on the UPN that can be used as key to the Graph /users API... but the other way around, I am really not sure...
   I did not test that $filter on UPN in Rest API would work, and also, the user on the current site might not be ensured yet (not in site users list,... well is that even possible to this point if the user has no access to the current site ?)

Throwing plenty of brain dump here, please give any input you might have :)

[ypcode]

[EDITED]
getting more into the code I got more insights about some of my questions below so updated them.

Very good insights and a step forward in the design, Thank you very much @jansenbe !

Some other thinking:

• 1 thing regarding the UPN as the key, the special SharePoint users like "Everyone" or the service accounts etc... will have a UPN that will not mean anything to Graph. I think that should be handled with some specific client exception if properties can't be fetched from Graph or if possible some resilient approach... not sure it worth it... we'll see.
  Also, I kind of remember there were issues in graph if the UPN contains some special characters... (but I no longer have the concrete example in mind...)

• Great that we already have an approach for populating the metadata! I will still have to figure out if a filter query would work using the UPN...
  [EDIT]
  When getting info from SharePoint first, I can set the Graph Metadata with the UserPrincipalName
  When getting info from Graph first, I should query SharePoint Rest based on UPN to get the SharePoint Id, can the converter method call the Rest API (e.g. using RawRequest ? If yes, how to handle batched requests ?)

• Also, just to be sure, the library will already handle calling both API in the same Get method call as soon as the model have both SharePoint and Graph properties attributes, am I right ? the SharePoint Id has to be resolved before calling Rest API...

I will already start trying implement some PoC on it and see if I bump into some blocking stone :)

[jansenbe]

About the UPN: you're right about those special claims. The underlying graph call will fail in those cases, maybe the error provided by the Graph API is clear enough?

About the approach: I would try to avoid doing upfront calls to prepare for a possible "rest to graph" or "graph to rest" transition as these "hidden" calls can result in perf issues (imagine someone iterating over thousands of users and for each user we do a rest call to get more info). When you get a graph user you do get the UPN and the same is true for when doing a SharePoint request for a user...not fully clear to me why you would need additional calls to start with. If someone wants to load more data of the user they can do a GetAsync (which will default to Graph), if they need SharePoint REST specific data then let's foresee an extra method (GetSharePointUserAsync) that uses SP REST to further complete the domain model. This method could then be called in "on-demand" scenarios when we see a transitioning from Graph to SP rest or vice versa.

About your last comment: the Get method will be flexible when both the SharePointType and GraphType attributes are set. If you ask a field that can only be provided via SharePoint REST we'll use SharePoint REST...else we favor the Graph API call. We'll never be doing both calls by default.

[jansenbe]

Also this works in Graph: https://graph.microsoft.com/v1.0/users?$filter=userPrincipalName eq 'bert.jansen@bertonline.onmicrosoft.com' and https://graph.microsoft.com/v1.0/users/bert.jansen@bertonline.onmicrosoft.com

[ypcode]

We're going forward.

A new challenge occurs :D

The Web object has an Author property which, according to REST and CSOM is a User.
However, it looks like, at least, on a Modern Team Site, it is actually a security group

        {
    "d": {
        "__metadata": {
            "id": "https://pvxdev.sharepoint.com/sites/pnpcoresdktestgroup/_api/Web",
            "uri": "https://pvxdev.sharepoint.com/sites/pnpcoresdktestgroup/_api/Web",
            "type": "SP.Web"
        },
        "Author": {
              .....
            "Id": 7,
            "IsHiddenInUI": true,
            "LoginName": "c:0o.c|federateddirectoryclaimprovider|d40f31cc-3876-40f4-bb39-fad5407d9ad4_o",
            "Title": "PnP Microsoft 365 library test with group Owners",
            "PrincipalType": 4,
            "Email": "pnpcoresdktestgroup@pvxdev.onmicrosoft.com",
            "Expiration": "",
            "IsEmailAuthenticationGuestUser": false,
            "IsShareByEmailGuestUser": false,
            "IsSiteAdmin": true,
            "UserId": null,
            "UserPrincipalName": null
        },
        "Id": "debb4230-758e-4649-aca8-d1cf0796684b"
    }
}

The big issue here is, as you can see, it has no UPN and its login name contains no UPN BUT
it contains the Guid of the group that can be fetched through Graph

GET https://graph.microsoft.com/v1.0/groups/d40f31cc-3876-40f4-bb39-fad5407d9ad4

{
    "@odata.context": "https://graph.microsoft.com/v1.0/$metadata#groups/$entity",
    "id": "d40f31cc-3876-40f4-bb39-fad5407d9ad4",
    "deletedDateTime": null,
    "classification": null,
    "createdDateTime": "2020-05-25T16:39:28Z",
    "creationOptions": [
        "ProvisionGroupHomepage",
        "HubSiteId:00000000-0000-0000-0000-000000000000"
    ],
    "description": "PnP Microsoft 365 library test with group",
    "displayName": "PnP Microsoft 365 library test with group",
    "expirationDateTime": null,
    "groupTypes": [
        "Unified"
    ],
    "isAssignableToRole": null,
    "mail": "pnpcoresdktestgroup@pvxdev.onmicrosoft.com",
    "mailEnabled": true,
    "mailNickname": "pnpcoresdktestgroup",
    "membershipRule": null,
    "membershipRuleProcessingState": null,
    "onPremisesDomainName": null,
    "onPremisesLastSyncDateTime": null,
    "onPremisesNetBiosName": null,
    "onPremisesSamAccountName": null,
    "onPremisesSecurityIdentifier": null,
    "onPremisesSyncEnabled": null,
    "preferredDataLocation": null,
    "preferredLanguage": null,
    "proxyAddresses": [
        "SMTP:pnpcoresdktestgroup@pvxdev.onmicrosoft.com"
    ],
    "renewedDateTime": "2020-05-25T16:39:28Z",
    "resourceBehaviorOptions": [],
    "resourceProvisioningOptions": [],
    "securityEnabled": false,
    "securityIdentifier": "S-1-12-1-3557765580-1089747062-3589945787-3566894400",
    "theme": null,
    "visibility": "Public",
    "onPremisesProvisioningErrors": []
}

Should we then type this property differently (IGroup ? ) and then have a specific way to get unified Graph/SP info for Groups as well ?

[jansenbe]

Wrapping up this thread: in the end we decided to go for a split model in which there's a specific method to get a SharePointUser instance from a GraphUser instance and vice versa.