Skip to content
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
126 changes: 113 additions & 13 deletions internal/providers/azure/roles.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,14 @@ import (
"github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/authorization/armauthorization"
"github.com/google/uuid"
msgraphsdk "github.com/microsoftgraph/msgraph-sdk-go"
"github.com/microsoftgraph/msgraph-sdk-go/users"
"github.com/sirupsen/logrus"
"github.com/thand-io/agent/internal/data"
"github.com/thand-io/agent/internal/models"
)

var azureUserIDCache = make(map[string]string)

Copilot AI Jan 13, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

// Never synchronize roles from Azure as they are
// statically defined by Azure and cannot be modified
func (p *azureProvider) CanSynchronizeRoles() bool {
Expand Down Expand Up @@ -89,6 +92,15 @@ func (p *azureProvider) createRoleAssignment(ctx context.Context, user *models.U
}

roleAssignmentID := uuid.New().String()

// Log the principal ID for debugging
logrus.WithFields(logrus.Fields{
"principal_id": principalID,
"user_email": user.Email,
"role_id": roleDefinitionID,
"scope": scope,
}).Info("Creating Azure role assignment")

roleAssignment := armauthorization.RoleAssignmentCreateParameters{
Properties: &armauthorization.RoleAssignmentProperties{
RoleDefinitionID: &roleDefinitionID,
Expand Down Expand Up @@ -147,15 +159,15 @@ func (p *azureProvider) getUserPrincipalID(ctx context.Context, user *models.Use
return "", fmt.Errorf("user email is required for Azure role assignments")
}

// If the user's ID field already contains an Azure object ID (GUID format), use it
if len(user.ID) > 0 && len(user.ID) >= 32 {
// Validate it looks like a GUID
if _, err := uuid.Parse(user.ID); err == nil {
logrus.WithField("user_id", user.ID).Debug("Using existing Azure object ID from user.ID field")
return user.ID, nil
}
// Check cache first
if objectID, found := azureUserIDCache[user.Email]; found {
logrus.WithField("email", user.Email).Debug("Using cached Azure AD object ID")
return objectID, nil
}

// NOTE: We always use Microsoft Graph API to lookup the user's Azure AD object ID
// even if user.ID is set, because user.ID may be a Thand-internal ID, not an Azure AD object ID.

// Use Microsoft Graph API to lookup the user by email and get their object ID
logrus.WithField("email", user.Email).Debug("Looking up Azure AD object ID via Microsoft Graph API")

Expand All @@ -165,23 +177,111 @@ func (p *azureProvider) getUserPrincipalID(ctx context.Context, user *models.Use
return "", fmt.Errorf("failed to create Microsoft Graph client: %w", err)
}

// Query the user by their email address (UPN)
// GET https://graph.microsoft.com/v1.0/users/{email}
graphUser, err := client.Users().ByUserId(user.Email).Get(ctx, nil)
// Sanitize email to prevent OData filter injection
// Escape single quotes by doubling them (OData escaping standard)
sanitizedEmail := strings.ReplaceAll(user.Email, "'", "''")
Comment on lines +180 to +182

Copilot AI Jan 13, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

// 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'", sanitizedEmail, sanitizedEmail)
requestConfig := &users.UsersRequestBuilderGetRequestConfiguration{
QueryParameters: &users.UsersRequestBuilderGetQueryParameters{
Filter: &filter,
},
}

userList, err := client.Users().Get(ctx, requestConfig)
Comment on lines +184 to +194

Copilot AI Jan 13, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
if err != nil {
return "", fmt.Errorf("failed to lookup user '%s' in Azure AD via Microsoft Graph API: %w", user.Email, err)
return "", fmt.Errorf("failed to search for user '%s' in Azure AD via Microsoft Graph API: %w", user.Email, err)
}

// Check if we found any users
if userList == nil || len(userList.GetValue()) == 0 {
return "", fmt.Errorf("user '%s' not found in Azure AD", user.Email)
}

// Resolve the matching user from the result set, handling multiple matches
usersValue := userList.GetValue()
var graphUser = usersValue[0] // Default to first user

if len(usersValue) == 1 {
// Only one user returned, use it directly
logrus.WithField("email", user.Email).Debug("Single Azure AD user found")
} else {
// Multiple users returned; try to find an exact match on mail or userPrincipalName
logrus.WithFields(logrus.Fields{
"email": user.Email,
"matches_returned": len(usersValue),
}).Warn("Multiple Azure AD users matched email filter; attempting exact match")

var exactMatches []int // Store indices of exact matches
for i, u := range usersValue {
if u == nil {
continue
}

// 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)
}
Comment on lines +224 to +232

Copilot AI Jan 13, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
}

switch len(exactMatches) {
case 0:
// No exact matches; use the first result but log a warning
logrus.WithFields(logrus.Fields{
"email": user.Email,
"matches_returned": len(usersValue),
"user_id": usersValue[0],
Comment on lines +238 to +241

Copilot AI Jan 13, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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,

Copilot uses AI. Check for mistakes.
}).Warn("Multiple Azure AD users matched filter, but none matched exactly; using first result")
graphUser = usersValue[0]
case 1:
// Single exact match found, use it
logrus.WithField("email", user.Email).Info("Single exact Azure AD match found among multiple results")
graphUser = usersValue[exactMatches[0]]
default:
// Multiple exact matches; log a warning and use the first exact match
logrus.WithFields(logrus.Fields{
"email": user.Email,
"exact_matches": len(exactMatches),
"matches_returned": len(usersValue),
}).Warn("Multiple Azure AD users matched email exactly; using first exact match")
graphUser = usersValue[exactMatches[0]]
}
}

if graphUser == nil {
return "", fmt.Errorf("user '%s' found in Azure AD but response is invalid", user.Email)
}

// Extract the object ID from the response
if graphUser == nil || graphUser.GetId() == nil {
if graphUser.GetId() == nil {
return "", fmt.Errorf("user '%s' found in Azure AD but object ID is missing", user.Email)
}

objectID := *graphUser.GetId()
azureUserIDCache[user.Email] = objectID

Copilot AI Jan 13, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

// Validate the object ID is a proper GUID
if _, err := uuid.Parse(objectID); err != nil {
logrus.WithFields(logrus.Fields{
"email": user.Email,
"object_id": objectID,
"error": err,
}).Error("Retrieved object ID is not a valid GUID")
return "", fmt.Errorf("user '%s' has invalid object ID '%s': %w", user.Email, objectID, err)
}

logrus.WithFields(logrus.Fields{
"email": user.Email,
"object_id": objectID,
}).Debug("Successfully retrieved Azure AD object ID")
}).Info("Successfully retrieved Azure AD object ID")

return objectID, nil
}
Expand Down
Loading