fix: assemble oidc discovery url correctly

Also removes some of the default provider endpoints, since it doesn't make sense to have them for unknown brands.

Author: Jeidnx

src/service/oauth/providers.rs

@@ -152,64 +152,46 @@ async fn configure(&self, mut provider: Provider) -> Result<Provider> {
 		.and_then(|response| check_issuer(response, &provider))?;
 
 	if provider.authorization_url.is_none() {
-		response
-			.get("authorization_endpoint")
-			.and_then(JsonValue::as_str)
-			.map(Url::parse)
-			.transpose()?
-			.or_else(|| make_url(&provider, "/authorize").ok())
-			.map(|url| provider.authorization_url.replace(url));
+		provider.authorization_url = match provider.brand.as_str() {
+			| "github" => "https://api.github.com/authorize".try_into().ok(),
+			| _ =>
+				Some(assert_and_parse_url(provider.id(), &response, "authorization_endpoint")?),
+		};
 	}
 
 	if provider.revocation_url.is_none() {
-		response
-			.get("revocation_endpoint")
-			.and_then(JsonValue::as_str)
-			.map(Url::parse)
-			.transpose()?
-			.or_else(|| make_url(&provider, "/revocation").ok())
-			.map(|url| provider.revocation_url.replace(url));
+		provider.revocation_url = match provider.brand.as_str() {
+			| "github" => "https://api.github.com/revocation"
+				.try_into()
+				.ok(),
+			| _ => Some(assert_and_parse_url(provider.id(), &response, "revocation_endpoint")?),
+		};
 	}
 
 	if provider.introspection_url.is_none() {
-		response
-			.get("introspection_endpoint")
-			.and_then(JsonValue::as_str)
-			.map(Url::parse)
-			.transpose()?
-			.or_else(|| make_url(&provider, "/introspection").ok())
-			.map(|url| provider.introspection_url.replace(url));
+		provider.introspection_url = match provider.brand.as_str() {
+			| "github" => "https://api.github.com/introspection"
+				.try_into()
+				.ok(),
+			| _ =>
+				Some(assert_and_parse_url(provider.id(), &response, "introspection_endpoint")?),
+		};
 	}
 
 	if provider.userinfo_url.is_none() {
-		response
-			.get("userinfo_endpoint")
-			.and_then(JsonValue::as_str)
-			.map(Url::parse)
-			.transpose()?
-			.or_else(|| match provider.brand.as_str() {
-				| "github" => "https://api.github.com/user".try_into().ok(),
-				| _ => make_url(&provider, "/userinfo").ok(),
-			})
-			.map(|url| provider.userinfo_url.replace(url));
+		provider.userinfo_url = match provider.brand.as_str() {
+			| "github" => "https://api.github.com/user".try_into().ok(),
+			| _ => Some(assert_and_parse_url(provider.id(), &response, "userinfo_endpoint")?),
+		};
 	}
 
 	if provider.token_url.is_none() {
-		response
-			.get("token_endpoint")
-			.and_then(JsonValue::as_str)
-			.map(Url::parse)
-			.transpose()?
-			.or_else(|| {
-				let path = if provider.brand == "github" {
-					"/access_token"
-				} else {
-					"/token"
-				};
-
-				make_url(&provider, path).ok()
-			})
-			.map(|url| provider.token_url.replace(url));
+		provider.token_url = match provider.brand.as_str() {
+			| "github" => "https://api.github.com/access_token"
+				.try_into()
+				.ok(),
+			| _ => Some(assert_and_parse_url(provider.id(), &response, "token_endpoint")?),
+		};
 	}
 
 	Ok(provider)
@@ -235,25 +217,54 @@ pub async fn discover(&self, provider: &Provider) -> Result<JsonValue> {
 /// Compute the location of the `/.well-known/openid-configuration` based on the
 /// local provider config.
 fn discovery_url(provider: &Provider) -> Result<Url> {
-	let default_url = provider
-		.discovery
-		.then(|| make_url(provider, "/.well-known/openid-configuration"))
-		.transpose()?;
-
-	let Some(url) = provider
-		.discovery_url
-		.clone()
-		.filter(|_| provider.discovery)
-		.or(default_url)
-	else {
+	if !provider.discovery {
 		return Err!(Config(
 			"discovery_url",
-			"Failed to determine URL for discovery of provider {}",
+			"Provider has discovery disabled {}",
+			provider.id()
+		));
+	}
+
+	if let Some(url) = &provider.discovery_url {
+		return Ok(url.to_owned());
+	}
+
+	let issuer = provider
+		.issuer_url
+		.as_ref()
+		.expect("Can we assert this exists at this point?");
+
+	let issuer_path = issuer.path();
+
+	let ressource_path = ".well-known/openid-configuration";
+
+	let base_url = if issuer_path.ends_with('/') {
+		issuer.to_owned()
+	} else {
+		let mut url = issuer.to_owned();
+		url.set_path((issuer_path.to_owned() + "/").as_str());
+		url
+	};
+
+	let Some(base_path) = provider.base_path.as_ref() else {
+		return Ok(base_url.join(ressource_path)?);
+	};
+
+	if base_path.is_empty() {
+		return Err!(Config(
+			"base_path",
+			"Provider '{}' has an empty base_path. Remove the key or add a value",
 			provider.id()
 		));
+	}
+
+	let base_path = if base_path.ends_with("/") {
+		base_path.to_owned()
+	} else {
+		base_path.to_owned() + "/"
 	};
 
-	Ok(url)
+	Ok(base_url.join((base_path + ressource_path).as_ref())?)
 }
 
 /// Validate that the locally configured `issuer_url` matches the issuer claimed
@@ -282,20 +293,21 @@ fn check_issuer(
 	Ok(response)
 }
 
-/// Generate a full URL for a request to the idp based on the idp's derived
-/// configuration.
-fn make_url(provider: &Provider, path: &str) -> Result<Url> {
-	let mut suffix = provider.base_path.clone().unwrap_or_default();
-
-	suffix.push_str(path);
-	let url = provider
-		.issuer_url
-		.as_ref()
-		.ok_or_else(|| {
-			let id = &provider.client_id;
-			err!(Config("issuer_url", "Provider {id:?} required field"))
-		})?
-		.join(&suffix)?;
+/// Assert that a url exists in the response and parse it
+fn assert_and_parse_url(
+	provider_id: &str,
+	response_map: &serde_json::Map<String, serde_json::Value>,
+	url_name: &str,
+) -> Result<Url> {
+	let Some(url_value) = response_map.get(url_name) else {
+		return Err(tuwunel_core::Error::Err(
+			format!(
+				"Error building oidc provider '{}': {} is missing from openid discovery response",
+				provider_id, url_name,
+			)
+			.into(),
+		));
+	};
 
-	Ok(url)
+	Ok(url_value.as_str().unwrap_or_default().parse()?)
 }