Skip to content

Commit ad128c4

Browse files
committed
fix: return mail and userPrincipalName for GroupMembership user members
Expanded group members are deserialized into typed SDK objects, so user properties (mail, userPrincipalName) and service principal appId live on the typed getters rather than in additionalData. Read from the typed getters with an additionalData fallback, and request the fields explicitly via a nested $select. Adds direct processMember tests covering typed users, service principals, and the DirectoryObject fallback path. Fixes #115
1 parent cddedcf commit ad128c4

2 files changed

Lines changed: 131 additions & 16 deletions

File tree

fn.go

Lines changed: 42 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -576,7 +576,10 @@ func (g *GraphQuery) fetchGroupMembers(ctx context.Context, client *msgraphsdk.G
576576
// See: https://developer.microsoft.com/en-us/graph/known-issues/?search=25984
577577
requestConfig := &groups.GroupItemRequestBuilderGetRequestConfiguration{
578578
QueryParameters: &groups.GroupItemRequestBuilderGetQueryParameters{
579-
Expand: []string{"members"},
579+
// Explicitly select the standard member fields via a nested $select so
580+
// that user properties such as mail and userPrincipalName are returned
581+
// for the expanded members (see issue #115).
582+
Expand: []string{"members($select=id,displayName,mail,userPrincipalName,appId)"},
580583
},
581584
}
582585

@@ -639,24 +642,47 @@ func (g *GraphQuery) extractStringProperty(additionalData map[string]interface{}
639642
return "", false
640643
}
641644

642-
// extractUserProperties extracts user-specific properties from additionalData
643-
func (g *GraphQuery) extractUserProperties(additionalData map[string]interface{}, memberMap map[string]interface{}) {
644-
// Extract mail property
645-
if mail, ok := g.extractStringProperty(additionalData, "mail"); ok {
646-
memberMap["mail"] = mail
645+
// extractUserProperties extracts user-specific properties.
646+
// Members expanded via $expand are deserialized into typed objects, so the
647+
// values live on the typed getters rather than in additionalData. We prefer
648+
// the typed getters and fall back to additionalData for plain DirectoryObjects.
649+
func (g *GraphQuery) extractUserProperties(member models.DirectoryObjectable, additionalData map[string]interface{}, memberMap map[string]interface{}) {
650+
if user, ok := member.(models.Userable); ok {
651+
if mail := ptr.Deref(user.GetMail(), ""); mail != "" {
652+
memberMap["mail"] = mail
653+
}
654+
if upn := ptr.Deref(user.GetUserPrincipalName(), ""); upn != "" {
655+
memberMap["userPrincipalName"] = upn
656+
}
647657
}
648658

649-
// Extract userPrincipalName property
650-
if upn, ok := g.extractStringProperty(additionalData, "userPrincipalName"); ok {
651-
memberMap["userPrincipalName"] = upn
659+
// Fall back to additionalData when the typed getters did not provide a value.
660+
if _, ok := memberMap["mail"]; !ok {
661+
if mail, found := g.extractStringProperty(additionalData, "mail"); found {
662+
memberMap["mail"] = mail
663+
}
664+
}
665+
if _, ok := memberMap["userPrincipalName"]; !ok {
666+
if upn, found := g.extractStringProperty(additionalData, "userPrincipalName"); found {
667+
memberMap["userPrincipalName"] = upn
668+
}
652669
}
653670
}
654671

655-
// extractServicePrincipalProperties extracts service principal specific properties
656-
func (g *GraphQuery) extractServicePrincipalProperties(additionalData map[string]interface{}, memberMap map[string]interface{}) {
657-
// Extract appId property
658-
if appID, ok := g.extractStringProperty(additionalData, "appId"); ok {
659-
memberMap["appId"] = appID
672+
// extractServicePrincipalProperties extracts service principal specific properties.
673+
// As with users, prefer the typed getter and fall back to additionalData.
674+
func (g *GraphQuery) extractServicePrincipalProperties(member models.DirectoryObjectable, additionalData map[string]interface{}, memberMap map[string]interface{}) {
675+
if sp, ok := member.(models.ServicePrincipalable); ok {
676+
if appID := ptr.Deref(sp.GetAppId(), ""); appID != "" {
677+
memberMap["appId"] = appID
678+
}
679+
}
680+
681+
// Fall back to additionalData when the typed getter did not provide a value.
682+
if _, ok := memberMap["appId"]; !ok {
683+
if appID, found := g.extractStringProperty(additionalData, "appId"); found {
684+
memberMap["appId"] = appID
685+
}
660686
}
661687
}
662688

@@ -710,9 +736,9 @@ func (g *GraphQuery) processMember(member models.DirectoryObjectable) map[string
710736
// Extract type-specific properties
711737
switch memberType {
712738
case userType:
713-
g.extractUserProperties(additionalData, memberMap)
739+
g.extractUserProperties(member, additionalData, memberMap)
714740
case servicePrincipalType:
715-
g.extractServicePrincipalProperties(additionalData, memberMap)
741+
g.extractServicePrincipalProperties(member, additionalData, memberMap)
716742
}
717743

718744
return memberMap

fn_test.go

Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77

88
"github.com/google/go-cmp/cmp"
99
"github.com/google/go-cmp/cmp/cmpopts"
10+
"github.com/microsoftgraph/msgraph-sdk-go/models"
1011
"github.com/upbound/function-msgraph/input/v1beta1"
1112
"google.golang.org/protobuf/testing/protocmp"
1213
"google.golang.org/protobuf/types/known/durationpb"
@@ -3815,3 +3816,91 @@ func TestIdentityType(t *testing.T) {
38153816
})
38163817
}
38173818
}
3819+
3820+
// newTestUser builds a typed user directory object, mirroring how the Graph SDK
3821+
// deserializes expanded group members (the values land on the typed struct, not
3822+
// in additionalData).
3823+
func newTestUser() models.DirectoryObjectable {
3824+
user := models.NewUser()
3825+
user.SetId(ptr.To("user-id-1"))
3826+
user.SetDisplayName(ptr.To("Test User 1"))
3827+
user.SetMail(ptr.To("user1@example.com"))
3828+
user.SetUserPrincipalName(ptr.To("user1@example.com"))
3829+
return user
3830+
}
3831+
3832+
// newTestServicePrincipal builds a typed service principal directory object.
3833+
func newTestServicePrincipal() models.DirectoryObjectable {
3834+
sp := models.NewServicePrincipal()
3835+
sp.SetId(ptr.To("sp-id-1"))
3836+
sp.SetDisplayName(ptr.To("Test Service Principal"))
3837+
sp.SetAppId(ptr.To("sp-app-id-1"))
3838+
return sp
3839+
}
3840+
3841+
// newTestDirectoryObject builds a plain directory object whose properties are
3842+
// only available via additionalData, exercising the fallback extraction path.
3843+
func newTestDirectoryObject() models.DirectoryObjectable {
3844+
do := models.NewDirectoryObject()
3845+
do.SetId(ptr.To("user-id-2"))
3846+
do.SetAdditionalData(map[string]interface{}{
3847+
"displayName": "Fallback User",
3848+
"mail": "user2@example.com",
3849+
"userPrincipalName": "user2@example.com",
3850+
})
3851+
return do
3852+
}
3853+
3854+
// TestProcessMember exercises the real member-extraction path (bypassed by the
3855+
// mocked graphQuery in the RunFunction tests). It is the regression test for
3856+
// issue #115: typed user members must expose mail and userPrincipalName.
3857+
func TestProcessMember(t *testing.T) {
3858+
cases := map[string]struct {
3859+
reason string
3860+
member models.DirectoryObjectable
3861+
want map[string]interface{}
3862+
}{
3863+
"TypedUserIncludesMailAndUPN": {
3864+
reason: "A typed user member should expose mail and userPrincipalName from the typed getters",
3865+
member: newTestUser(),
3866+
want: map[string]interface{}{
3867+
"id": "user-id-1",
3868+
"displayName": "Test User 1",
3869+
"type": "user",
3870+
"mail": "user1@example.com",
3871+
"userPrincipalName": "user1@example.com",
3872+
},
3873+
},
3874+
"TypedServicePrincipalIncludesAppID": {
3875+
reason: "A typed service principal member should expose appId from the typed getter",
3876+
member: newTestServicePrincipal(),
3877+
want: map[string]interface{}{
3878+
"id": "sp-id-1",
3879+
"displayName": "Test Service Principal",
3880+
"type": "servicePrincipal",
3881+
"appId": "sp-app-id-1",
3882+
},
3883+
},
3884+
"PlainDirectoryObjectUsesAdditionalDataFallback": {
3885+
reason: "A plain directory object should fall back to additionalData for user properties",
3886+
member: newTestDirectoryObject(),
3887+
want: map[string]interface{}{
3888+
"id": "user-id-2",
3889+
"displayName": "Fallback User",
3890+
"type": "user",
3891+
"mail": "user2@example.com",
3892+
"userPrincipalName": "user2@example.com",
3893+
},
3894+
},
3895+
}
3896+
3897+
for name, tc := range cases {
3898+
t.Run(name, func(t *testing.T) {
3899+
g := &GraphQuery{log: logging.NewNopLogger()}
3900+
got := g.processMember(tc.member)
3901+
if diff := cmp.Diff(tc.want, got); diff != "" {
3902+
t.Errorf("%s\nprocessMember(...): -want, +got:\n%s", tc.reason, diff)
3903+
}
3904+
})
3905+
}
3906+
}

0 commit comments

Comments
 (0)