Skip to content
This repository was archived by the owner on Oct 10, 2025. It is now read-only.

feat: added EntraID in favor of AzureAD and restored old behavior for AzureAD - #668

Merged
dbarrosop merged 6 commits into
mainfrom
azureadbug
Aug 26, 2025
Merged

feat: added EntraID in favor of AzureAD and restored old behavior for AzureAD#668
dbarrosop merged 6 commits into
mainfrom
azureadbug

Conversation

@dbarrosop

@dbarrosop dbarrosop commented Aug 25, 2025

Copy link
Copy Markdown
Member

PR Type

Enhancement


Description

  • Add EntraID OAuth provider as replacement for deprecated AzureAD

  • Deprecate AzureAD provider with warning message

  • Fix OAuth profile validation for empty provider user IDs

  • Update API types and OpenAPI specification


Diagram Walkthrough

flowchart LR
  AzureAD["AzureAD Provider (deprecated)"] -- "replaced by" --> EntraID["EntraID Provider"]
  EntraID --> API["API Types Updated"]
  EntraID --> Migration["Database Migration"]
  EntraID --> Validation["Profile Validation"]
Loading

File Walkthrough

Relevant files
Enhancement
9 files
server.gen.go
Update generated API server with EntraID support                 
+159/-159
types.gen.go
Add EntraID constants to generated types                                 
+4/-0     
oauth.go
Add EntraID provider configuration and deprecation warning
+18/-0   
serve.go
Add EntraID CLI flags and configuration                                   
+41/-1   
azuread.go
Simplify AzureAD provider implementation                                 
+8/-10   
entraid.go
Implement new EntraID OAuth provider                                         
+70/-0   
scopes.go
Add default scopes for EntraID provider                                   
+4/-1     
00018_entraid-provider.down.sql
Add EntraID provider migration rollback                                   
+4/-0     
00018_entraid-provider.up.sql
Add EntraID provider database migration                                   
+8/-0     
Formatting
2 files
elevate_webauthn_test.go
Fix linter comment for credential ID constant                       
+1/-1     
sign_in_webauthn_test.go
Fix linter comment for credential ID constant                       
+1/-1     
Bug fix
2 files
sign_in_provider_callback_get.go
Add validation for empty provider user ID                               
+5/-0     
workflows.go
Add validation for empty provider user ID                               
+5/-0     
Configuration changes
1 files
.golangci.yaml
Update linter configuration for issue limits                         
+3/-0     
Documentation
1 files
openapi.yaml
Add EntraID to OpenAPI specification                                         
+1/-0     

@github-actions

github-actions Bot commented Aug 25, 2025

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

(Review updated until commit 3149fbb)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Breaking Change

The AzureAD provider implementation has been significantly modified, changing from v2.0 endpoints to older v1.0 endpoints and removing custom parameters. This could break existing integrations and may not be backward compatible.

		AuthURL:  formatAzureADURL(tenant, "/oauth2/authorize?prompt=select_account"),
		TokenURL: formatAzureADURL(tenant, "/oauth2/token"),
	},
},
ProfileURL: formatAzureADURL(tenant, "/openid/userinfo"),
Validation Logic

The empty provider user ID validation is added after profile fetching but should be validated to ensure it doesn't interfere with legitimate empty ID scenarios from certain OAuth providers.

if profile.ProviderUserID == "" {
	logger.ErrorContext(ctx, "provider user id is empty")
	return oidc.Profile{}, ErrOauthProfileFetchFailed
}
Error Handling

The EntraID provider implementation lacks comprehensive error handling for API failures and should validate the user profile structure more thoroughly before constructing the final profile.

func (a *EntraID) GetProfile(
	ctx context.Context,
	accessToken string,
	_ *string,
	_ map[string]any,
) (oidc.Profile, error) {
	var userProfile entraidUser
	if err := fetchOAuthProfile(
		ctx,
		a.ProfileURL,
		accessToken,
		&userProfile,
	); err != nil {
		return oidc.Profile{}, fmt.Errorf("EntraID API error: %w", err)
	}

	return oidc.Profile{
		ProviderUserID: userProfile.Sub,
		Email:          userProfile.Email,
		EmailVerified:  userProfile.Email != "",
		Name:           userProfile.GivenName + " " + userProfile.FamilyName,
		Picture:        "",
	}, nil
}

@github-actions

github-actions Bot commented Aug 25, 2025

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Latest suggestions up to 3149fbb
Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Update to OAuth v2.0 endpoints

The AzureAD provider is using OAuth 2.0 v1.0 endpoints instead of the modern v2.0
endpoints. This could cause compatibility issues and limits access to newer
Microsoft Graph features. Update to use v2.0 endpoints for consistency with the
EntraID provider.

go/providers/azuread.go [32-33]

-AuthURL:  formatAzureADURL(tenant, "/oauth2/authorize?prompt=select_account"),
-TokenURL: formatAzureADURL(tenant, "/oauth2/token"),
+AuthURL:  formatAzureADURL(tenant, "/oauth2/v2.0/authorize?prompt=select_account"),
+TokenURL: formatAzureADURL(tenant, "/oauth2/v2.0/token"),
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that the AzureAD provider should use v2.0 endpoints for consistency with modern OAuth standards and the new EntraID provider. This improves compatibility and access to newer features.

Medium
Use Microsoft Graph userinfo endpoint

The profile URL is using the v1.0 OpenID endpoint which may not provide consistent
user information. For better compatibility and consistency with the EntraID
provider, use the Microsoft Graph userinfo endpoint instead.

go/providers/azuread.go [36]

-ProfileURL: formatAzureADURL(tenant, "/openid/userinfo"),
+ProfileURL: "https://graph.microsoft.com/oidc/userinfo",
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly points out that using the Microsoft Graph userinfo endpoint provides better consistency with the EntraID provider and more reliable user information access.

Low
Eliminate duplicate scope arrays

Having identical scope arrays creates maintenance overhead and potential
inconsistency. Consider making DefaultEntraIDScopes reference DefaultAzureadScopes
to maintain a single source of truth.

go/providers/scopes.go [36-39]

 // DefaultAzureadScopes defines the default scopes for AzureAd OAuth2.
 DefaultAzureadScopes = []string{"email", "profile", "openid", "offline_access"}
 
 // DefaultEntraIDScopes defines the default scopes for EntraID OAuth2.
-DefaultEntraIDScopes = []string{"email", "profile", "openid", "offline_access"}
+DefaultEntraIDScopes = DefaultAzureadScopes
Suggestion importance[1-10]: 6

__

Why: Valid suggestion to reduce code duplication by making DefaultEntraIDScopes reference DefaultAzureadScopes. This improves maintainability and ensures consistency between the two scope arrays.

Low
Move validation check earlier

The validation should occur before any profile processing to avoid unnecessary work.
Consider moving this check earlier in the function flow to fail fast when the
provider user ID is missing.

go/controller/sign_in_provider_callback_get.go [119-122]

+if profile.ProviderUserID == "" {
+    logger.ErrorContext(ctx, "provider user id is empty")
+    return oidc.Profile{}, ErrOauthProfileFetchFailed
+}
 
-
Suggestion importance[1-10]: 4

__

Why: The suggestion is technically correct but lacks context about where to move the check. The improved_code is identical to existing_code, showing no actual improvement.

Low

Previous suggestions

Suggestions up to commit f96a837
CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix Azure AD OAuth endpoints

The OAuth2 endpoints are using incorrect paths for Azure AD. Azure AD v2.0 endpoints
require /oauth2/v2.0/authorize and /oauth2/v2.0/token paths to function properly.

go/providers/azuread.go [33-34]

-AuthURL:  formatAzureADURL(tenant, "/oauth2/authorize"),
-TokenURL: formatAzureADURL(tenant, "/oauth2/token"),
+AuthURL:  formatAzureADURL(tenant, "/oauth2/v2.0/authorize"),
+TokenURL: formatAzureADURL(tenant, "/oauth2/v2.0/token"),
Suggestion importance[1-10]: 9

__

Why: This is a critical bug fix. The Azure AD OAuth endpoints require /oauth2/v2.0/ paths to function properly, and using incorrect paths would cause authentication failures.

High
Use correct Microsoft Graph endpoint

The profile URL path is incorrect for Azure AD. The correct Microsoft Graph endpoint
for user information is https://graph.microsoft.com/oidc/userinfo, not a
tenant-specific URL.

go/providers/azuread.go [37]

-ProfileURL:   formatAzureADURL(tenant, "/openid/userinfo"),
+ProfileURL:   "https://graph.microsoft.com/oidc/userinfo",
Suggestion importance[1-10]: 9

__

Why: This is a critical bug fix. The Microsoft Graph API endpoint for user information is https://graph.microsoft.com/oidc/userinfo, not a tenant-specific URL, and using the wrong endpoint would cause profile fetching to fail.

High

@dbarrosop dbarrosop changed the title fix: treat provider AzureAD correctly feat: add EntraID in favor of AzureAD and restore old behavior Aug 26, 2025
@dbarrosop dbarrosop closed this Aug 26, 2025
@dbarrosop dbarrosop reopened this Aug 26, 2025
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 3149fbb

@dbarrosop dbarrosop changed the title feat: add EntraID in favor of AzureAD and restore old behavior feat: added EntraID in favor of AzureAD and restore old behavior Aug 26, 2025
@dbarrosop dbarrosop changed the title feat: added EntraID in favor of AzureAD and restore old behavior feat: added EntraID in favor of AzureAD and restored old behavior for AzureAD Aug 26, 2025
@dbarrosop
dbarrosop merged commit 3672af6 into main Aug 26, 2025
12 checks passed
@dbarrosop
dbarrosop deleted the azureadbug branch August 26, 2025 13:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants