Skip to content

Wire authorization into workspace model #2445

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
derekwaynecarr wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Wire authorization into workspace model #2445

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 8 additions & 2 deletions crates/openshell-cli/src/run.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2026,11 +2026,17 @@ pub async fn sandbox_create(
}
None => None,
};
let providers_v2_enabled = gateway_providers_v2_enabled(&mut client).await?;
let inferred_provider = inferred_provider_type(command);
let providers_v2_enabled =
if inferred_provider.is_some() && auto_providers_override != Some(false) {
gateway_providers_v2_enabled(&mut client).await?
} else {
false
};
let inferred_types: Vec<String> = if providers_v2_enabled {
Vec::new()
} else {
inferred_provider_type(command).into_iter().collect()
inferred_provider.into_iter().collect()
};
let configured_providers = ensure_required_providers(
&mut client,
Expand Down
40 changes: 39 additions & 1 deletion crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ use std::collections::HashMap;
use std::fs;
use std::os::unix::fs::PermissionsExt;
use std::sync::Arc;
use std::sync::atomic::{AtomicBool, Ordering};
use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering};
use std::time::{Duration, Instant};
use tempfile::TempDir;
use tokio::net::TcpListener;
Expand All @@ -48,6 +48,7 @@ struct SandboxState {
vm_slow_progress_before_ready: Arc<AtomicBool>,
vm_log_churn_before_ready: Arc<AtomicBool>,
global_settings: Arc<Mutex<HashMap<String, SettingValue>>>,
gateway_config_requests: Arc<AtomicUsize>,
}

#[derive(Clone, Default)]
Expand Down Expand Up @@ -182,6 +183,9 @@ impl OpenShell for TestOpenShell {
&self,
_request: tonic::Request<GetGatewayConfigRequest>,
) -> Result<Response<GetGatewayConfigResponse>, Status> {
self.state
.gateway_config_requests
.fetch_add(1, Ordering::SeqCst);
Ok(Response::new(GetGatewayConfigResponse {
settings: self.state.global_settings.lock().await.clone(),
settings_revision: 1,
Expand Down Expand Up @@ -1208,6 +1212,40 @@ async fn sandbox_create_keeps_command_sessions_by_default() {
);
}

#[tokio::test]
async fn sandbox_create_without_inferred_provider_skips_gateway_config() {
let server = run_server().await;
let fake_ssh_dir = tempfile::tempdir().unwrap();
let xdg_dir = tempfile::tempdir().unwrap();
let _env = test_env(&fake_ssh_dir, &xdg_dir);
let tls = test_tls(&server);
install_fake_ssh(&fake_ssh_dir);

run::sandbox_create(
&server.endpoint,
"openshell",
run::SandboxCreateConfig {
name: Some("no-provider-config"),
command: &["echo".into(), "OK".into()],
..test_config()
},
"default",
&tls,
)
.await
.expect("sandbox create should succeed without reading gateway config");

assert_eq!(
server
.openshell
.state
.gateway_config_requests
.load(Ordering::SeqCst),
0,
"commands without an inferred provider must not require global gateway settings"
);
}

#[tokio::test]
async fn sandbox_create_sends_cpu_and_memory_limits_only() {
let server = run_server().await;
Expand Down
2 changes: 1 addition & 1 deletion crates/openshell-server/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ openshell-policy = { path = "../openshell-policy" }
openshell-prover = { path = "../openshell-prover" }
openshell-providers = { path = "../openshell-providers" }
openshell-router = { path = "../openshell-router" }
openshell-server-macros = { path = "../openshell-server-macros" }
openshell-supervisor-middleware = { path = "../openshell-supervisor-middleware" }
openshell-supervisor-middleware-builtins = { path = "../openshell-supervisor-middleware-builtins" }

Expand All @@ -40,6 +39,7 @@ tokio = { workspace = true }
# gRPC
tonic = { workspace = true, features = ["channel", "tls-native-roots"] }
prost = { workspace = true }
prost-reflect = { workspace = true }
prost-types = { workspace = true }

# HTTP server
Expand Down
103 changes: 68 additions & 35 deletions crates/openshell-server/src/auth/authz.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
//! authorization is a gateway concern.

use super::identity::Identity;
use super::method_authz::{self, Role};
use super::{descriptor_authz, method_authz};
use tonic::Status;
use tracing::debug;

Expand Down Expand Up @@ -62,14 +62,16 @@ impl AuthzPolicy {
/// Returns `Ok(())` if authorized, `Err(PERMISSION_DENIED)` if not.
/// When both role names are empty, all authenticated callers are authorized
/// (authentication-only mode for providers like GitHub).
///
/// Methods annotated with `global_role` (e.g. `"platform_admin"`) require
/// the `admin_role` OIDC claim. Methods annotated with only
/// `workspace_role` require the `user_role` OIDC claim — the handler
/// enforces workspace-level role via `authorize_workspace()`.
#[allow(clippy::result_large_err)]
pub fn check(&self, identity: &Identity, method: &str) -> Result<(), Status> {
let required = match method_authz::required_role(method) {
Some(Role::Admin) => &self.admin_role,
// Default to user role for unknown methods, matching the
// pre-annotation behavior. The exhaustiveness test ensures
// every real RPC has an explicit declaration.
Some(Role::User) | None => &self.user_role,
let required = match descriptor_authz::lookup(method) {
Some(entry) if entry.global_role.is_some() => &self.admin_role,
_ => &self.user_role,
};

// Empty role name = skip role check for this level (auth-only mode).
Expand Down Expand Up @@ -180,25 +182,51 @@ mod tests {
}

#[test]
fn user_cannot_access_admin_methods() {
fn user_blocked_for_platform_admin_methods() {
let id = identity_with_roles(&["openshell-user"]);
let policy = default_policy();
assert!(
policy
.check(&id, "/openshell.v1.OpenShell/CreateProvider")
.check(&id, "/openshell.v1.OpenShell/CreateWorkspace")
.is_err()
);
assert!(
policy
.check(&id, "/openshell.v1.OpenShell/GetGatewayInfo")
.is_err()
);
}

#[test]
fn admin_can_access_admin_methods() {
let id = identity_with_roles(&["openshell-admin", "openshell-user"]);
fn user_passes_middleware_for_workspace_admin_methods() {
let id = identity_with_roles(&["openshell-user"]);
let policy = default_policy();
assert!(
policy
.check(&id, "/openshell.v1.OpenShell/CreateProvider")
.is_ok()
);
assert!(
policy
.check(&id, "/openshell.v1.OpenShell/DeleteProvider")
.is_ok()
);
assert!(
policy
.check(&id, "/openshell.v1.OpenShell/AddWorkspaceMember")
.is_ok()
);
}

#[test]
fn admin_can_access_platform_admin_methods() {
let id = identity_with_roles(&["openshell-admin", "openshell-user"]);
let policy = default_policy();
assert!(
policy
.check(&id, "/openshell.v1.OpenShell/CreateWorkspace")
.is_ok()
);
}

#[test]
Expand Down Expand Up @@ -253,7 +281,7 @@ mod tests {
};
assert!(
policy
.check(&id, "/openshell.v1.OpenShell/CreateProvider")
.check(&id, "/openshell.v1.OpenShell/CreateWorkspace")
.is_ok()
);
assert!(
Expand Down Expand Up @@ -408,7 +436,7 @@ mod tests {
}

#[test]
fn provider_refresh_methods_require_provider_scopes_and_admin_for_writes() {
fn provider_refresh_methods_require_provider_scopes() {
let policy = scoped_policy();
let reader = identity_with_roles_and_scopes(&["openshell-user"], &["provider:read"]);
assert!(
Expand All @@ -417,17 +445,26 @@ mod tests {
.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"));
// Workspace-admin methods now pass middleware with user role + correct scope.
// Handler enforces workspace membership.
let writer = identity_with_roles_and_scopes(&["openshell-user"], &["provider:write"]);
assert!(
policy
.check(&writer, "/openshell.v1.OpenShell/ConfigureProviderRefresh")
.is_ok()
);
assert!(
policy
.check(&writer, "/openshell.v1.OpenShell/RotateProviderCredential")
.is_ok()
);
assert!(
policy
.check(&writer, "/openshell.v1.OpenShell/DeleteProviderRefresh")
.is_ok()
);

// Wrong scope still rejected.
let admin_without_scope =
identity_with_roles_and_scopes(&["openshell-admin"], &["provider:read"]);
let err = policy
Expand All @@ -438,16 +475,6 @@ mod tests {
.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]
Expand Down Expand Up @@ -493,10 +520,16 @@ mod tests {
.check(&id, "/openshell.v1.OpenShell/GetProvider")
.is_ok()
);
// admin methods still denied by role check
// Workspace-admin methods pass middleware with user role.
assert!(
policy
.check(&id, "/openshell.v1.OpenShell/CreateProvider")
.is_ok()
);
// Platform-admin methods still denied by role check.
assert!(
policy
.check(&id, "/openshell.v1.OpenShell/CreateWorkspace")
.is_err()
);
}
Expand All @@ -507,7 +540,7 @@ mod tests {
let policy = scoped_policy();
assert!(
policy
.check(&id, "/openshell.v1.OpenShell/CreateProvider")
.check(&id, "/openshell.v1.OpenShell/CreateWorkspace")
.is_ok()
);
assert!(
Expand Down
Loading
Loading