Skip to content

Commit 3855f6b

Browse files
committed
Address review: replace unreachable!() + add ownership-scoping tests
- Replace unreachable!() with Status::internal() for defense-in-depth in get_inference_bundle when principal variant is unexpected. - Add unit tests covering per-user ownership scoping logic: - scoped_route_name: non-admin, admin, anonymous, none, sandbox cases - upsert: non-admin creates scoped route - upsert: non-owner blocked from updating another user's route - resolve_get_route: falls back to global when no per-user route - resolve_get_route: prefers per-user route over global - resolve_route_for_owner: sandbox owner resolves per-user route - resolve_route_for_owner: falls back without per-user route Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
1 parent 8277d6f commit 3855f6b

1 file changed

Lines changed: 278 additions & 1 deletion

File tree

crates/openshell-server/src/inference.rs

Lines changed: 278 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -167,7 +167,9 @@ impl Inference for InferenceService {
167167

168168
let sandbox_id = match principal {
169169
Some(crate::auth::principal::Principal::Sandbox(sp)) => &sp.sandbox_id,
170-
_ => unreachable!("authorize_inference_bundle ensures Sandbox variant"),
170+
_ => {
171+
return Err(Status::internal("unexpected principal variant"));
172+
}
171173
};
172174

173175
let sandbox_owner = self
@@ -2879,4 +2881,279 @@ mod tests {
28792881
route.version
28802882
);
28812883
}
2884+
2885+
// ---- Per-user ownership scoping tests ----
2886+
2887+
fn test_admin_principal() -> Principal {
2888+
Principal::User(UserPrincipal {
2889+
identity: Identity {
2890+
subject: "admin-uuid".to_string(),
2891+
display_name: None,
2892+
roles: vec!["openshell-admin".to_string(), "openshell-user".to_string()],
2893+
scopes: vec![],
2894+
provider: IdentityProvider::Oidc,
2895+
},
2896+
})
2897+
}
2898+
2899+
fn test_user_b_principal() -> Principal {
2900+
Principal::User(UserPrincipal {
2901+
identity: Identity {
2902+
subject: "user-b".to_string(),
2903+
display_name: None,
2904+
roles: vec!["openshell-user".to_string()],
2905+
scopes: vec![],
2906+
provider: IdentityProvider::Oidc,
2907+
},
2908+
})
2909+
}
2910+
2911+
#[test]
2912+
fn scoped_route_name_non_admin_creates_scoped_name() {
2913+
let principal = test_user_principal();
2914+
let result = scoped_route_name("cluster", Some(&principal), "openshell-admin").unwrap();
2915+
assert_eq!(result, "cluster/user-a");
2916+
}
2917+
2918+
#[test]
2919+
fn scoped_route_name_admin_gets_global_name() {
2920+
let principal = test_admin_principal();
2921+
let result = scoped_route_name("cluster", Some(&principal), "openshell-admin").unwrap();
2922+
assert_eq!(result, "cluster");
2923+
}
2924+
2925+
#[test]
2926+
fn scoped_route_name_anonymous_gets_global_name() {
2927+
let result =
2928+
scoped_route_name("cluster", Some(&Principal::Anonymous), "openshell-admin").unwrap();
2929+
assert_eq!(result, "cluster");
2930+
}
2931+
2932+
#[test]
2933+
fn scoped_route_name_none_principal_gets_global_name() {
2934+
let result = scoped_route_name("cluster", None, "openshell-admin").unwrap();
2935+
assert_eq!(result, "cluster");
2936+
}
2937+
2938+
#[test]
2939+
fn scoped_route_name_sandbox_rejected() {
2940+
let principal = test_sandbox_principal();
2941+
let err = scoped_route_name("cluster", Some(&principal), "openshell-admin").unwrap_err();
2942+
assert_eq!(err.code(), tonic::Code::Unauthenticated);
2943+
}
2944+
2945+
#[tokio::test]
2946+
async fn upsert_non_admin_creates_scoped_route() {
2947+
let store = test_store().await;
2948+
let provider = make_provider("openai-dev", "openai", "OPENAI_API_KEY", "sk-test");
2949+
store.put_message(&provider).await.expect("persist");
2950+
2951+
let user = test_user_principal();
2952+
let result = upsert_cluster_inference_route(
2953+
&store,
2954+
"cluster/user-a",
2955+
"openai-dev",
2956+
"gpt-4o",
2957+
0,
2958+
false,
2959+
Some(&user),
2960+
"openshell-admin",
2961+
)
2962+
.await
2963+
.expect("create should succeed");
2964+
assert_eq!(result.route.object_name(), "cluster/user-a");
2965+
}
2966+
2967+
#[tokio::test]
2968+
async fn upsert_non_owner_blocked_from_updating_another_users_route() {
2969+
let store = test_store().await;
2970+
let provider = make_provider("openai-dev", "openai", "OPENAI_API_KEY", "sk-test");
2971+
store.put_message(&provider).await.expect("persist");
2972+
2973+
let user_a = test_user_principal();
2974+
upsert_cluster_inference_route(
2975+
&store,
2976+
"cluster/user-a",
2977+
"openai-dev",
2978+
"gpt-4o",
2979+
0,
2980+
false,
2981+
Some(&user_a),
2982+
"openshell-admin",
2983+
)
2984+
.await
2985+
.expect("owner create");
2986+
2987+
let user_b = test_user_b_principal();
2988+
let err = upsert_cluster_inference_route(
2989+
&store,
2990+
"cluster/user-a",
2991+
"openai-dev",
2992+
"gpt-4.1",
2993+
0,
2994+
false,
2995+
Some(&user_b),
2996+
"openshell-admin",
2997+
)
2998+
.await
2999+
.expect_err("non-owner should be blocked");
3000+
assert_eq!(err.code(), tonic::Code::PermissionDenied);
3001+
}
3002+
3003+
#[tokio::test]
3004+
async fn resolve_get_route_falls_back_to_global() {
3005+
let store = test_store().await;
3006+
let provider = make_provider("openai-dev", "openai", "OPENAI_API_KEY", "sk-test");
3007+
store.put_message(&provider).await.expect("persist");
3008+
3009+
// Create a global route (no owner)
3010+
upsert_cluster_inference_route(
3011+
&store,
3012+
CLUSTER_INFERENCE_ROUTE_NAME,
3013+
"openai-dev",
3014+
"gpt-4o",
3015+
0,
3016+
false,
3017+
None,
3018+
"",
3019+
)
3020+
.await
3021+
.expect("global create");
3022+
3023+
// Non-admin user with no personal route should fall back to global
3024+
let user = test_user_principal();
3025+
let route = resolve_get_route(
3026+
&store,
3027+
CLUSTER_INFERENCE_ROUTE_NAME,
3028+
Some(&user),
3029+
"openshell-admin",
3030+
)
3031+
.await
3032+
.expect("should fall back to global");
3033+
let config = route.config.as_ref().expect("config");
3034+
assert_eq!(config.model_id, "gpt-4o");
3035+
}
3036+
3037+
#[tokio::test]
3038+
async fn resolve_get_route_prefers_per_user_route() {
3039+
let store = test_store().await;
3040+
let provider = make_provider("openai-dev", "openai", "OPENAI_API_KEY", "sk-test");
3041+
store.put_message(&provider).await.expect("persist");
3042+
3043+
// Create global route
3044+
upsert_cluster_inference_route(
3045+
&store,
3046+
CLUSTER_INFERENCE_ROUTE_NAME,
3047+
"openai-dev",
3048+
"gpt-4o",
3049+
0,
3050+
false,
3051+
None,
3052+
"",
3053+
)
3054+
.await
3055+
.expect("global");
3056+
3057+
// Create per-user route
3058+
let user = test_user_principal();
3059+
upsert_cluster_inference_route(
3060+
&store,
3061+
&format!("{CLUSTER_INFERENCE_ROUTE_NAME}/user-a"),
3062+
"openai-dev",
3063+
"gpt-4.1",
3064+
0,
3065+
false,
3066+
Some(&user),
3067+
"openshell-admin",
3068+
)
3069+
.await
3070+
.expect("per-user");
3071+
3072+
// resolve_get_route should prefer the per-user route
3073+
let route = resolve_get_route(
3074+
&store,
3075+
CLUSTER_INFERENCE_ROUTE_NAME,
3076+
Some(&user),
3077+
"openshell-admin",
3078+
)
3079+
.await
3080+
.expect("should find per-user route");
3081+
let config = route.config.as_ref().expect("config");
3082+
assert_eq!(config.model_id, "gpt-4.1");
3083+
}
3084+
3085+
#[tokio::test]
3086+
async fn resolve_route_for_owner_with_sandbox_owner() {
3087+
let store = test_store().await;
3088+
let provider = make_provider("openai-dev", "openai", "OPENAI_API_KEY", "sk-test");
3089+
store.put_message(&provider).await.expect("persist");
3090+
3091+
// Create global route
3092+
upsert_cluster_inference_route(
3093+
&store,
3094+
CLUSTER_INFERENCE_ROUTE_NAME,
3095+
"openai-dev",
3096+
"gpt-4o",
3097+
0,
3098+
false,
3099+
None,
3100+
"",
3101+
)
3102+
.await
3103+
.expect("global");
3104+
3105+
// Create per-user route for user-a
3106+
let user = test_user_principal();
3107+
upsert_cluster_inference_route(
3108+
&store,
3109+
&format!("{CLUSTER_INFERENCE_ROUTE_NAME}/user-a"),
3110+
"openai-dev",
3111+
"gpt-4.1",
3112+
0,
3113+
false,
3114+
Some(&user),
3115+
"openshell-admin",
3116+
)
3117+
.await
3118+
.expect("per-user");
3119+
3120+
// resolve_route_for_owner with owner=user-a should find per-user route
3121+
let resolved =
3122+
resolve_route_for_owner(&store, CLUSTER_INFERENCE_ROUTE_NAME, Some("user-a"))
3123+
.await
3124+
.expect("should resolve");
3125+
let resolved = resolved.expect("should find a route");
3126+
// Name should be the BASE name (not scoped) for sandbox proxy compatibility
3127+
assert_eq!(resolved.name, CLUSTER_INFERENCE_ROUTE_NAME);
3128+
assert_eq!(resolved.model_id, "gpt-4.1");
3129+
}
3130+
3131+
#[tokio::test]
3132+
async fn resolve_route_for_owner_falls_back_without_per_user() {
3133+
let store = test_store().await;
3134+
let provider = make_provider("openai-dev", "openai", "OPENAI_API_KEY", "sk-test");
3135+
store.put_message(&provider).await.expect("persist");
3136+
3137+
// Only global route exists
3138+
upsert_cluster_inference_route(
3139+
&store,
3140+
CLUSTER_INFERENCE_ROUTE_NAME,
3141+
"openai-dev",
3142+
"gpt-4o",
3143+
0,
3144+
false,
3145+
None,
3146+
"",
3147+
)
3148+
.await
3149+
.expect("global");
3150+
3151+
// Sandbox owner has no personal route → falls back to global
3152+
let resolved =
3153+
resolve_route_for_owner(&store, CLUSTER_INFERENCE_ROUTE_NAME, Some("user-x"))
3154+
.await
3155+
.expect("should resolve");
3156+
let resolved = resolved.expect("should fall back to global");
3157+
assert_eq!(resolved.model_id, "gpt-4o");
3158+
}
28823159
}

0 commit comments

Comments
 (0)