Fix invalid authority uri

AzureAD · Nov 6, 2024 · d00ce0d · d00ce0d
1 parent 06d3fb2
commit d00ce0d
Show file tree

Hide file tree

Showing 2 changed files with 51 additions and 9 deletions.
diff --git a/apps/internal/oauth/ops/authority/authority.go b/apps/internal/oauth/ops/authority/authority.go
@@ -47,12 +47,12 @@ type jsonCaller interface {
 }
 
 var aadTrustedHostList = map[string]bool{
-	"login.windows.net":                      true, // Microsoft Azure Worldwide - Used in validation scenarios where host is not this list
-	"login.partner.microsoftonline.cn":       true, // Microsoft Azure China
-	"login.microsoftonline.de":               true, // Microsoft Azure Blackforest
-	"login-us.microsoftonline.com":           true, // Microsoft Azure US Government - Legacy
-	"login.microsoftonline.us":               true, // Microsoft Azure US Government
-	"login.microsoftonline.com":              true, // Microsoft Azure Worldwide
+	"login.windows.net":                true, // Microsoft Azure Worldwide - Used in validation scenarios where host is not this list
+	"login.partner.microsoftonline.cn": true, // Microsoft Azure China
+	"login.microsoftonline.de":         true, // Microsoft Azure Blackforest
+	"login-us.microsoftonline.com":     true, // Microsoft Azure US Government - Legacy
+	"login.microsoftonline.us":         true, // Microsoft Azure US Government
+	"login.microsoftonline.com":        true, // Microsoft Azure Worldwide
 }
 
 // TrustedHost checks if an AAD host is trusted/valid.
@@ -358,7 +358,16 @@ type Info struct {
 
 // NewInfoFromAuthorityURI creates an AuthorityInfo instance from the authority URL provided.
 func NewInfoFromAuthorityURI(authority string, validateAuthority bool, instanceDiscoveryDisabled bool) (Info, error) {
-	u, err := url.Parse(strings.ToLower(authority))
+
+	cannonicalAuthority := authority
+
+	// suffix authority with / if it doesn't have one
+	if !strings.HasSuffix(authority, "/") {
+		cannonicalAuthority += "/"
+	}
+
+	u, err := url.Parse(strings.ToLower(cannonicalAuthority))
+
 	if err != nil {
 		return Info{}, fmt.Errorf("couldn't parse authority url: %w", err)
 	}
@@ -376,7 +385,7 @@ func NewInfoFromAuthorityURI(authority string, validateAuthority bool, instanceD
 	case "adfs":
 		authorityType = ADFS
 	case "dstsv2":
-		if len(pathParts) != 3 {
+		if len(pathParts) != 4 {
 			return Info{}, fmt.Errorf("dSTS authority must be an https URL such as https://<authority>/dstsv2/%s", DSTSTenant)
 		}
 		if pathParts[2] != DSTSTenant {
@@ -392,7 +401,7 @@ func NewInfoFromAuthorityURI(authority string, validateAuthority bool, instanceD
 	// u.Host includes the port, if any, which is required for private cloud deployments
 	return Info{
 		Host:                      u.Host,
-		CanonicalAuthorityURI:     authority,
+		CanonicalAuthorityURI:     cannonicalAuthority,
 		AuthorityType:             authorityType,
 		ValidateAuthority:         validateAuthority,
 		Tenant:                    tenant,

diff --git a/apps/internal/oauth/ops/authority/authority_test.go b/apps/internal/oauth/ops/authority/authority_test.go
@@ -320,6 +320,39 @@ func TestCreateAuthorityInfoFromAuthorityUri(t *testing.T) {
 	}
 }
 
+func TestAuthorityParsing(t *testing.T) {
+
+	dSTSWithSlash := fmt.Sprintf("https://dstsv2.example.com/dstsv2/%s/", DSTSTenant)
+	dSTSNoSlash := fmt.Sprintf("https://dstsv2.example.com/dstsv2/%s", DSTSTenant)
+
+	tests := map[string]struct {
+		authority, expectedType, expectedCannonical, expectedTenant string
+	}{
+		"AAD with slash":     {"https://login.microsoftonline.com/common/", "MSSTS", "https://login.microsoftonline.com/common/", "common"},
+		"AAD without slash":  {"https://login.microsoftonline.com/common", "MSSTS", "https://login.microsoftonline.com/common/", "common"},
+		"ADFS with slash":    {"https://adfs.example.com/adfs/", "ADFS", "https://adfs.example.com/adfs/", ""},
+		"ADFS without slash": {"https://adfs.example.com/adfs", "ADFS", "https://adfs.example.com/adfs/", ""},
+		"dSTS with slash":    {dSTSWithSlash, "DSTS", dSTSWithSlash, DSTSTenant},
+		"dSTS without slash": {dSTSNoSlash, "DSTS", dSTSWithSlash, DSTSTenant},
+	}
+
+	for name, test := range tests {
+		actual, err := NewInfoFromAuthorityURI(test.authority, false, false)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if actual.AuthorityType != test.expectedType {
+			t.Fatalf("%s: unexpected authority type %s", name, actual.AuthorityType)
+		}
+		if actual.CanonicalAuthorityURI != test.expectedCannonical {
+			t.Fatalf("%s: unexpected canonical authority %s", name, actual.CanonicalAuthorityURI)
+		}
+		if actual.Tenant != test.expectedTenant {
+			t.Fatalf("%s: unexpected tenant %s", name, actual.Tenant)
+		}
+	}
+}
+
 func TestAuthParamsWithTenant(t *testing.T) {
 	uuid1 := uuid.New().String()
 	uuid2 := uuid.New().String()