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..9c1c91d 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,113 @@ 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 +} + +// 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() + 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", + }, + }, + "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(), + 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) + } + }) + } +}