update how azure gets the userid#216
Conversation
There was a problem hiding this comment.
Pull request overview
This PR changes how Azure resolves the principal ID for users when creating role assignments. The key improvement is ensuring that the system always uses the Microsoft Graph API to look up the user's Azure AD object ID based on their email, rather than relying on the user.ID field which may contain a Thand-internal ID instead of an Azure AD object ID.
Changes:
- Removed logic that attempted to use user.ID directly as an Azure object ID
- Changed from direct user lookup (ByUserId) to a filtered search that queries both 'mail' and 'userPrincipalName' fields
- Added GUID validation for retrieved object IDs
- Enhanced logging for debugging role assignment operations
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Query the user by their email address using a filter | ||
| // This searches both 'mail' and 'userPrincipalName' fields | ||
| // GET https://graph.microsoft.com/v1.0/users?$filter=mail eq 'email' or userPrincipalName eq 'email' | ||
| filter := fmt.Sprintf("mail eq '%s' or userPrincipalName eq '%s'", user.Email, user.Email) | ||
| requestConfig := &users.UsersRequestBuilderGetRequestConfiguration{ | ||
| QueryParameters: &users.UsersRequestBuilderGetQueryParameters{ | ||
| Filter: &filter, | ||
| }, | ||
| } | ||
|
|
||
| userList, err := client.Users().Get(ctx, requestConfig) |
There was a problem hiding this comment.
The change from a direct user lookup by email (ByUserId) to a filtered search may impact performance, especially if called frequently or in bulk operations. The filtered search requires processing query results and checking for null values, adding overhead compared to the direct lookup. Consider caching the principalID results if this method is called multiple times for the same user, or document the performance implications of this change.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "github.com/thand-io/agent/internal/models" | ||
| ) | ||
|
|
||
| var azureUserIDCache = make(map[string]string) |
There was a problem hiding this comment.
The azureUserIDCache is a package-level map that is accessed concurrently without synchronization. This can lead to race conditions when multiple goroutines call getUserPrincipalID simultaneously for different users. Consider using sync.Map or protecting this map with a sync.RWMutex to ensure thread-safe access.
| } | ||
|
|
||
| objectID := *graphUser.GetId() | ||
| azureUserIDCache[user.Email] = objectID |
There was a problem hiding this comment.
The objectID is stored in the cache before being validated as a proper GUID. If the validation fails (lines 272-279), the invalid objectID will remain in the cache and be returned on subsequent calls for the same email. Move the cache population (line 269) to after the GUID validation succeeds to ensure only valid object IDs are cached.
| // Sanitize email to prevent OData filter injection | ||
| // Escape single quotes by doubling them (OData escaping standard) | ||
| sanitizedEmail := strings.ReplaceAll(user.Email, "'", "''") |
There was a problem hiding this comment.
The email sanitization only escapes single quotes to prevent OData filter injection. However, this may not be sufficient for all edge cases. Consider also handling backslashes and other special characters that could be used in OData injection attacks, or use a more robust escaping function if available in the Microsoft Graph SDK.
| logrus.WithFields(logrus.Fields{ | ||
| "email": user.Email, | ||
| "matches_returned": len(usersValue), | ||
| "user_id": usersValue[0], |
There was a problem hiding this comment.
The log statement is attempting to log the entire user object (usersValue[0]) in the "user_id" field. This will likely print the object's memory address or type information rather than the user's ID. Extract and log the actual ID using usersValue[0].GetId() instead.
| logrus.WithFields(logrus.Fields{ | |
| "email": user.Email, | |
| "matches_returned": len(usersValue), | |
| "user_id": usersValue[0], | |
| var firstUserID interface{} | |
| if len(usersValue) > 0 && usersValue[0] != nil && usersValue[0].GetId() != nil { | |
| firstUserID = *usersValue[0].GetId() | |
| } | |
| logrus.WithFields(logrus.Fields{ | |
| "email": user.Email, | |
| "matches_returned": len(usersValue), | |
| "user_id": firstUserID, |
| // Check for exact match on mail field | ||
| if mail := u.GetMail(); mail != nil && strings.EqualFold(*mail, user.Email) { | ||
| exactMatches = append(exactMatches, i) | ||
| continue | ||
| } | ||
| // Check for exact match on userPrincipalName field | ||
| if upn := u.GetUserPrincipalName(); upn != nil && strings.EqualFold(*upn, user.Email) { | ||
| exactMatches = append(exactMatches, i) | ||
| } |
There was a problem hiding this comment.
The exact match logic has a potential issue: when a user matches on the mail field (line 225), the code adds the index and continues, skipping the userPrincipalName check. However, when a user matches on userPrincipalName (line 230), the code adds the index but doesn't continue. This means if a user has both mail and userPrincipalName matching (which should be the same user), they could be added to exactMatches twice at the same index, creating duplicate entries in the exactMatches array. Consider adding a 'continue' after line 231 or restructuring the logic to use 'else if' to prevent this potential duplication.
this change aims to improve how we resolve the principleId for a user in azure.