From 337c745bced8776e3226b1db6a3361566e67561f Mon Sep 17 00:00:00 2001
From: Ratchanan Srirattanamet <peathot@hotmail.com>
Date: Sat, 22 Apr 2023 17:40:09 +0000
Subject: [PATCH 1/2] Improve error messages for common errors found in a new
 setup

These errors usually comes from a mistake in setting up Azure AD
application. Shows more useful messages (and sometimes a remedy) in
the log to help in debugging.
---
 internal/aad/aad.go       | 19 ++++++++++++++++---
 internal/aad/aad_test.go  |  2 ++
 internal/aad/msal_mock.go | 28 ++++++++++++++++++++++++++--
 3 files changed, 44 insertions(+), 5 deletions(-)

diff --git a/internal/aad/aad.go b/internal/aad/aad.go
index e507684b..f6490157 100644
--- a/internal/aad/aad.go
+++ b/internal/aad/aad.go
@@ -17,9 +17,11 @@ import (
 const (
 	endpoint = "https://login.microsoftonline.com"
 
-	invalidCredCode = 50126
-	requiresMFACode = 50076
-	noSuchUserCode  = 50034
+	invalidCredCode    = 50126
+	requiresMFACode    = 50076
+	noSuchUserCode     = 50034
+	noConsentCode      = 65001
+	noClientSecretCode = 7000218
 )
 
 var (
@@ -86,6 +88,17 @@ func (auth AAD) Authenticate(ctx context.Context, cfg config.AAD, username, pass
 				logger.Debug(ctx, "Authentication successful even if requiring MFA")
 				return nil
 			}
+			if errcode == noConsentCode {
+				logger.Err(ctx, "Azure AD application requires consent, either from tenant, or from user. "+
+					"If you're a tenant's administrator, go to: %s/adminconsent?client_id=%s",
+					authority, cfg.AppID)
+				return ErrDeny
+			}
+			if errcode == noClientSecretCode {
+				logger.Err(ctx, "Azure AD application requires enabling 'Allow public client flows'. "+
+					"https://learn.microsoft.com/en-us/azure/active-directory/develop/scenario-desktop-app-registration#redirect-uris")
+				return ErrDeny
+			}
 		}
 		logger.Err(ctx, "Unknown error code(s) from server: %v", addErrWithCodes.ErrorCodes)
 		return ErrDeny
diff --git a/internal/aad/aad_test.go b/internal/aad/aad_test.go
index 1643cb0c..82343eb9 100644
--- a/internal/aad/aad_test.go
+++ b/internal/aad/aad_test.go
@@ -24,6 +24,8 @@ func TestAuthenticate(t *testing.T) {
 
 		// error cases
 		"can't connect to authority": {appID: "connection failed", wantErr: aad.ErrNoNetwork},
+		"public client disallowed":   {appID: "public client disallowed", wantErr: aad.ErrDeny},
+		"no tenant-wide consent":     {appID: "no tenant-wide consent", wantErr: aad.ErrDeny},
 		"unreadable server response": {username: "unreadable server response", wantErr: aad.ErrDeny},
 		"invalid server response":    {username: "invalid server response", wantErr: aad.ErrDeny},
 		"invalid credentials":        {username: "invalid credentials", wantErr: aad.ErrDeny},
diff --git a/internal/aad/msal_mock.go b/internal/aad/msal_mock.go
index 3350d317..866e5c7d 100644
--- a/internal/aad/msal_mock.go
+++ b/internal/aad/msal_mock.go
@@ -21,17 +21,31 @@ func NewWithMockClient() AAD {
 
 func publicNewMockClient(clientID string, _ ...public.Option) (publicClient, error) {
 	var forceOffline bool
+	var publicClientDisallowed bool
+	var noTenantWideConsent bool
+
 	switch clientID {
 	case "connection failed":
 		return publicClientMock{}, errors.New("connection failed")
 	case "force offline":
 		forceOffline = true
+	case "public client disallowed":
+		publicClientDisallowed = true
+	case "no tenant-wide consent":
+		noTenantWideConsent = true
 	}
-	return publicClientMock{forceOffline: forceOffline}, nil
+
+	return publicClientMock{
+		forceOffline:           forceOffline,
+		publicClientDisallowed: publicClientDisallowed,
+		noTenantWideConsent:    noTenantWideConsent,
+	}, nil
 }
 
 type publicClientMock struct {
-	forceOffline bool
+	forceOffline           bool
+	publicClientDisallowed bool
+	noTenantWideConsent    bool
 }
 
 func (m publicClientMock) AcquireTokenByUsernamePassword(_ context.Context, _ []string, username string, _ string, _ ...public.AcquireByUsernamePasswordOption) (public.AuthResult, error) {
@@ -44,6 +58,16 @@ func (m publicClientMock) AcquireTokenByUsernamePassword(_ context.Context, _ []
 		return r, fmt.Errorf("Offline")
 	}
 
+	if m.publicClientDisallowed {
+		callErr.Resp.Body = io.NopCloser(strings.NewReader(fmt.Sprintf("{\"error_codes\": [%d]}", noClientSecretCode)))
+		return r, callErr
+	}
+
+	if m.noTenantWideConsent {
+		callErr.Resp.Body = io.NopCloser(strings.NewReader(fmt.Sprintf("{\"error_codes\": [%d]}", noConsentCode)))
+		return r, callErr
+	}
+
 	switch username {
 	case "success@domain.com":
 	case "success@otherdomain.com":

From b1f06b7ff8c5e1ff9a333c40604d2c22daca75bd Mon Sep 17 00:00:00 2001
From: Ratchanan Srirattanamet <peathot@hotmail.com>
Date: Sat, 22 Apr 2023 17:47:12 +0000
Subject: [PATCH 2/2] Show link(s) to error code(s)' description on Microsoft's
 site

This makes the life easier when debugging.
---
 internal/aad/aad.go | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/internal/aad/aad.go b/internal/aad/aad.go
index f6490157..c1a2385b 100644
--- a/internal/aad/aad.go
+++ b/internal/aad/aad.go
@@ -101,6 +101,12 @@ func (auth AAD) Authenticate(ctx context.Context, cfg config.AAD, username, pass
 			}
 		}
 		logger.Err(ctx, "Unknown error code(s) from server: %v", addErrWithCodes.ErrorCodes)
+
+		logger.Debug(ctx, "For more information about the error code(s), see:")
+		for _, errcode := range addErrWithCodes.ErrorCodes {
+			logger.Debug(ctx, "- Error code %d: https://login.microsoftonline.com/error?code=%d", errcode, errcode)
+		}
+
 		return ErrDeny
 	}