fix(auth): address PR review feedback on workspace authorization

- Docker e2e: add --health-port and switch readiness probe from
  `openshell status` to `curl /healthz`, fixing a false-positive
  readiness check in OIDC mode where the CLI exited 0 without
  actually contacting the gateway

- ListWorkspaces: move membership filtering from post-query N+1
  lookups into a SQL EXISTS subquery so pagination applies to the
  visible set, not the global ordering. Add generic
  list_with_membership to the persistence layer.

- Descriptor validator: reject role/scope fields on unauthenticated
  and sandbox auth modes, and allow-list workspace_role as
  user/admin and global_role as platform_admin to catch typos at
  startup

Signed-off-by: Derek Carr <decarr@redhat.com>

Author: derekwaynecarr

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

@@ -150,6 +150,9 @@ impl DescriptorAuthTable {
     }
 }
 
+const VALID_WORKSPACE_ROLES: &[&str] = &["user", "admin"];
+const VALID_GLOBAL_ROLES: &[&str] = &["platform_admin"];
+
 fn validate_entry(
     path: &str,
     auth_mode: AuthMode,
@@ -163,7 +166,27 @@ fn validate_entry(
         ));
     }
 
+    if let Some(wr) = workspace_role {
+        if !VALID_WORKSPACE_ROLES.contains(&wr) {
+            return Err(format!(
+                "method {path}: unknown workspace_role '{wr}' (expected one of {VALID_WORKSPACE_ROLES:?})"
+            ));
+        }
+    }
+    if let Some(gr) = global_role {
+        if !VALID_GLOBAL_ROLES.contains(&gr) {
+            return Err(format!(
+                "method {path}: unknown global_role '{gr}' (expected one of {VALID_GLOBAL_ROLES:?})"
+            ));
+        }
+    }
+
     if !matches!(auth_mode, AuthMode::Bearer | AuthMode::Dual) {
+        if workspace_role.is_some() || global_role.is_some() || scope.is_some() {
+            return Err(format!(
+                "method {path}: {auth_mode:?} method must not declare role or scope"
+            ));
+        }
         return Ok(());
     }
 
@@ -317,4 +340,49 @@ mod tests {
         .expect_err("multiple authorization layers must be rejected");
         assert!(err.ends_with("workspace_role and global_role are mutually exclusive"));
     }
+
+    #[test]
+    fn non_bearer_methods_reject_role_and_scope_fields() {
+        let err = validate_entry(FUTURE_RPC, AuthMode::Unauthenticated, Some("user"), None, None)
+            .expect_err("unauthenticated with role must be rejected");
+        assert!(err.contains("must not declare role or scope"));
+
+        let err =
+            validate_entry(FUTURE_RPC, AuthMode::Sandbox, None, Some("platform_admin"), None)
+                .expect_err("sandbox with global_role must be rejected");
+        assert!(err.contains("must not declare role or scope"));
+
+        let err = validate_entry(
+            FUTURE_RPC,
+            AuthMode::Sandbox,
+            None,
+            None,
+            Some("sandbox:read"),
+        )
+        .expect_err("sandbox with scope must be rejected");
+        assert!(err.contains("must not declare role or scope"));
+    }
+
+    #[test]
+    fn unknown_role_literals_rejected() {
+        let err = validate_entry(
+            FUTURE_RPC,
+            AuthMode::Bearer,
+            Some("superuser"),
+            None,
+            Some("sandbox:read"),
+        )
+        .expect_err("unknown workspace_role must be rejected");
+        assert!(err.contains("unknown workspace_role 'superuser'"));
+
+        let err = validate_entry(
+            FUTURE_RPC,
+            AuthMode::Bearer,
+            None,
+            Some("root"),
+            Some("sandbox:read"),
+        )
+        .expect_err("unknown global_role must be rejected");
+        assert!(err.contains("unknown global_role 'root'"));
+    }
 }

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

@@ -48,38 +48,26 @@ impl ObjectType for WorkspaceMember {
     }
 }
 
-async fn filter_workspaces_by_membership(
-    state: &Arc<ServerState>,
-    principal: &Principal,
-    workspaces: Vec<Workspace>,
-) -> Result<Vec<Workspace>, Status> {
-    let subject = match principal {
+/// Extract the subject that needs membership filtering, or `None` if the
+/// principal has unrestricted visibility (platform admin, sandbox caller).
+fn membership_filter_subject<'a>(
+    state: &ServerState,
+    principal: &'a Principal,
+) -> Result<Option<&'a str>, Status> {
+    match principal {
         Principal::User(u) => {
             if crate::auth::workspace_authz::is_platform_admin_principal(
                 &u.identity.roles,
                 &state.admin_role,
             ) {
-                return Ok(workspaces);
+                Ok(None)
+            } else {
+                Ok(Some(&u.identity.subject))
             }
-            &u.identity.subject
-        }
-        Principal::Sandbox(_) => return Ok(workspaces),
-        Principal::Anonymous => return Err(Status::unauthenticated("authentication required")),
-    };
-
-    let mut visible = Vec::new();
-    for ws in workspaces {
-        let ws_name = ws.metadata.as_ref().map_or("", |m| m.name.as_str());
-        let member = state
-            .store
-            .get_message_by_name::<WorkspaceMember>(ws_name, subject)
-            .await
-            .map_err(|e| Status::internal(format!("membership check failed: {e}")))?;
-        if member.is_some() {
-            visible.push(ws);
         }
+        Principal::Sandbox(_) => Ok(None),
+        Principal::Anonymous => Err(Status::unauthenticated("authentication required")),
     }
-    Ok(visible)
 }
 
 fn validate_workspace_name(name: &str) -> Result<(), Status> {
@@ -279,23 +267,36 @@ pub(super) async fn handle_list_workspaces(
     let principal = super::extract_principal(&request)?;
     let req = request.into_inner();
     let limit = clamp_limit(req.limit, 100, MAX_PAGE_SIZE);
+    let subject = membership_filter_subject(state, &principal)?;
 
-    let workspaces: Vec<Workspace> = if req.label_selector.is_empty() {
-        state
+    let member_type = WorkspaceMember::object_type();
+    let workspaces = match subject {
+        Some(subject) if req.label_selector.is_empty() => state
+            .store
+            .list_messages_with_membership::<Workspace>(member_type, subject, limit, req.offset)
+            .await
+            .map_err(|e| Status::internal(format!("list workspaces failed: {e}")))?,
+        Some(subject) => {
+            let all: Vec<Workspace> = state
+                .store
+                .list_messages_with_membership(member_type, subject, u32::MAX, 0)
+                .await
+                .map_err(|e| Status::internal(format!("list workspaces failed: {e}")))?;
+            crate::persistence::filter_by_labels(all, &req.label_selector, limit, req.offset)
+                .map_err(|e| Status::internal(format!("list workspaces failed: {e}")))?
+        }
+        None if req.label_selector.is_empty() => state
             .store
             .list_messages("", limit, req.offset)
             .await
-            .map_err(|e| Status::internal(format!("list workspaces failed: {e}")))?
-    } else {
-        state
+            .map_err(|e| Status::internal(format!("list workspaces failed: {e}")))?,
+        None => state
             .store
             .list_messages_with_selector("", &req.label_selector, limit, req.offset)
             .await
-            .map_err(|e| Status::internal(format!("list workspaces failed: {e}")))?
+            .map_err(|e| Status::internal(format!("list workspaces failed: {e}")))?,
     };
 
-    let workspaces = filter_workspaces_by_membership(state, &principal, workspaces).await?;
-
     Ok(Response::new(ListWorkspacesResponse { workspaces }))
 }
 

crates/openshell-server/src/persistence/mod.rs

@@ -396,6 +396,25 @@ impl Store {
         ))
     }
 
+    /// List objects of `object_type` that have a related `member_type` record
+    /// whose `name` column matches `member_name` in the same workspace.
+    pub async fn list_with_membership(
+        &self,
+        object_type: &str,
+        member_type: &str,
+        member_name: &str,
+        limit: u32,
+        offset: u32,
+    ) -> PersistenceResult<Vec<ObjectRecord>> {
+        store_dispatch!(self.list_with_membership(
+            object_type,
+            member_type,
+            member_name,
+            limit,
+            offset
+        ))
+    }
+
     /// List objects by type across all workspaces with label selector filtering.
     pub async fn list_all_with_selector(
         &self,
@@ -498,6 +517,24 @@ impl Store {
             .collect()
     }
 
+    /// List and decode objects that have a related membership record, with
+    /// pagination. See [`Store::list_with_membership`] for details.
+    pub async fn list_messages_with_membership<
+        T: Message + Default + ObjectType + SetResourceVersion,
+    >(
+        &self,
+        member_type: &str,
+        member_name: &str,
+        limit: u32,
+        offset: u32,
+    ) -> PersistenceResult<Vec<T>> {
+        self.list_with_membership(T::object_type(), member_type, member_name, limit, offset)
+            .await?
+            .into_iter()
+            .map(decode_record)
+            .collect()
+    }
+
     /// List and decode protobuf messages across all workspaces with label
     /// selector filtering, hydrating `resource_version` from the authoritative
     /// DB row.
@@ -722,6 +759,27 @@ pub fn parse_label_selector(selector: &str) -> PersistenceResult<HashMap<String,
     Ok(labels)
 }
 
+/// Filter decoded protobuf messages by label selector, then apply pagination.
+pub fn filter_by_labels<T: ObjectLabels>(
+    items: Vec<T>,
+    label_selector: &str,
+    limit: u32,
+    offset: u32,
+) -> PersistenceResult<Vec<T>> {
+    let required = parse_label_selector(label_selector)?;
+    Ok(items
+        .into_iter()
+        .filter(|item| {
+            let labels = item.object_labels().unwrap_or_default();
+            required
+                .iter()
+                .all(|(k, v)| labels.get(k).is_some_and(|lv| lv == v))
+        })
+        .skip(offset as usize)
+        .take(limit as usize)
+        .collect())
+}
+
 /// Unconditional write helpers — test-only.
 ///
 /// Production code must use [`Store::put_if`] (with [`WriteCondition`]) or

crates/openshell-server/src/persistence/postgres.rs

@@ -457,6 +457,42 @@ LIMIT $2 OFFSET $3
         Ok(rows.into_iter().map(row_to_object_record).collect())
     }
 
+    pub async fn list_with_membership(
+        &self,
+        object_type: &str,
+        member_type: &str,
+        member_name: &str,
+        limit: u32,
+        offset: u32,
+    ) -> PersistenceResult<Vec<ObjectRecord>> {
+        let rows = sqlx::query(
+            r"
+SELECT w.object_type, w.id, w.name, w.workspace, w.payload,
+       w.created_at_ms, w.updated_at_ms, w.labels, w.resource_version
+FROM objects w
+WHERE w.object_type = $1 AND w.workspace = ''
+AND EXISTS (
+    SELECT 1 FROM objects m
+    WHERE m.object_type = $2
+    AND m.workspace = w.name
+    AND m.name = $3
+)
+ORDER BY w.created_at_ms ASC, w.name ASC
+LIMIT $4 OFFSET $5
+",
+        )
+        .bind(object_type)
+        .bind(member_type)
+        .bind(member_name)
+        .bind(i64::from(limit))
+        .bind(i64::from(offset))
+        .fetch_all(&self.pool)
+        .await
+        .map_err(|e| map_db_error(&e))?;
+
+        Ok(rows.into_iter().map(row_to_object_record).collect())
+    }
+
     pub async fn list_by_scope(
         &self,
         object_type: &str,

crates/openshell-server/src/persistence/sqlite.rs

@@ -498,6 +498,42 @@ LIMIT ?2 OFFSET ?3
         Ok(rows.into_iter().map(row_to_object_record).collect())
     }
 
+    pub async fn list_with_membership(
+        &self,
+        object_type: &str,
+        member_type: &str,
+        member_name: &str,
+        limit: u32,
+        offset: u32,
+    ) -> PersistenceResult<Vec<ObjectRecord>> {
+        let rows = sqlx::query(
+            r#"
+SELECT w."object_type", w."id", w."name", w."workspace", w."payload",
+       w."created_at_ms", w."updated_at_ms", w."labels", w."resource_version"
+FROM "objects" w
+WHERE w."object_type" = ?1 AND w."workspace" = ''
+AND EXISTS (
+    SELECT 1 FROM "objects" m
+    WHERE m."object_type" = ?2
+    AND m."workspace" = w."name"
+    AND m."name" = ?3
+)
+ORDER BY w."created_at_ms" ASC, w."name" ASC
+LIMIT ?4 OFFSET ?5
+"#,
+        )
+        .bind(object_type)
+        .bind(member_type)
+        .bind(member_name)
+        .bind(i64::from(limit))
+        .bind(i64::from(offset))
+        .fetch_all(&self.pool)
+        .await
+        .map_err(|e| map_db_error(&e))?;
+
+        Ok(rows.into_iter().map(row_to_object_record).collect())
+    }
+
     pub async fn list_by_scope(
         &self,
         object_type: &str,

e2e/with-docker-gateway.sh

@@ -445,6 +445,7 @@ e2e_generate_pki "${GATEWAY_BIN}" "${PKI_DIR}"
 export OPENSHELL_E2E_GATEWAY_CA_CERT="${PKI_DIR}/ca.crt"
 
 HOST_PORT=$(e2e_pick_port)
+HEALTH_PORT=$(e2e_pick_port)
 STATE_DIR="${XDG_STATE_HOME}"
 mkdir -p "${STATE_DIR}"
 JWT_DIR="${STATE_DIR}/jwt"
@@ -513,6 +514,7 @@ GATEWAY_ARGS=(
   --config "${GATEWAY_CONFIG}"
   --bind-address 0.0.0.0
   --port "${HOST_PORT}"
+  --health-port "${HEALTH_PORT}"
   --drivers docker
   --tls-cert "${PKI_DIR}/server/tls.crt"
   --tls-key "${PKI_DIR}/server/tls.key"
@@ -567,13 +569,12 @@ fi
 echo "Waiting for gateway to become healthy..."
 elapsed=0
 timeout=120
-last_status_output=""
 while [ "${elapsed}" -lt "${timeout}" ]; do
   if ! kill -0 "${GATEWAY_PID}" 2>/dev/null; then
     echo "ERROR: openshell-gateway exited before becoming healthy"
     exit 1
   fi
-  if last_status_output="$("${CLI_BIN}" status 2>&1)"; then
+  if curl -sf "http://127.0.0.1:${HEALTH_PORT}/healthz" >/dev/null 2>&1; then
     echo "Gateway healthy after ${elapsed}s."
     break
   fi
@@ -582,13 +583,6 @@ while [ "${elapsed}" -lt "${timeout}" ]; do
 done
 if [ "${elapsed}" -ge "${timeout}" ]; then
   echo "ERROR: gateway did not become healthy within ${timeout}s"
-  echo "=== last openshell status output ==="
-  if [ -n "${last_status_output}" ]; then
-    printf '%s\n' "${last_status_output}"
-  else
-    echo "<no output>"
-  fi
-  echo "=== end openshell status output ==="
   exit 1
 fi