allow storing client_id and client_secret in coman config for envs without keyring

[Panaetius]

PR Type

Enhancement

---

Description

• Added support for storing CSCS client credentials in config

• Enhanced authentication flow to fallback to keyring when config values missing

• Improved configuration flexibility for environments without keyring

• Updated token retrieval logic to prioritize config over keyring

---

Diagram Walkthrough

flowchart LR
  A["Config::new()"] -- "loads cscs config" --> B["CscsConfig"]
  B -- "client_id" --> C["Secret::from_config"]
  B -- "client_secret" --> D["Secret::from_config"]
  C -- "fallback to keyring" --> E["get_secret(CLIENT_ID_SECRET_NAME)"]
  D -- "fallback to keyring" --> F["get_secret(CLIENT_SECRET_SECRET_NAME)"]
  E -- "or None" --> G["Err('not logged in')"]
  F -- "or None" --> G

Loading

File Walkthrough

Relevant files

Enhancement

config.rs Add CSCS client credential fields to config                            coman/src/config.rs Added client_id field to CscsConfig struct Added client_secret field to CscsConfig struct Both fields are optional and serde-defaulted +4/-0      handlers.rs Update auth flow to use config credentials first                  coman/src/cscs/handlers.rs Modified get_access_token to check config before keyring Added fallback logic using config.values.cscs.client_id Added fallback logic using config.values.cscs.client_secret Maintained existing keyring lookup behavior as fallback +22/-10

---

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Fallback Logic The fallback logic for retrieving client credentials prioritizes config values over keyring, but the error handling could be improved to provide more context when both sources fail. let client_id = config .values .cscs .client_id .map(Secret) .unwrap_or(match get_secret(CLIENT_ID_SECRET_NAME).await { Ok(Some(client_id)) => client_id, Ok(None) => Err(eyre!("not logged in"))?, Err(e) => Err(e)?, }); let client_secret = config .values .cscs .client_secret .map(Secret) .unwrap_or(match get_secret(CLIENT_SECRET_SECRET_NAME).await { Ok(Some(client_secret)) => client_secret, Ok(None) => Err(eyre!("not logged in"))?, Err(e) => Err(e)?, }); Configuration Fields The addition of client_id and client_secret as optional fields in CscsConfig allows for configuration-based storage, but the implications of this change on existing configurations should be considered. #[serde(default)] pub client_id: Option<String>, #[serde(default)] pub client_secret: Option<String>,

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Reduce code duplication in secret retrieval Consider extracting the secret retrieval logic into a helper function to reduce code duplication and improve readability. The repeated pattern for fetching client_id and client_secret can be simplified. coman/src/cscs/handlers.rs [60-81] let config = Config::new()?; -let client_id = config - .values - .cscs - .client_id - .map(Secret) - .unwrap_or(match get_secret(CLIENT_ID_SECRET_NAME).await { - Ok(Some(client_id)) => client_id, - Ok(None) => Err(eyre!("not logged in"))?, - Err(e) => Err(e)?, - }); -let client_secret = - config - .values - .cscs - .client_secret +let get_secret_or_fallback = |secret_name: &str, config_value: Option<String>| { + config_value .map(Secret) - .unwrap_or(match get_secret(CLIENT_SECRET_SECRET_NAME).await { - Ok(Some(client_secret)) => client_secret, - Ok(None) => Err(eyre!("not logged in"))?, - Err(e) => Err(e)?, - }); + .unwrap_or_else(|| async move { + match get_secret(secret_name).await { + Ok(Some(secret)) => secret, + Ok(None) => Err(eyre!("not logged in"))?, + Err(e) => Err(e)?, + } + }) +}; +let client_id = get_secret_or_fallback(CLIENT_ID_SECRET_NAME, config.values.cscs.client_id); +let client_secret = get_secret_or_fallback(CLIENT_SECRET_SECRET_NAME, config.values.cscs.client_secret); Suggestion importance[1-10]: 7 __ Why: The suggestion proposes extracting duplicated secret retrieval logic into a helper function, which improves readability and maintainability. However, the implementation uses an async closure which introduces complexity and may not be necessary for this use case.	Medium
Possible issue	Improve error handling for secret retrieval The current implementation may cause issues if get_secret returns an error when the secret doesn't exist. Ensure that the error handling properly distinguishes between a missing secret and a genuine error during retrieval. coman/src/cscs/handlers.rs [61-70] -let client_id = config - .values - .cscs - .client_id - .map(Secret) - .unwrap_or(match get_secret(CLIENT_ID_SECRET_NAME).await { +let client_id = match config.values.cscs.client_id { + Some(id) => Secret(id), + None => match get_secret(CLIENT_ID_SECRET_NAME).await { Ok(Some(client_id)) => client_id, Ok(None) => Err(eyre!("not logged in"))?, Err(e) => Err(e)?, - }); + }, +}; Suggestion importance[1-10]: 5 __ Why: The suggestion identifies a potential issue with error handling but provides a less clear improvement over the existing code. The refactored version is slightly more verbose without significantly improving clarity or robustness.	Low

[github-actions]

The serde(default) attribute should be applied to all fields that can be missing from the config file.

#ai-review-inline

[github-actions]

Consider adding documentation to explain the purpose of client_id.

#ai-review-inline

[github-actions]

Consider adding documentation to explain the purpose of client_secret.

#ai-review-inline

[github-actions]

The serde(default) attribute should be applied to all fields that can be missing from the config file.

#ai-review-inline

[github-actions]

Repeated pattern for client_secret; consider refactoring into a helper function.

Suggested change

	.unwrap_or(match get_secret(CLIENT_SECRET_SECRET_NAME).await {
	.unwrap_or_else(|| get_secret(CLIENT_SECRET_SECRET_NAME).await.unwrap_or_else(|e| Err(e)?))

#ai-review-inline

[github-actions]

Consider extracting config loading into a separate function for better readability and testability.

Suggested change

	let config = Config::new()?;
	let config = Config::new()?;

#ai-review-inline

[github-actions]

The unwrap_or pattern can be simplified by using or with a closure for cleaner logic.

Suggested change

	.unwrap_or(match get_secret(CLIENT_ID_SECRET_NAME).await {
	.unwrap_or_else(|| get_secret(CLIENT_ID_SECRET_NAME).await.unwrap_or_else(|e| Err(e)?))

#ai-review-inline