From ad128c4eae8fb9e34575bf716dc1c301b14bfeb5 Mon Sep 17 00:00:00 2001 From: Yury Tsarev Date: Fri, 19 Jun 2026 16:14:43 +0200 Subject: [PATCH 1/2] 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 --- fn.go | 58 +++++++++++++++++++++++++---------- fn_test.go | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 131 insertions(+), 16 deletions(-) diff --git a/fn.go b/fn.go index ea8d2e4..bee9863 100644 --- a/fn.go +++ b/fn.go @@ -576,7 +576,10 @@ func (g *GraphQuery) fetchGroupMembers(ctx context.Context, client *msgraphsdk.G // See: https://developer.microsoft.com/en-us/graph/known-issues/?search=25984 requestConfig := &groups.GroupItemRequestBuilderGetRequestConfiguration{ QueryParameters: &groups.GroupItemRequestBuilderGetQueryParameters{ - Expand: []string{"members"}, + // Explicitly select the standard member fields via a nested $select so + // that user properties such as mail and userPrincipalName are returned + // for the expanded members (see issue #115). + Expand: []string{"members($select=id,displayName,mail,userPrincipalName,appId)"}, }, } @@ -639,24 +642,47 @@ func (g *GraphQuery) extractStringProperty(additionalData map[string]interface{} return "", false } -// extractUserProperties extracts user-specific properties from additionalData -func (g *GraphQuery) extractUserProperties(additionalData map[string]interface{}, memberMap map[string]interface{}) { - // Extract mail property - if mail, ok := g.extractStringProperty(additionalData, "mail"); ok { - memberMap["mail"] = mail +// extractUserProperties extracts user-specific properties. +// Members expanded via $expand are deserialized into typed objects, so the +// values live on the typed getters rather than in additionalData. We prefer +// the typed getters and fall back to additionalData for plain DirectoryObjects. +func (g *GraphQuery) extractUserProperties(member models.DirectoryObjectable, additionalData map[string]interface{}, memberMap map[string]interface{}) { + if user, ok := member.(models.Userable); ok { + if mail := ptr.Deref(user.GetMail(), ""); mail != "" { + memberMap["mail"] = mail + } + if upn := ptr.Deref(user.GetUserPrincipalName(), ""); upn != "" { + memberMap["userPrincipalName"] = upn + } } - // Extract userPrincipalName property - if upn, ok := g.extractStringProperty(additionalData, "userPrincipalName"); ok { - memberMap["userPrincipalName"] = upn + // Fall back to additionalData when the typed getters did not provide a value. + if _, ok := memberMap["mail"]; !ok { + if mail, found := g.extractStringProperty(additionalData, "mail"); found { + memberMap["mail"] = mail + } + } + if _, ok := memberMap["userPrincipalName"]; !ok { + if upn, found := g.extractStringProperty(additionalData, "userPrincipalName"); found { + memberMap["userPrincipalName"] = upn + } } } -// extractServicePrincipalProperties extracts service principal specific properties -func (g *GraphQuery) extractServicePrincipalProperties(additionalData map[string]interface{}, memberMap map[string]interface{}) { - // Extract appId property - if appID, ok := g.extractStringProperty(additionalData, "appId"); ok { - memberMap["appId"] = appID +// extractServicePrincipalProperties extracts service principal specific properties. +// As with users, prefer the typed getter and fall back to additionalData. +func (g *GraphQuery) extractServicePrincipalProperties(member models.DirectoryObjectable, additionalData map[string]interface{}, memberMap map[string]interface{}) { + if sp, ok := member.(models.ServicePrincipalable); ok { + if appID := ptr.Deref(sp.GetAppId(), ""); appID != "" { + memberMap["appId"] = appID + } + } + + // Fall back to additionalData when the typed getter did not provide a value. + if _, ok := memberMap["appId"]; !ok { + if appID, found := g.extractStringProperty(additionalData, "appId"); found { + memberMap["appId"] = appID + } } } @@ -710,9 +736,9 @@ func (g *GraphQuery) processMember(member models.DirectoryObjectable) map[string // Extract type-specific properties switch memberType { case userType: - g.extractUserProperties(additionalData, memberMap) + g.extractUserProperties(member, additionalData, memberMap) case servicePrincipalType: - g.extractServicePrincipalProperties(additionalData, memberMap) + g.extractServicePrincipalProperties(member, additionalData, memberMap) } return memberMap diff --git a/fn_test.go b/fn_test.go index 359440d..7770349 100644 --- a/fn_test.go +++ b/fn_test.go @@ -7,6 +7,7 @@ import ( "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" + "github.com/microsoftgraph/msgraph-sdk-go/models" "github.com/upbound/function-msgraph/input/v1beta1" "google.golang.org/protobuf/testing/protocmp" "google.golang.org/protobuf/types/known/durationpb" @@ -3815,3 +3816,91 @@ func TestIdentityType(t *testing.T) { }) } } + +// newTestUser builds a typed user directory object, mirroring how the Graph SDK +// deserializes expanded group members (the values land on the typed struct, not +// in additionalData). +func newTestUser() models.DirectoryObjectable { + user := models.NewUser() + user.SetId(ptr.To("user-id-1")) + user.SetDisplayName(ptr.To("Test User 1")) + user.SetMail(ptr.To("user1@example.com")) + user.SetUserPrincipalName(ptr.To("user1@example.com")) + return user +} + +// newTestServicePrincipal builds a typed service principal directory object. +func newTestServicePrincipal() models.DirectoryObjectable { + sp := models.NewServicePrincipal() + sp.SetId(ptr.To("sp-id-1")) + sp.SetDisplayName(ptr.To("Test Service Principal")) + sp.SetAppId(ptr.To("sp-app-id-1")) + return sp +} + +// newTestDirectoryObject builds a plain directory object whose properties are +// only available via additionalData, exercising the fallback extraction path. +func newTestDirectoryObject() models.DirectoryObjectable { + do := models.NewDirectoryObject() + do.SetId(ptr.To("user-id-2")) + do.SetAdditionalData(map[string]interface{}{ + "displayName": "Fallback User", + "mail": "user2@example.com", + "userPrincipalName": "user2@example.com", + }) + return do +} + +// TestProcessMember exercises the real member-extraction path (bypassed by the +// mocked graphQuery in the RunFunction tests). It is the regression test for +// issue #115: typed user members must expose mail and userPrincipalName. +func TestProcessMember(t *testing.T) { + cases := map[string]struct { + reason string + member models.DirectoryObjectable + want map[string]interface{} + }{ + "TypedUserIncludesMailAndUPN": { + reason: "A typed user member should expose mail and userPrincipalName from the typed getters", + member: newTestUser(), + want: map[string]interface{}{ + "id": "user-id-1", + "displayName": "Test User 1", + "type": "user", + "mail": "user1@example.com", + "userPrincipalName": "user1@example.com", + }, + }, + "TypedServicePrincipalIncludesAppID": { + reason: "A typed service principal member should expose appId from the typed getter", + member: newTestServicePrincipal(), + want: map[string]interface{}{ + "id": "sp-id-1", + "displayName": "Test Service Principal", + "type": "servicePrincipal", + "appId": "sp-app-id-1", + }, + }, + "PlainDirectoryObjectUsesAdditionalDataFallback": { + reason: "A plain directory object should fall back to additionalData for user properties", + member: newTestDirectoryObject(), + want: map[string]interface{}{ + "id": "user-id-2", + "displayName": "Fallback User", + "type": "user", + "mail": "user2@example.com", + "userPrincipalName": "user2@example.com", + }, + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + g := &GraphQuery{log: logging.NewNopLogger()} + got := g.processMember(tc.member) + if diff := cmp.Diff(tc.want, got); diff != "" { + t.Errorf("%s\nprocessMember(...): -want, +got:\n%s", tc.reason, diff) + } + }) + } +} From dc67ff3d31682d4d56b1c9dc0b09c8e5c4b13bd7 Mon Sep 17 00:00:00 2001 From: Yury Tsarev Date: Sat, 20 Jun 2026 01:35:56 +0200 Subject: [PATCH 2/2] test: add typed user without mail edge case for processMember Covers the common Entra ID state where a user has a userPrincipalName but no mail attribute: mail is omitted while userPrincipalName and type=user are still returned. Addresses review feedback on PR #116. --- fn_test.go | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/fn_test.go b/fn_test.go index 7770349..9c1c91d 100644 --- a/fn_test.go +++ b/fn_test.go @@ -3829,6 +3829,18 @@ func newTestUser() models.DirectoryObjectable { return user } +// newTestUserWithoutMail builds a typed user directory object that has no mail +// attribute set (a common Entra ID state where the account has a UPN but the +// mail attribute is null). +func newTestUserWithoutMail() models.DirectoryObjectable { + user := models.NewUser() + user.SetId(ptr.To("user-id-3")) + user.SetDisplayName(ptr.To("No Mail User")) + user.SetUserPrincipalName(ptr.To("nomail@example.com")) + // mail intentionally left unset. + return user +} + // newTestServicePrincipal builds a typed service principal directory object. func newTestServicePrincipal() models.DirectoryObjectable { sp := models.NewServicePrincipal() @@ -3871,6 +3883,16 @@ func TestProcessMember(t *testing.T) { "userPrincipalName": "user1@example.com", }, }, + "TypedUserWithoutMailOmitsMail": { + reason: "A typed user without a mail attribute should omit mail but still expose userPrincipalName", + member: newTestUserWithoutMail(), + want: map[string]interface{}{ + "id": "user-id-3", + "displayName": "No Mail User", + "type": "user", + "userPrincipalName": "nomail@example.com", + }, + }, "TypedServicePrincipalIncludesAppID": { reason: "A typed service principal member should expose appId from the typed getter", member: newTestServicePrincipal(),