fix(config): read nested options such as OIDC__CLIENT_ID from environment variables

README.md

@@ -65,6 +65,8 @@ The public instance's privacy policy is in [PRIVACY.md](./PRIVACY.md).
 
 Set options in a `config.toml` in the server's working directory, or as
 environment variables (case-insensitive, e.g. `ALLOW_REGISTRATION=false`).
+Environment variables override `config.toml`. For options inside a section, join
+the names with `__`, e.g. `OIDC__CLIENT_ID` for `client_id` in `[oidc]`.
 [config.dev.toml](./config.dev.toml) is an annotated example.
 
 | Option                        | Description                                                                          | Default      |
@@ -156,9 +158,13 @@ The server never migrates an existing database on its own. To upgrade:
 
 ### OIDC
 
-Add an `[oidc]` section to `config.toml` to let users sign in through an
-OpenID Connect provider. All options except `allowed_redirect_urls` are
-required.
+Add an `[oidc]` section to `config.toml`, or set the matching `OIDC__*`
+environment variables, to let users sign in through an OpenID Connect provider.
+All options except `allowed_redirect_urls` are required. As an environment
+variable, `allowed_redirect_urls` takes a comma-separated list, e.g.
+`OIDC__ALLOWED_REDIRECT_URLS=myapp://oidc,https://app.example.com/oidc`.
+With the Compose quick start, add the variables to the service's `environment`
+list. Compose doesn't pass entries in `.env` to the container on its own.
 
 | Option                  | Description                                              | Example                    |
 | ----------------------- | -------------------------------------------------------- | -------------------------- |

src/config.rs

@@ -32,10 +32,31 @@ pub struct OidcConfig {
     pub app_url: String,
     /// Client URLs the OIDC flows may redirect back to. Login passes the session
     /// token to this URL, so only list clients you trust.
-    #[serde(default)]
+    #[serde(default, deserialize_with = "string_list")]
     pub allowed_redirect_urls: Vec<String>,
 }
 
+/// Accepts a list, or a comma-separated string so that lists can be set
+/// through environment variables.
+fn string_list<'de, D: serde::Deserializer<'de>>(deserializer: D) -> Result<Vec<String>, D::Error> {
+    #[derive(serde::Deserialize)]
+    #[serde(untagged)]
+    enum StringList {
+        List(Vec<String>),
+        Joined(String),
+    }
+
+    Ok(match serde::Deserialize::deserialize(deserializer)? {
+        StringList::List(list) => list,
+        StringList::Joined(joined) => joined
+            .split(',')
+            .map(str::trim)
+            .filter(|item| !item.is_empty())
+            .map(str::to_owned)
+            .collect(),
+    })
+}
+
 #[derive(serde::Deserialize, Clone)]
 pub struct Config {
     #[serde(rename = "secret_key")]
@@ -81,7 +102,7 @@ impl Config {
     pub fn build() -> Result<Config, ConfigError> {
         config::Config::builder()
             .add_source(config::File::with_name("config").required(false))
-            .add_source(config::Environment::with_convert_case(config::Case::Snake))
+            .add_source(environment())
             .build()?
             .try_deserialize()
     }
@@ -160,6 +181,12 @@ impl Config {
     }
 }
 
+/// Environment variable source. Nested options use `__` as the separator,
+/// e.g. `OIDC__CLIENT_ID` for `oidc.client_id`.
+fn environment() -> config::Environment {
+    config::Environment::with_convert_case(config::Case::Snake).separator("__")
+}
+
 /// Invalid config errors.
 #[derive(thiserror::Error, Debug, PartialEq, Eq)]
 pub enum ConfigValidationError {
@@ -201,7 +228,7 @@ fn validate_secret(name: &'static str, secret: &str) -> Result<(), ConfigValidat
 
 #[cfg(test)]
 mod tests {
-    use super::{Config, ConfigValidationError, OidcConfig, validate_secret};
+    use super::{Config, ConfigValidationError, OidcConfig, environment, validate_secret};
 
     fn config_with(secret: &str, username_secret: Option<&str>) -> Config {
         Config {
@@ -334,4 +361,86 @@ mod tests {
         });
         assert!(config.validate().is_ok());
     }
+
+    /// Builds a config from the given variables instead of the process
+    /// environment, so tests cannot race on shared state.
+    fn config_from_env(vars: &[(&str, &str)]) -> Config {
+        let vars = vars
+            .iter()
+            .map(|(key, value)| ((*key).to_owned(), (*value).to_owned()))
+            .collect();
+        config::Config::builder()
+            .add_source(environment().source(Some(vars)))
+            .build()
+            .unwrap()
+            .try_deserialize()
+            .unwrap()
+    }
+
+    #[test]
+    fn environment_variables_configure_nested_oidc_options() {
+        let config = config_from_env(&[
+            ("SECRET_KEY", STRONG),
+            ("DATABASE_URL", "./data/db.sqlite"),
+            ("ALLOW_REGISTRATION", "false"),
+            ("TRUSTED_PROXY_HOPS", "2"),
+            ("OIDC__PROVIDER_URL", "https://auth.example.com"),
+            ("OIDC__CLIENT_ID", "sync-server"),
+            ("OIDC__CLIENT_SECRET", STRONG),
+            ("OIDC__APP_URL", "https://sync.example.com"),
+            (
+                "OIDC__ALLOWED_REDIRECT_URLS",
+                "myapp://oidc, https://app.example.com/oidc",
+            ),
+        ]);
+
+        assert_eq!(config.secret, STRONG);
+        assert_eq!(config.database_url, "./data/db.sqlite");
+        assert!(!config.allow_registration);
+        assert_eq!(config.trusted_proxy_hops, 2);
+        let oidc = config.oidc.expect("OIDC__* variables should set `oidc`");
+        assert_eq!(oidc.provider_url, "https://auth.example.com");
+        assert_eq!(oidc.client_id, "sync-server");
+        assert_eq!(oidc.client_secret, STRONG);
+        assert_eq!(oidc.app_url, "https://sync.example.com");
+        assert_eq!(
+            oidc.allowed_redirect_urls,
+            ["myapp://oidc", "https://app.example.com/oidc"]
+        );
+    }
+
+    /// Variables override single options of a `config.toml` section, and the
+    /// file's TOML array form for lists keeps working.
+    #[test]
+    fn environment_variables_override_the_config_file() {
+        let file = r#"
+            secret_key = "from-file"
+            database_url = "./db.sqlite"
+
+            [oidc]
+            provider_url = "https://auth.example.com"
+            client_id = "from-file"
+            client_secret = "from-file"
+            app_url = "https://sync.example.com"
+            allowed_redirect_urls = ["myapp://oidc", "https://app.example.com/oidc"]
+        "#;
+        let vars = [("OIDC__CLIENT_ID".to_owned(), "from-env".to_owned())]
+            .into_iter()
+            .collect();
+        let config: Config = config::Config::builder()
+            .add_source(config::File::from_str(file, config::FileFormat::Toml))
+            .add_source(environment().source(Some(vars)))
+            .build()
+            .unwrap()
+            .try_deserialize()
+            .unwrap();
+
+        let oidc = config.oidc.unwrap();
+        assert_eq!(oidc.client_id, "from-env");
+        assert_eq!(oidc.client_secret, "from-file");
+        assert_eq!(
+            oidc.allowed_redirect_urls,
+            ["myapp://oidc", "https://app.example.com/oidc"]
+        );
+    }
 }

src/rate_limit.rs

@@ -104,6 +104,9 @@ impl RateLimiter {
     }
 
     /// Record a request from `client` and report whether it is permitted.
+    ///
+    /// Every attempt counts, including failed ones. Counting only successful
+    /// requests would leave failed logins, and so password guessing, unlimited.
     pub fn check(&self, client: IpAddr) -> bool {
         let now = Instant::now();
         let mut windows = self.windows.lock().expect("rate limiter mutex poisoned");