fix(providers): tighten refresh authorization and collisions

Author: johntmyers

crates/openshell-cli/src/run.rs

@@ -4079,15 +4079,27 @@ pub async fn provider_create(
 }
 
 fn provider_profile_allows_refresh_bootstrap(profile: &ProviderProfile) -> bool {
-    profile.credentials.iter().any(|credential| {
-        credential.refresh.as_ref().is_some_and(|refresh| {
-            matches!(
-                ProviderCredentialRefreshStrategy::try_from(refresh.strategy),
-                Ok(ProviderCredentialRefreshStrategy::Oauth2ClientCredentials
-                    | ProviderCredentialRefreshStrategy::GoogleServiceAccountJwt)
-            )
+    let required_credentials = profile
+        .credentials
+        .iter()
+        .filter(|credential| credential.required)
+        .collect::<Vec<_>>();
+    !required_credentials.is_empty()
+        && required_credentials.iter().all(|credential| {
+            credential
+                .refresh
+                .as_ref()
+                .is_some_and(|refresh| is_gateway_mintable_refresh_strategy(refresh.strategy))
         })
-    })
+}
+
+fn is_gateway_mintable_refresh_strategy(strategy: i32) -> bool {
+    matches!(
+        ProviderCredentialRefreshStrategy::try_from(strategy),
+        Ok(ProviderCredentialRefreshStrategy::Oauth2RefreshToken
+            | ProviderCredentialRefreshStrategy::Oauth2ClientCredentials
+            | ProviderCredentialRefreshStrategy::GoogleServiceAccountJwt)
+    )
 }
 
 pub async fn provider_get(server: &str, name: &str, tls: &TlsOptions) -> Result<()> {
@@ -6593,9 +6605,9 @@ mod tests {
         git_sync_files, http_health_check, image_requests_gpu, import_local_package_mtls_bundle,
         inferred_provider_type, package_managed_tls_dirs, parse_cli_setting_value,
         parse_credential_pairs, plaintext_gateway_is_remote, progress_step_from_metadata,
-        provisioning_timeout_message, ready_false_condition_message, refresh_status_header,
-        refresh_status_row, resolve_from, sandbox_should_persist, service_expose_status_error,
-        service_url_for_gateway,
+        provider_profile_allows_refresh_bootstrap, provisioning_timeout_message,
+        ready_false_condition_message, refresh_status_header, refresh_status_row, resolve_from,
+        sandbox_should_persist, service_expose_status_error, service_url_for_gateway,
     };
     use crate::TEST_ENV_LOCK;
     use hyper::StatusCode;
@@ -6614,7 +6626,8 @@ mod tests {
         PROGRESS_STEP_STARTING_SANDBOX,
     };
     use openshell_core::proto::{
-        Provider, ProviderCredentialRefreshStatus, ProviderCredentialRefreshStrategy,
+        Provider, ProviderCredentialRefresh, ProviderCredentialRefreshStatus,
+        ProviderCredentialRefreshStrategy, ProviderProfile, ProviderProfileCredential,
         SandboxCondition, SandboxStatus, datamodel::v1::ObjectMeta,
     };
 
@@ -6799,6 +6812,65 @@ mod tests {
         assert!(row.contains("..."));
     }
 
+    #[test]
+    fn refresh_bootstrap_requires_all_required_credentials_to_be_gateway_mintable() {
+        let refresh_token_profile = ProviderProfile {
+            credentials: vec![ProviderProfileCredential {
+                name: "MS_GRAPH_ACCESS_TOKEN".to_string(),
+                required: true,
+                refresh: Some(ProviderCredentialRefresh {
+                    strategy: ProviderCredentialRefreshStrategy::Oauth2RefreshToken as i32,
+                    ..Default::default()
+                }),
+                ..Default::default()
+            }],
+            ..Default::default()
+        };
+        assert!(provider_profile_allows_refresh_bootstrap(
+            &refresh_token_profile
+        ));
+
+        let mixed_static_profile = ProviderProfile {
+            credentials: vec![
+                ProviderProfileCredential {
+                    name: "ACCESS_TOKEN".to_string(),
+                    required: true,
+                    refresh: Some(ProviderCredentialRefresh {
+                        strategy: ProviderCredentialRefreshStrategy::Oauth2ClientCredentials as i32,
+                        ..Default::default()
+                    }),
+                    ..Default::default()
+                },
+                ProviderProfileCredential {
+                    name: "STATIC_API_KEY".to_string(),
+                    required: true,
+                    refresh: None,
+                    ..Default::default()
+                },
+            ],
+            ..Default::default()
+        };
+        assert!(!provider_profile_allows_refresh_bootstrap(
+            &mixed_static_profile
+        ));
+
+        let optional_refresh_profile = ProviderProfile {
+            credentials: vec![ProviderProfileCredential {
+                name: "OPTIONAL_TOKEN".to_string(),
+                required: false,
+                refresh: Some(ProviderCredentialRefresh {
+                    strategy: ProviderCredentialRefreshStrategy::GoogleServiceAccountJwt as i32,
+                    ..Default::default()
+                }),
+                ..Default::default()
+            }],
+            ..Default::default()
+        };
+        assert!(!provider_profile_allows_refresh_bootstrap(
+            &optional_refresh_profile
+        ));
+    }
+
     #[cfg(feature = "dev-settings")]
     #[test]
     fn parse_cli_setting_value_parses_bool_aliases() {

crates/openshell-server/src/auth/authz.rs

@@ -22,6 +22,9 @@ const ADMIN_METHODS: &[&str] = &[
     "/openshell.v1.OpenShell/CreateProvider",
     "/openshell.v1.OpenShell/UpdateProvider",
     "/openshell.v1.OpenShell/DeleteProvider",
+    "/openshell.v1.OpenShell/ConfigureProviderRefresh",
+    "/openshell.v1.OpenShell/RotateProviderCredential",
+    "/openshell.v1.OpenShell/DeleteProviderRefresh",
     // Global config and policy
     "/openshell.v1.OpenShell/UpdateConfig",
     // Draft policy approvals
@@ -77,10 +80,26 @@ const SCOPED_METHODS: &[(&str, &str)] = &[
     // provider:read
     ("/openshell.v1.OpenShell/GetProvider", "provider:read"),
     ("/openshell.v1.OpenShell/ListProviders", "provider:read"),
+    (
+        "/openshell.v1.OpenShell/GetProviderRefreshStatus",
+        "provider:read",
+    ),
     // provider:write
     ("/openshell.v1.OpenShell/CreateProvider", "provider:write"),
     ("/openshell.v1.OpenShell/UpdateProvider", "provider:write"),
     ("/openshell.v1.OpenShell/DeleteProvider", "provider:write"),
+    (
+        "/openshell.v1.OpenShell/ConfigureProviderRefresh",
+        "provider:write",
+    ),
+    (
+        "/openshell.v1.OpenShell/RotateProviderCredential",
+        "provider:write",
+    ),
+    (
+        "/openshell.v1.OpenShell/DeleteProviderRefresh",
+        "provider:write",
+    ),
     // config:read
     ("/openshell.v1.OpenShell/GetGatewayConfig", "config:read"),
     ("/openshell.v1.OpenShell/GetSandboxConfig", "config:read"),
@@ -500,6 +519,49 @@ mod tests {
         assert!(err.message().contains("sandbox:write"));
     }
 
+    #[test]
+    fn provider_refresh_methods_require_provider_scopes_and_admin_for_writes() {
+        let policy = scoped_policy();
+        let reader = identity_with_roles_and_scopes(&["openshell-user"], &["provider:read"]);
+        assert!(
+            policy
+                .check(&reader, "/openshell.v1.OpenShell/GetProviderRefreshStatus")
+                .is_ok()
+        );
+
+        let writer_without_admin =
+            identity_with_roles_and_scopes(&["openshell-user"], &["provider:write"]);
+        let err = policy
+            .check(
+                &writer_without_admin,
+                "/openshell.v1.OpenShell/ConfigureProviderRefresh",
+            )
+            .unwrap_err();
+        assert_eq!(err.code(), tonic::Code::PermissionDenied);
+        assert!(err.message().contains("openshell-admin"));
+
+        let admin_without_scope =
+            identity_with_roles_and_scopes(&["openshell-admin"], &["provider:read"]);
+        let err = policy
+            .check(
+                &admin_without_scope,
+                "/openshell.v1.OpenShell/RotateProviderCredential",
+            )
+            .unwrap_err();
+        assert_eq!(err.code(), tonic::Code::PermissionDenied);
+        assert!(err.message().contains("provider:write"));
+
+        let admin_writer =
+            identity_with_roles_and_scopes(&["openshell-admin"], &["provider:write"]);
+        for method in [
+            "/openshell.v1.OpenShell/ConfigureProviderRefresh",
+            "/openshell.v1.OpenShell/RotateProviderCredential",
+            "/openshell.v1.OpenShell/DeleteProviderRefresh",
+        ] {
+            assert!(policy.check(&admin_writer, method).is_ok(), "{method}");
+        }
+    }
+
     #[test]
     fn get_sandbox_config_requires_config_read_scope() {
         let policy = scoped_policy();

crates/openshell-server/src/grpc/provider.rs

@@ -482,7 +482,7 @@ async fn validate_provider_environment_keys_unique_at(
                 })?,
         };
         let provider_name = provider.object_name().to_string();
-        for key in active_provider_credential_keys(&provider, now_ms) {
+        for key in active_provider_environment_keys(store, &provider, now_ms).await? {
             if let Some(first_provider) = seen.get(&key) {
                 if first_provider != &provider_name {
                     return Err(Status::failed_precondition(format!(
@@ -497,6 +497,26 @@ async fn validate_provider_environment_keys_unique_at(
     Ok(())
 }
 
+async fn active_provider_environment_keys(
+    store: &Store,
+    provider: &Provider,
+    now_ms: i64,
+) -> Result<Vec<String>, Status> {
+    let mut keys = active_provider_credential_keys(provider, now_ms);
+    if !provider.object_id().is_empty() {
+        keys.extend(
+            crate::provider_refresh::list_refresh_states_for_provider(store, provider.object_id())
+                .await?
+                .into_iter()
+                .map(|state| state.credential_key)
+                .filter(|key| is_valid_env_key(key)),
+        );
+    }
+    keys.sort();
+    keys.dedup();
+    Ok(keys)
+}
+
 fn active_provider_credential_keys(provider: &Provider, now_ms: i64) -> Vec<String> {
     provider
         .credentials
@@ -2289,6 +2309,89 @@ mod tests {
         assert!(states.is_empty());
     }
 
+    #[tokio::test]
+    async fn configure_provider_refresh_treats_existing_refresh_state_keys_as_reserved() {
+        let state = test_server_state().await;
+        for name in ["first-graph", "second-graph"] {
+            create_provider_record(
+                state.store.as_ref(),
+                Provider {
+                    metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta {
+                        id: String::new(),
+                        name: name.to_string(),
+                        created_at_ms: 0,
+                        labels: HashMap::new(),
+                    }),
+                    r#type: "outlook".to_string(),
+                    credentials: HashMap::new(),
+                    config: HashMap::new(),
+                    credential_expires_at_ms: HashMap::new(),
+                },
+            )
+            .await
+            .unwrap();
+        }
+        state
+            .store
+            .put_message(&Sandbox {
+                metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta {
+                    id: "sandbox-refresh-collision".to_string(),
+                    name: "refresh-collision".to_string(),
+                    created_at_ms: 0,
+                    labels: HashMap::new(),
+                }),
+                spec: Some(SandboxSpec {
+                    providers: vec!["first-graph".to_string(), "second-graph".to_string()],
+                    ..SandboxSpec::default()
+                }),
+                ..Default::default()
+            })
+            .await
+            .unwrap();
+
+        handle_configure_provider_refresh(
+            &state,
+            Request::new(ConfigureProviderRefreshRequest {
+                provider: "first-graph".to_string(),
+                credential_key: "MS_GRAPH_ACCESS_TOKEN".to_string(),
+                strategy: ProviderCredentialRefreshStrategy::Oauth2ClientCredentials as i32,
+                material: HashMap::from([
+                    ("tenant_id".to_string(), "tenant".to_string()),
+                    ("client_id".to_string(), "client-id".to_string()),
+                    ("client_secret".to_string(), "client-secret".to_string()),
+                ]),
+                secret_material_keys: vec!["client_secret".to_string()],
+                expires_at_ms: None,
+            }),
+        )
+        .await
+        .unwrap();
+
+        let err = handle_configure_provider_refresh(
+            &state,
+            Request::new(ConfigureProviderRefreshRequest {
+                provider: "second-graph".to_string(),
+                credential_key: "MS_GRAPH_ACCESS_TOKEN".to_string(),
+                strategy: ProviderCredentialRefreshStrategy::Oauth2ClientCredentials as i32,
+                material: HashMap::from([
+                    ("tenant_id".to_string(), "tenant".to_string()),
+                    ("client_id".to_string(), "client-id".to_string()),
+                    ("client_secret".to_string(), "client-secret".to_string()),
+                ]),
+                secret_material_keys: vec!["client_secret".to_string()],
+                expires_at_ms: None,
+            }),
+        )
+        .await
+        .unwrap_err();
+
+        assert_eq!(err.code(), Code::FailedPrecondition);
+        assert!(err.message().contains("collision"));
+        assert!(err.message().contains("MS_GRAPH_ACCESS_TOKEN"));
+        assert!(err.message().contains("first-graph"));
+        assert!(err.message().contains("second-graph"));
+    }
+
     #[tokio::test]
     async fn configure_provider_refresh_rejects_profile_endpoint_override_and_missing_material() {
         let state = test_server_state().await;