feat: Add role-aware plugin share context APIs (#21867)

Expose discoverability and full share principals in share context, carry roles through save/updateTargets, hydrate local shared plugin reads, and keep share URLs only under plugin.shareContext.
2026-07-01 00:31:56 +08:00 · 2026-05-08 20:46:39 -07:00
parent c579da41b1
commit 479491ed89
25 changed files with 826 additions and 221 deletions
@@ -32,9 +32,11 @@ pub use remote_installed_plugin_sync::sync_remote_installed_plugin_bundles_once;
 pub use share::RemotePluginShareAccessPolicy;
 pub use share::RemotePluginShareDiscoverability;
 pub use share::RemotePluginSharePrincipal;
+pub use share::RemotePluginSharePrincipalRole;
 pub use share::RemotePluginSharePrincipalType;
 pub use share::RemotePluginShareSaveResult;
 pub use share::RemotePluginShareTarget;
+pub use share::RemotePluginShareTargetRole;
 pub use share::RemotePluginShareUpdateDiscoverability;
 pub use share::RemotePluginShareUpdateTargetsResult;
 pub use share::delete_remote_plugin_share;
@@ -99,16 +101,16 @@ pub struct RemotePluginSummary {
 #[derive(Debug, Clone, PartialEq, Eq)]
 pub struct RemotePluginShareContext {
    pub remote_plugin_id: String,
+    pub discoverability: RemotePluginShareDiscoverability,
    pub share_url: Option<String>,
    pub creator_account_user_id: Option<String>,
    pub creator_name: Option<String>,
-    pub share_targets: Option<Vec<RemotePluginSharePrincipal>>,
+    pub share_principals: Option<Vec<RemotePluginSharePrincipal>>,
 }

 #[derive(Debug, Clone, PartialEq)]
 pub struct RemotePluginShareSummary {
    pub summary: RemotePluginSummary,
-    pub share_url: Option<String>,
    pub local_plugin_path: Option<AbsolutePathBuf>,
 }

@@ -361,6 +363,8 @@ struct RemotePluginDirectoryItem {
    name: String,
    scope: RemotePluginScope,
    #[serde(default)]
+    discoverability: Option<RemotePluginShareDiscoverability>,
+    #[serde(default)]
    creator_account_user_id: Option<String>,
    #[serde(default)]
    creator_name: Option<String>,
@@ -379,8 +383,7 @@ struct RemotePluginDirectoryItem {
 struct RemotePluginDirectorySharePrincipal {
    principal_type: RemotePluginSharePrincipalType,
    principal_id: String,
-    #[serde(default)]
-    role: Option<String>,
+    role: RemotePluginSharePrincipalRole,
    name: String,
 }

@@ -444,7 +447,7 @@ pub async fn fetch_remote_marketplaces(
                    directory_plugins,
                    installed_plugins,
                    /*include_installed_only*/ true,
-                )
+                )?
            }
            RemoteMarketplaceSource::WorkspaceDirectory => {
                let scope = RemotePluginScope::Workspace;
@@ -456,7 +459,7 @@ pub async fn fetch_remote_marketplaces(
                    directory_plugins,
                    workspace_installed_plugins.clone().unwrap_or_default(),
                    /*include_installed_only*/ false,
-                )
+                )?
            }
            RemoteMarketplaceSource::SharedWithMe => build_remote_marketplace(
                REMOTE_SHARED_WITH_ME_MARKETPLACE_NAME,
@@ -464,7 +467,7 @@ pub async fn fetch_remote_marketplaces(
                fetch_shared_workspace_plugins(config, auth).await?,
                workspace_installed_plugins.clone().unwrap_or_default(),
                /*include_installed_only*/ false,
-            ),
+            )?,
        };
        if let Some(marketplace) = marketplace {
            marketplaces.push(marketplace);
@@ -480,7 +483,7 @@ fn build_remote_marketplace(
    directory_plugins: Vec<RemotePluginDirectoryItem>,
    installed_plugins: Vec<RemotePluginInstalledItem>,
    include_installed_only: bool,
-) -> Option<RemoteMarketplace> {
+) -> Result<Option<RemoteMarketplace>, RemotePluginCatalogError> {
    let directory_plugins = directory_plugins
        .into_iter()
        .map(|plugin| (plugin.id.clone(), plugin))
@@ -500,7 +503,7 @@ fn build_remote_marketplace(
        .cloned()
        .collect::<BTreeSet<_>>();
    if plugin_ids.is_empty() {
-        return None;
+        return Ok(None);
    }

    let mut plugins = plugin_ids
@@ -510,9 +513,10 @@ fn build_remote_marketplace(
            let installed_plugin = installed_plugins.get(&plugin_id);
            directory_plugin
                .or_else(|| installed_plugin.map(|plugin| &plugin.plugin))
-                .map(|plugin| build_remote_plugin_summary(plugin, installed_plugin))
+                .map(|plugin| (plugin, installed_plugin))
        })
-        .collect::<Vec<_>>();
+        .map(|(plugin, installed_plugin)| build_remote_plugin_summary(plugin, installed_plugin))
+        .collect::<Result<Vec<_>, _>>()?;
    plugins.sort_by(|left, right| {
        remote_plugin_display_name(left)
            .to_ascii_lowercase()
@@ -520,11 +524,11 @@ fn build_remote_marketplace(
            .then_with(|| remote_plugin_display_name(left).cmp(remote_plugin_display_name(right)))
            .then_with(|| left.id.cmp(&right.id))
    });
-    Some(RemoteMarketplace {
+    Ok(Some(RemoteMarketplace {
        name: name.to_string(),
        display_name: display_name.to_string(),
        plugins,
-    })
+    }))
 }

 pub async fn fetch_remote_installed_plugins(
@@ -576,6 +580,19 @@ pub async fn fetch_remote_plugin_detail(
    .await
 }

+pub async fn fetch_remote_plugin_share_context(
+    config: &RemotePluginServiceConfig,
+    auth: Option<&CodexAuth>,
+    plugin_id: &str,
+) -> Result<Option<RemotePluginShareContext>, RemotePluginCatalogError> {
+    let auth = ensure_chatgpt_auth(auth)?;
+    let plugin = fetch_plugin_detail(
+        config, auth, plugin_id, /*include_download_urls*/ false,
+    )
+    .await?;
+    remote_plugin_share_context(&plugin)
+}
+
 pub async fn fetch_remote_plugin_detail_with_download_urls(
    config: &RemotePluginServiceConfig,
    auth: Option<&CodexAuth>,
@@ -687,7 +704,7 @@ async fn build_remote_plugin_detail(
    Ok(RemotePluginDetail {
        marketplace_name,
        marketplace_display_name: scope.marketplace_display_name().to_string(),
-        summary: build_remote_plugin_summary(&plugin, installed_plugin.as_ref()),
+        summary: build_remote_plugin_summary(&plugin, installed_plugin.as_ref())?,
        description: non_empty_string(Some(&plugin.release.description)),
        release_version: plugin.release.version,
        bundle_download_url: plugin.release.bundle_download_url,
@@ -823,11 +840,11 @@ fn remove_remote_plugin_cache(
 fn build_remote_plugin_summary(
    plugin: &RemotePluginDirectoryItem,
    installed_plugin: Option<&RemotePluginInstalledItem>,
-) -> RemotePluginSummary {
-    RemotePluginSummary {
+) -> Result<RemotePluginSummary, RemotePluginCatalogError> {
+    Ok(RemotePluginSummary {
        id: plugin.id.clone(),
        name: plugin.name.clone(),
-        share_context: remote_plugin_share_context(plugin),
+        share_context: remote_plugin_share_context(plugin)?,
        installed: installed_plugin.is_some(),
        enabled: installed_plugin.is_some_and(|plugin| plugin.enabled),
        install_policy: plugin.installation_policy,
@@ -835,31 +852,40 @@ fn build_remote_plugin_summary(
        availability: plugin.availability,
        interface: remote_plugin_interface_to_info(plugin),
        keywords: plugin.release.keywords.clone(),
-    }
+    })
 }

 fn remote_plugin_share_context(
    plugin: &RemotePluginDirectoryItem,
-) -> Option<RemotePluginShareContext> {
+) -> Result<Option<RemotePluginShareContext>, RemotePluginCatalogError> {
    match plugin.scope {
-        RemotePluginScope::Global => None,
-        RemotePluginScope::Workspace => Some(RemotePluginShareContext {
-            remote_plugin_id: plugin.id.clone(),
-            share_url: plugin.share_url.clone(),
-            creator_account_user_id: plugin.creator_account_user_id.clone(),
-            creator_name: plugin.creator_name.clone(),
-            share_targets: plugin.share_principals.as_ref().map(|principals| {
-                principals
-                    .iter()
-                    .filter(|principal| principal.role.as_deref() == Some("reader"))
-                    .map(|principal| RemotePluginSharePrincipal {
-                        principal_type: principal.principal_type,
-                        principal_id: principal.principal_id.clone(),
-                        name: principal.name.clone(),
-                    })
-                    .collect()
-            }),
-        }),
+        RemotePluginScope::Global => Ok(None),
+        RemotePluginScope::Workspace => {
+            let discoverability = plugin.discoverability.ok_or_else(|| {
+                RemotePluginCatalogError::UnexpectedResponse(format!(
+                    "workspace plugin `{}` did not include discoverability",
+                    plugin.id
+                ))
+            })?;
+            Ok(Some(RemotePluginShareContext {
+                remote_plugin_id: plugin.id.clone(),
+                discoverability,
+                share_url: plugin.share_url.clone(),
+                creator_account_user_id: plugin.creator_account_user_id.clone(),
+                creator_name: plugin.creator_name.clone(),
+                share_principals: plugin.share_principals.as_ref().map(|share_principals| {
+                    share_principals
+                        .iter()
+                        .map(|principal| RemotePluginSharePrincipal {
+                            principal_type: principal.principal_type,
+                            principal_id: principal.principal_id.clone(),
+                            role: principal.role,
+                            name: principal.name.clone(),
+                        })
+                        .collect()
+                }),
+            }))
+        }
    }
 }

@@ -59,15 +59,32 @@ pub enum RemotePluginSharePrincipalType {
 pub struct RemotePluginShareTarget {
    pub principal_type: RemotePluginSharePrincipalType,
    pub principal_id: String,
+    pub role: RemotePluginShareTargetRole,
 }

 #[derive(Debug, Clone, PartialEq, Eq, Deserialize)]
 pub struct RemotePluginSharePrincipal {
    pub principal_type: RemotePluginSharePrincipalType,
    pub principal_id: String,
+    pub role: RemotePluginSharePrincipalRole,
    pub name: String,
 }

+#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)]
+#[serde(rename_all = "lowercase")]
+pub enum RemotePluginShareTargetRole {
+    Reader,
+    Editor,
+}
+
+#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)]
+#[serde(rename_all = "lowercase")]
+pub enum RemotePluginSharePrincipalRole {
+    Reader,
+    Editor,
+    Owner,
+}
+
 #[derive(Debug, Clone, PartialEq, Eq)]
 pub struct RemotePluginShareUpdateTargetsResult {
    pub principals: Vec<RemotePluginSharePrincipal>,
@@ -115,7 +132,7 @@ struct RemotePluginShareUpdateTargetsRequest {
 #[derive(Debug, Clone, PartialEq, Eq, Deserialize)]
 struct RemotePluginShareUpdateTargetsResponse {
    principals: Vec<RemotePluginSharePrincipal>,
-    discoverability: Option<RemotePluginShareDiscoverability>,
+    discoverability: RemotePluginShareDiscoverability,
 }

 pub async fn save_remote_plugin_share(
@@ -203,33 +220,54 @@ pub async fn list_remote_plugin_shares(
            .map(|plugin| (plugin.plugin.id.clone(), plugin))
            .collect::<BTreeMap<_, _>>();
    let local_plugin_paths =
-        local_paths::load_plugin_share_local_paths(codex_home).unwrap_or_else(|err| {
-            warn!("failed to load plugin share local path mapping: {err}");
-            BTreeMap::new()
-        });
+        local_paths::load_plugin_share_local_paths(codex_home).map_err(|err| {
+            RemotePluginCatalogError::UnexpectedResponse(format!(
+                "failed to load plugin share local path mapping: {err}"
+            ))
+        })?;

-    Ok(created_plugins
+    created_plugins
        .into_iter()
        .map(|plugin| {
-            let summary = build_remote_plugin_summary(&plugin, installed_by_id.get(&plugin.id));
-            let local_plugin_path = local_plugin_paths.get(&plugin.id).cloned();
-            RemotePluginShareSummary {
-                summary,
-                share_url: plugin.share_url,
-                local_plugin_path,
+            let summary = build_remote_plugin_summary(&plugin, installed_by_id.get(&plugin.id))?;
+            if summary
+                .share_context
+                .as_ref()
+                .and_then(|context| context.share_principals.as_ref())
+                .is_none()
+            {
+                return Err(RemotePluginCatalogError::UnexpectedResponse(format!(
+                    "created workspace plugin `{}` did not include share_principals",
+                    plugin.id
+                )));
            }
+            let local_plugin_path = local_plugin_paths.get(&plugin.id).cloned();
+            Ok(RemotePluginShareSummary {
+                summary,
+                local_plugin_path,
+            })
        })
-        .collect())
+        .collect()
 }

 pub fn load_plugin_share_remote_ids_by_local_path(
    codex_home: &Path,
 ) -> io::Result<BTreeMap<AbsolutePathBuf, String>> {
    let local_paths = local_paths::load_plugin_share_local_paths(codex_home)?;
-    Ok(local_paths
+    local_paths
        .into_iter()
-        .map(|(remote_plugin_id, local_plugin_path)| (local_plugin_path, remote_plugin_id))
-        .collect())
+        .map(|(remote_plugin_id, local_plugin_path)| {
+            if !is_valid_remote_plugin_id(&remote_plugin_id) {
+                return Err(io::Error::new(
+                    io::ErrorKind::InvalidData,
+                    format!(
+                        "invalid remote plugin id in share local path mapping: {remote_plugin_id}"
+                    ),
+                ));
+            }
+            Ok((local_plugin_path, remote_plugin_id))
+        })
+        .collect()
 }

 pub async fn delete_remote_plugin_share(
@@ -284,9 +322,7 @@ pub async fn update_remote_plugin_share_targets(
    let response: RemotePluginShareUpdateTargetsResponse = send_and_decode(request, &url).await?;
    Ok(RemotePluginShareUpdateTargetsResult {
        principals: response.principals,
-        // TODO: Remove this fallback once deployed plugin-service responses always include
-        // discoverability per the API schema.
-        discoverability: response.discoverability.unwrap_or(target_discoverability),
+        discoverability: response.discoverability,
    })
 }

@@ -311,6 +347,7 @@ fn ensure_unlisted_workspace_target(
        targets.push(RemotePluginShareTarget {
            principal_type: RemotePluginSharePrincipalType::Workspace,
            principal_id: account_id,
+            role: RemotePluginShareTargetRole::Reader,
        });
    }
    Ok(Some(targets))
@@ -116,6 +116,7 @@ fn remote_plugin_json_with_share_url_and_principals(
    let serde_json::Value::Object(fields) = &mut plugin else {
        unreachable!("plugin json should be an object");
    };
+    fields.insert("discoverability".to_string(), json!("PRIVATE"));
    fields.insert("share_url".to_string(), json!(share_url));
    fields.insert("share_principals".to_string(), share_principals);
    plugin
@@ -209,10 +210,12 @@ async fn save_remote_plugin_share_creates_workspace_plugin() {
                {
                    "principal_type": "user",
                    "principal_id": "user-1",
+                    "role": "reader",
                },
                {
                    "principal_type": "workspace",
                    "principal_id": "account_id",
+                    "role": "reader",
                },
            ],
        })))
@@ -235,6 +238,7 @@ async fn save_remote_plugin_share_creates_workspace_plugin() {
            share_targets: Some(vec![RemotePluginShareTarget {
                principal_type: RemotePluginSharePrincipalType::User,
                principal_id: "user-1".to_string(),
+                role: RemotePluginShareTargetRole::Reader,
            }]),
        },
    )
@@ -404,14 +408,17 @@ async fn update_remote_plugin_share_targets_updates_targets() {
                {
                    "principal_type": "user",
                    "principal_id": "user-1",
+                    "role": "editor",
                },
                {
                    "principal_type": "group",
                    "principal_id": "group-1",
+                    "role": "reader",
                },
                {
                    "principal_type": "workspace",
                    "principal_id": "account_id",
+                    "role": "reader",
                },
            ],
        })))
@@ -420,11 +427,13 @@ async fn update_remote_plugin_share_targets_updates_targets() {
                {
                    "principal_type": "user",
                    "principal_id": "user-1",
+                    "role": "editor",
                    "name": "Gavin",
                },
                {
                    "principal_type": "group",
                    "principal_id": "group-1",
+                    "role": "reader",
                    "name": "Engineering",
                },
            ],
@@ -442,10 +451,12 @@ async fn update_remote_plugin_share_targets_updates_targets() {
            RemotePluginShareTarget {
                principal_type: RemotePluginSharePrincipalType::User,
                principal_id: "user-1".to_string(),
+                role: RemotePluginShareTargetRole::Editor,
            },
            RemotePluginShareTarget {
                principal_type: RemotePluginSharePrincipalType::Group,
                principal_id: "group-1".to_string(),
+                role: RemotePluginShareTargetRole::Reader,
            },
        ],
        RemotePluginShareUpdateDiscoverability::Unlisted,
@@ -460,11 +471,13 @@ async fn update_remote_plugin_share_targets_updates_targets() {
                RemotePluginSharePrincipal {
                    principal_type: RemotePluginSharePrincipalType::User,
                    principal_id: "user-1".to_string(),
+                    role: RemotePluginSharePrincipalRole::Editor,
                    name: "Gavin".to_string(),
                },
                RemotePluginSharePrincipal {
                    principal_type: RemotePluginSharePrincipalType::Group,
                    principal_id: "group-1".to_string(),
+                    role: RemotePluginSharePrincipalRole::Reader,
                    name: "Engineering".to_string(),
                },
            ],
@@ -473,64 +486,6 @@ async fn update_remote_plugin_share_targets_updates_targets() {
    );
 }

-#[tokio::test]
-async fn update_remote_plugin_share_targets_falls_back_to_requested_discoverability() {
-    let server = MockServer::start().await;
-    let config = test_config(&server);
-    let auth = test_auth();
-
-    Mock::given(method("PUT"))
-        .and(path("/backend-api/ps/plugins/plugins_123/shares"))
-        .and(header("authorization", "Bearer Access Token"))
-        .and(header("chatgpt-account-id", "account_id"))
-        .and(body_json(json!({
-            "discoverability": "PRIVATE",
-            "targets": [
-                {
-                    "principal_type": "user",
-                    "principal_id": "user-1",
-                },
-            ],
-        })))
-        .respond_with(ResponseTemplate::new(200).set_body_json(json!({
-            "principals": [
-                {
-                    "principal_type": "user",
-                    "principal_id": "user-1",
-                    "name": "Gavin",
-                },
-            ],
-        })))
-        .expect(1)
-        .mount(&server)
-        .await;
-
-    let result = update_remote_plugin_share_targets(
-        &config,
-        Some(&auth),
-        "plugins_123",
-        vec![RemotePluginShareTarget {
-            principal_type: RemotePluginSharePrincipalType::User,
-            principal_id: "user-1".to_string(),
-        }],
-        RemotePluginShareUpdateDiscoverability::Private,
-    )
-    .await
-    .unwrap();
-
-    assert_eq!(
-        result,
-        RemotePluginShareUpdateTargetsResult {
-            principals: vec![RemotePluginSharePrincipal {
-                principal_type: RemotePluginSharePrincipalType::User,
-                principal_id: "user-1".to_string(),
-                name: "Gavin".to_string(),
-            }],
-            discoverability: RemotePluginShareDiscoverability::Private,
-        }
-    );
-}
-
 #[tokio::test]
 async fn list_remote_plugin_shares_fetches_created_workspace_plugins() {
    let codex_home = TempDir::new().unwrap();
@@ -602,11 +557,6 @@ async fn list_remote_plugin_shares_fetches_created_workspace_plugins() {
                        "role": "editor",
                        "name": "Editor",
                    },
-                    {
-                        "principal_type": "user",
-                        "principal_id": "user-missing-role",
-                        "name": "Missing Role",
-                    },
                ]),
            )],
            "pagination": empty_pagination_json(),
@@ -638,16 +588,26 @@ async fn list_remote_plugin_shares_fetches_created_workspace_plugins() {
                    name: "demo-plugin".to_string(),
                    share_context: Some(RemotePluginShareContext {
                        remote_plugin_id: "plugins_123".to_string(),
+                        discoverability: RemotePluginShareDiscoverability::Private,
                        share_url: Some(
                            "https://chatgpt.example/plugins/share/share-key-1".to_string(),
                        ),
                        creator_account_user_id: None,
                        creator_name: None,
-                        share_targets: Some(vec![RemotePluginSharePrincipal {
-                            principal_type: RemotePluginSharePrincipalType::User,
-                            principal_id: "user-reader".to_string(),
-                            name: "Reader".to_string(),
-                        }]),
+                        share_principals: Some(vec![
+                            RemotePluginSharePrincipal {
+                                principal_type: RemotePluginSharePrincipalType::User,
+                                principal_id: "user-owner".to_string(),
+                                role: RemotePluginSharePrincipalRole::Owner,
+                                name: "Owner".to_string(),
+                            },
+                            RemotePluginSharePrincipal {
+                                principal_type: RemotePluginSharePrincipalType::User,
+                                principal_id: "user-reader".to_string(),
+                                role: RemotePluginSharePrincipalRole::Reader,
+                                name: "Reader".to_string(),
+                            },
+                        ]),
                    }),
                    installed: false,
                    enabled: false,
@@ -657,7 +617,6 @@ async fn list_remote_plugin_shares_fetches_created_workspace_plugins() {
                    interface: Some(expected_plugin_interface()),
                    keywords: Vec::new(),
                },
-                share_url: Some("https://chatgpt.example/plugins/share/share-key-1".to_string()),
                local_plugin_path: Some(local_plugin_path),
            },
            RemotePluginShareSummary {
@@ -666,10 +625,24 @@ async fn list_remote_plugin_shares_fetches_created_workspace_plugins() {
                    name: "demo-plugin".to_string(),
                    share_context: Some(RemotePluginShareContext {
                        remote_plugin_id: "plugins_456".to_string(),
+                        discoverability: RemotePluginShareDiscoverability::Private,
                        share_url: None,
                        creator_account_user_id: None,
                        creator_name: None,
-                        share_targets: Some(Vec::new()),
+                        share_principals: Some(vec![
+                            RemotePluginSharePrincipal {
+                                principal_type: RemotePluginSharePrincipalType::User,
+                                principal_id: "user-owner".to_string(),
+                                role: RemotePluginSharePrincipalRole::Owner,
+                                name: "Owner".to_string(),
+                            },
+                            RemotePluginSharePrincipal {
+                                principal_type: RemotePluginSharePrincipalType::User,
+                                principal_id: "user-editor".to_string(),
+                                role: RemotePluginSharePrincipalRole::Editor,
+                                name: "Editor".to_string(),
+                            },
+                        ]),
                    }),
                    installed: true,
                    enabled: true,
@@ -679,7 +652,6 @@ async fn list_remote_plugin_shares_fetches_created_workspace_plugins() {
                    interface: Some(expected_plugin_interface()),
                    keywords: Vec::new(),
                },
-                share_url: None,
                local_plugin_path: None,
            }
        ]