refactor: oidc provider configuration

Fixes the issue where discovery urls where assembled incorrectly.
Also removes the default values for unbranded providers, since there is no sane default that can be provided.

Author: Jeidnx

src/service/oauth/providers.rs

@@ -3,7 +3,7 @@ use std::collections::BTreeMap;
 use serde_json::{Map as JsonObject, Value as JsonValue};
 use tokio::sync::RwLock;
 pub use tuwunel_core::config::IdentityProvider as Provider;
-use tuwunel_core::{Err, Result, debug, debug::INFO_SPAN_LEVEL, err, implement};
+use tuwunel_core::{Err, Result, debug, debug::INFO_SPAN_LEVEL, implement};
 use url::Url;
 
 use crate::SelfServices;
@@ -123,103 +123,154 @@ async fn configure(&self, mut provider: Provider) -> Result<Provider> {
 		.name
 		.get_or_insert_with(|| provider.brand.clone());
 
+	if provider.brand == "github" {
+		return configure_github(provider);
+	}
+
 	if provider.issuer_url.is_none() {
-		_ = provider
-			.issuer_url
-			.replace(match provider.brand.as_str() {
-				| "github" => "https://github.com".try_into()?,
-				| "gitlab" => "https://gitlab.com".try_into()?,
-				| "google" => "https://accounts.google.com".try_into()?,
-				| _ => return Err!(Config("issuer_url", "Required for this provider.")),
-			});
+		provider.issuer_url = Some(match provider.brand.as_str() {
+			| "gitlab" => "https://gitlab.com".try_into()?,
+			| "google" => "https://accounts.google.com".try_into()?,
+			| _ => return Err!(Config("issuer_url", "Required for this provider.")),
+		});
 	}
 
-	if provider.base_path.is_none() {
-		provider.base_path = match provider.brand.as_str() {
-			| "github" => Some("/login/oauth".to_owned()),
-			| _ => None,
-		};
+	if !provider.discovery {
+		assert_manual_urls(&provider)?;
+		return Ok(provider);
 	}
 
-	let response = self
-		.discover(&provider)
-		.await
-		.and_then(|response| {
-			response.as_object().cloned().ok_or_else(|| {
-				err!(Request(NotJson("Expecting JSON object for discovery response")))
-			})
-		})
-		.and_then(|response| check_issuer(response, &provider))?;
+	let discovery_response = {
+		let response = self.discover(&provider).await?;
+		let Some(response_map) = response.as_object() else {
+			return Err!(Request(NotJson("Expecting JSON object for discovery response")));
+		};
+
+		check_issuer(response_map, &provider)?;
+
+		response_map.to_owned()
+	};
 
 	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 = Some(assert_and_parse_url(
+			provider.id(),
+			&discovery_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 = Some(assert_and_parse_url(
+			provider.id(),
+			&discovery_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 = Some(assert_and_parse_url(
+			provider.id(),
+			&discovery_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 =
+			Some(assert_and_parse_url(provider.id(), &discovery_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 =
+			Some(assert_and_parse_url(provider.id(), &discovery_response, "token_endpoint")?);
 	}
 
 	Ok(provider)
 }
 
+fn configure_github(mut provider: Provider) -> Result<Provider> {
+	// See: https://logto.io/oauth-providers-explorer/github
+	// TODO: find better (first-party?) documentation for these endpoints
+
+	provider.base_path = Some("/login/oauth".to_owned());
+	provider.authorization_url = Some(
+		"https://api.github.com/authorize"
+			.try_into()
+			.expect("valid url"),
+	);
+	provider.revocation_url = Some(
+		"https://api.github.com/revocation"
+			.try_into()
+			.expect("valid url"),
+	);
+	provider.introspection_url = Some(
+		"https://api.github.com/introspection"
+			.try_into()
+			.expect("valid url"),
+	);
+	provider.userinfo_url = Some(
+		"https://api.github.com/user"
+			.try_into()
+			.expect("valid url"),
+	);
+	provider.token_url = Some(
+		"https://api.github.com/access_token"
+			.try_into()
+			.expect("valid url"),
+	);
+
+	Ok(provider)
+}
+
+fn assert_manual_urls(provider: &Provider) -> Result<()> {
+	if provider.authorization_url.is_none() {
+		return Err!(Config(
+			"authorization_url",
+			"Required for provider {}, since discovery is disabled",
+			provider.client_id
+		));
+	}
+
+	if provider.revocation_url.is_none() {
+		return Err!(Config(
+			"revocation_url",
+			"Required for provider {}, since discovery is disabled",
+			provider.client_id
+		));
+	}
+
+	if provider.introspection_url.is_none() {
+		return Err!(Config(
+			"introspection_url",
+			"Required for provider {}, since discovery is disabled",
+			provider.client_id
+		));
+	}
+
+	if provider.userinfo_url.is_none() {
+		return Err!(Config(
+			"userinfo_url",
+			"Required for provider {}, since discovery is disabled",
+			provider.client_id
+		));
+	}
+
+	if provider.token_url.is_none() {
+		return Err!(Config(
+			"token_url",
+			"Required for provider {}, since discovery is disabled",
+			provider.client_id
+		));
+	}
+
+	Ok(())
+}
+
 /// Send a network request to a provider at the computed location of the
 /// `.well-known/openid-configuration`, returning the configuration.
 #[implement(Providers)]
 #[tracing::instrument(level = "debug", ret(level = "trace"), skip(self))]
-pub async fn discover(&self, provider: &Provider) -> Result<JsonValue> {
+async fn discover(&self, provider: &Provider) -> Result<JsonValue> {
 	self.services
 		.client
 		.oauth
@@ -235,33 +286,51 @@ 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 let Some(url) = &provider.discovery_url {
+		return Ok(url.to_owned());
+	}
+
+	let issuer = provider
+		.issuer_url
+		.as_ref()
+		.expect("issuer to be asserted before calling discover");
+
+	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(
-			"discovery_url",
-			"Failed to determine URL for discovery of provider {}",
+			"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
 /// in any response. todo: cryptographic validation is not yet implemented here.
-fn check_issuer(
-	response: JsonObject<String, JsonValue>,
-	provider: &Provider,
-) -> Result<JsonObject<String, JsonValue>> {
+fn check_issuer(response: &JsonObject<String, JsonValue>, provider: &Provider) -> Result<()> {
 	let expected = provider
 		.issuer_url
 		.as_ref()
@@ -279,23 +348,21 @@ fn check_issuer(
 		)));
 	}
 
-	Ok(response)
+	Ok(())
 }
 
-/// 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!(
+			"Error building oidc provider '{provider_id}': {url_name} is missing from openid \
+			 discovery response",
+		);
+	};
 
-	Ok(url)
+	Ok(url_value.as_str().unwrap_or_default().parse()?)
 }