Environment Variable for Authentication Token Refactor

[amanjeev]

For important environment variables like CRITICALUP_TOKEN we should use a separate struct as a technique so as to avoid having to call std::env::var at multiple places. This can help with localizing errors and testing.

This does not change any functionality.

[Hoverbear]

@amanjeev is this ready for review? Can you give it a meaningful title and description if so?

[amanjeev]

@Hoverbear It is ready for review now.

[Hoverbear]

Paths does a similar thing (configurs some values based on env vars) here:

criticalup/crates/criticalup-core/src/config/paths.rs

Lines 12 to 21 in 2363ed2

	pub struct Paths {
	pub(crate) state_file: PathBuf,
	pub proxies_dir: PathBuf,
	pub installation_dir: PathBuf,
	pub cache_dir: PathBuf,
	#[cfg(test)]
	pub(crate) root: PathBuf,
	}

Specifically here:

criticalup/crates/criticalup-core/src/config/paths.rs

Lines 57 to 63 in 2363ed2

	fn find_root(whitelabel: &WhitelabelConfig) -> Option<PathBuf> {
	match env::var_os("CRITICALUP_ROOT") {
	Some(val) if val.is_empty() => platform_specific_root(whitelabel),
	Some(val) => Some(PathBuf::from(val)),
	None => platform_specific_root(whitelabel),
	}
	}

As well as:

criticalup/crates/criticalup-core/src/config/paths.rs

Lines 69 to 75 in 2363ed2

	fn find_cache_dir(whitelabel: &WhitelabelConfig) -> Option<PathBuf> {
	match env::var_os("CRITICALUP_CACHE_DIR") {
	Some(val) if val.is_empty() => platform_specific_cache_dir(whitelabel),
	Some(val) => Some(PathBuf::from(val)),
	None => platform_specific_cache_dir(whitelabel),
	}
	}

This makes me wonder if Paths should be configured with the instance of these EnvVars instead, or if the root/cache_dir passed to it should not be options, but instead PathBufs acquired from EnvVars somehow. After all, CRITICALUP_CACHE_DIR is indeed an env var, and if we ever want to test that feature having it pull from env::var will be very problematic.

[amanjeev]

Yes it makes sense to me to include these in EnvVars.

[Hoverbear]

I'm not sure EnvVars::default() resulting in a struct purely made of None values is what a programmer expects. Looking at EnvVars as a type name I would assume that EnvVars::default() would pull from the relevant environment. So I'd kind of assume it'd do the same thing as EnvVars::read().

As for an actually empty struct, I'd assume there would be an EnvVars::empty() I could call if I really needed (for testing etc)... I also think EnvVars::empty() could be a const fn and thus called in the const ENV_VARS: EnvVars = EnvVars::empty() below?

[amanjeev]

I think one of my issues was that I need to return an error when the string is not Unicode.

[Hoverbear]

This kind of muddies the waters on what State is. (In fact, having token_path here also muddies it, maybe we should consider removing the sole use of token_path here and find more clear logic).

At least from my reading, State represents what is on disk, yet now state.authentication_token(...) might return me the value from an env var instead of my state file (... or some random file path I passed in!?), and it requires me to pass in EnvVars.

Seems to me the logic around getting what the canonical token is should be something like:

let env = EnvVars::read();
let state = State::load();

// Determine the canonical auth token
// Having the env var set is higher priority than the on-disk
let auth_token = if let Some(env_token) = env.authentication_token() {
  env_token
} else if let Some(state_token) = state.authentication_token() {
  state_token
}

println!("The token is {}", auth_token);

[Hoverbear]

Noted elsewhere but this is the only usage of the token file path in our codebase, it creates some very confusing logic and perhaps we should refactor it out and have EnvVars intelligently detect if it's inside docker and look at /run/secrets when it's being built.