Skip to content

Commit 8b5fd3f

Browse files
shiju-nvshiju-nv
committed
fix(cli): explain denied provider profile lookups
Report exact and alias profile lookup denials with fixed permission and workspace guidance. Keep backend details redacted and stop before provider mutations. Cover denied create and update calls through the CLI. Verify the complete provider list independently in the cross-workspace OIDC regression, extracting its JSON object from surrounding startup diagnostics. Signed-off-by: Shiju <shiju@nvidia.com>
1 parent 1d92000 commit 8b5fd3f

3 files changed

Lines changed: 161 additions & 16 deletions

File tree

‎crates/openshell-cli/src/commands/provider.rs‎

Lines changed: 19 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -757,6 +757,19 @@ async fn rollback_provider_create_after_gcloud_adc_failure(
757757
}
758758
}
759759

760+
fn provider_profile_lookup_error(status: &Status) -> miette::Report {
761+
// A permission code supports recovery guidance, but cannot distinguish a
762+
// missing membership from an insufficient role. Never expose backend text.
763+
if status.code() == Code::PermissionDenied {
764+
miette!(
765+
"provider profile lookup denied (PERMISSION_DENIED): \
766+
verify workspace membership and required permissions"
767+
)
768+
} else {
769+
miette!("provider profile lookup failed ({})", status.code())
770+
}
771+
}
772+
760773
async fn fetch_provider_profile(
761774
client: &mut crate::tls::GrpcClient,
762775
provider_type: &str,
@@ -781,16 +794,11 @@ async fn fetch_provider_profile(
781794
"provider profile '{requested}' not found; import a matching profile before using this provider type"
782795
)
783796
} else {
784-
miette!("provider profile lookup failed ({})", fallback_status.code())
797+
provider_profile_lookup_error(&fallback_status)
785798
}
786799
})?
787800
}
788-
Err(status) => {
789-
return Err(miette!(
790-
"provider profile lookup failed ({})",
791-
status.code()
792-
));
793-
}
801+
Err(status) => return Err(provider_profile_lookup_error(&status)),
794802
};
795803

796804
Ok(response)
@@ -1077,11 +1085,10 @@ pub async fn provider_create_with_options(options: ProviderCreateOptions<'_>) ->
10771085
if profile_id.is_empty() {
10781086
return Err(miette::miette!("provider type is required"));
10791087
}
1080-
let provider_profile = fetch_provider_profile(&mut client, profile_id, profile_workspace)
1081-
.await
1082-
.map_err(|err| {
1083-
miette::miette!("unsupported provider type or profile: {profile_id} ({err})")
1084-
})?;
1088+
// Lookup already distinguishes absent profiles from permission and transport
1089+
// failures; those failures do not establish that the profile is unsupported.
1090+
let provider_profile =
1091+
fetch_provider_profile(&mut client, profile_id, profile_workspace).await?;
10851092
let provider_type = provider_profile.id.clone();
10861093

10871094
let adc_credential_key = if from_gcloud_adc {

‎crates/openshell-cli/tests/provider_commands_integration.rs‎

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,7 @@ struct ProviderState {
8282
scoped_profiles: Arc<Mutex<HashMap<(String, String), ProviderProfile>>>,
8383
refresh_statuses: Arc<Mutex<HashMap<(String, String), ProviderCredentialRefreshStatus>>>,
8484
refresh_requests: Arc<Mutex<Vec<ProviderRefreshRequestLog>>>,
85+
provider_create_requests: Arc<AtomicU64>,
8586
provider_update_requests: Arc<Mutex<Vec<Provider>>>,
8687
deny_provider_reads: Arc<AtomicBool>,
8788
fail_provider_reads: Arc<AtomicBool>,
@@ -667,6 +668,9 @@ impl OpenShell for TestOpenShell {
667668
&self,
668669
request: tonic::Request<CreateProviderRequest>,
669670
) -> Result<Response<ProviderResponse>, Status> {
671+
self.state
672+
.provider_create_requests
673+
.fetch_add(1, Ordering::SeqCst);
670674
let mut provider = request
671675
.into_inner()
672676
.provider
@@ -2163,6 +2167,98 @@ async fn provider_readiness_update_with_no_targets_preserves_saved_success() {
21632167
}
21642168
}
21652169

2170+
#[tokio::test]
2171+
async fn provider_profile_permission_denial_preserves_safe_workspace_guidance() {
2172+
let server = run_server().await;
2173+
seed_readiness_provider(&server).await;
2174+
for (profile, failed_lookup, expected_lookups) in [
2175+
("openai", "openai", vec!["openai"]),
2176+
("gh", "github", vec!["gh", "github"]),
2177+
] {
2178+
server
2179+
.state
2180+
.providers
2181+
.lock()
2182+
.await
2183+
.get_mut(READINESS_PROVIDER)
2184+
.expect("seeded provider")
2185+
.r#type = profile.to_string();
2186+
let mut errors = server.state.profile_read_errors.lock().await;
2187+
errors.clear();
2188+
errors.insert(failed_lookup.to_string(), Code::PermissionDenied);
2189+
drop(errors);
2190+
for args in [
2191+
vec![
2192+
"provider",
2193+
"create",
2194+
"--name",
2195+
"denied-new-provider",
2196+
"--type",
2197+
profile,
2198+
"--credential",
2199+
"OPENAI_API_KEY=fixture-provider-credential",
2200+
],
2201+
vec![
2202+
"provider",
2203+
"update",
2204+
READINESS_PROVIDER,
2205+
"--from-existing",
2206+
"--wait",
2207+
"--output",
2208+
"json",
2209+
],
2210+
] {
2211+
server.state.profile_read_requests.lock().await.clear();
2212+
let output = run_readiness_cli(&server, &args).await;
2213+
assert!(!output.status.success(), "{args:?}");
2214+
assert!(output.stdout.is_empty());
2215+
assert_readiness_output_redacted(&output);
2216+
let diagnostic = String::from_utf8_lossy(&output.stderr);
2217+
let compact: String = diagnostic
2218+
.chars()
2219+
.filter(|character| !character.is_whitespace() && *character != '│')
2220+
.collect();
2221+
assert!(
2222+
compact.contains("providerprofilelookupdenied"),
2223+
"{diagnostic}"
2224+
);
2225+
// The permission code supports recovery guidance without revealing
2226+
// backend text or inferring which membership or role check failed.
2227+
assert!(compact.contains("PERMISSION_DENIED"), "{diagnostic}");
2228+
assert!(
2229+
compact.contains("verifyworkspacemembershipandrequiredpermissions"),
2230+
"{diagnostic}"
2231+
);
2232+
assert!(!diagnostic.contains("unsupported provider type or profile"));
2233+
assert_eq!(
2234+
*server.state.profile_read_requests.lock().await,
2235+
expected_lookups
2236+
);
2237+
assert!(
2238+
!server
2239+
.state
2240+
.providers
2241+
.lock()
2242+
.await
2243+
.contains_key("denied-new-provider")
2244+
);
2245+
assert_eq!(
2246+
server.state.provider_create_requests.load(Ordering::SeqCst),
2247+
0
2248+
);
2249+
assert!(
2250+
server
2251+
.state
2252+
.provider_update_requests
2253+
.lock()
2254+
.await
2255+
.is_empty()
2256+
);
2257+
assert!(server.state.readiness_requests.lock().await.is_empty());
2258+
}
2259+
}
2260+
}
2261+
21662262
#[tokio::test]
21672263
async fn provider_readiness_update_redacts_exact_and_alias_profile_lookup_errors() {
21682264
let server = run_server().await;

‎e2e/rust/tests/oidc_pkce.rs‎

Lines changed: 46 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -808,6 +808,7 @@ async fn workspace_admin_cannot_manage_another_workspace_members() {
808808
async fn workspace_admin_cannot_manage_another_workspace_providers() {
809809
const WORKSPACE_A: &str = "oidc-wsa-xprov-a";
810810
const WORKSPACE_B: &str = "oidc-wsa-xprov-b";
811+
const PROVIDER: &str = "oidc-wsa-xprovider";
811812
let (admin, workspace_admin, _user_b) =
812813
prepare_isolated_workspaces_with_admin(WORKSPACE_A, WORKSPACE_B).await;
813814

@@ -818,17 +819,58 @@ async fn workspace_admin_cannot_manage_another_workspace_providers() {
818819
"provider",
819820
"create",
820821
"--name",
821-
"oidc-wsa-xprovider",
822+
PROVIDER,
822823
"--type",
823824
"openai",
824825
"--credential",
825826
"OPENAI_API_KEY=e2e-test-value",
826827
],
827828
)
828829
.await;
829-
assert_non_member_denial(
830-
&denied,
831-
"manage another workspace's providers as workspace admin",
830+
let diagnostic = combined_output(&denied);
831+
let compact: String = diagnostic
832+
.chars()
833+
.filter(|character| !character.is_whitespace() && *character != '│')
834+
.collect();
835+
// Profile lookup redacts backend diagnostics. Check the permission code and
836+
// safe recovery guidance without requiring the server's membership details.
837+
assert!(
838+
!denied.status.success()
839+
&& compact.contains("PERMISSION_DENIED")
840+
&& compact.contains("verifyworkspacemembershipandrequiredpermissions"),
841+
"cross-workspace provider creation did not report a safe permission denial:\n{diagnostic}"
842+
);
843+
assert!(!diagnostic.contains("e2e-test-value"));
844+
845+
// Query with an independent authorized identity so a failed command alone
846+
// cannot hide a provider created before the denial was returned.
847+
let listed = assert_workspace_allowed(
848+
&admin,
849+
WORKSPACE_B,
850+
&["provider", "list", "--output", "json"],
851+
"verify denied creation left the target workspace empty",
852+
)
853+
.await;
854+
// CLI startup diagnostics may precede the JSON object on stdout.
855+
let stdout = String::from_utf8(listed.stdout).expect("provider list output should be UTF-8");
856+
let json_start = stdout
857+
.find('{')
858+
.expect("provider list output should contain JSON");
859+
let json_end = stdout
860+
.rfind('}')
861+
.expect("provider list output should contain a complete JSON object");
862+
let listing: Value = serde_json::from_str(&stdout[json_start..=json_end])
863+
.expect("provider list --output json should return JSON on stdout");
864+
assert_eq!(
865+
listing["next_page_token"], "",
866+
"provider listing is incomplete"
867+
);
868+
assert!(
869+
listing["providers"]
870+
.as_array()
871+
.expect("provider collection")
872+
.is_empty(),
873+
"denied creation added a provider to the isolated target workspace"
832874
);
833875

834876
delete_workspace(&admin, WORKSPACE_B).await;

0 commit comments

Comments
 (0)