[SC 38762] Adding collection role create #1182
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| ### Enhancements | ||
|
|
||
| * Add a new command for interacting with collection roles, | ||
| `globus gcs collection role create` |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| import typing as t | ||
|
|
||
| import globus_sdk | ||
|
|
||
| from globus_cli.termio import Field | ||
| from globus_cli.termio.formatters.auth import PrincipalURNFormatter | ||
|
|
||
|
|
||
| class CollectionRoleFormatter(PrincipalURNFormatter): | ||
| """ | ||
| A formatter for collection roles | ||
| """ | ||
|
|
||
| def __init__( | ||
| self, auth_client: globus_sdk.AuthClient, collection_role: dict[str, t.Any] | ||
| ) -> None: | ||
| super().__init__(auth_client) | ||
| self.add_items(collection_role.get("id")) | ||
| self.add_items(collection_role.get("role", ())) | ||
| self.add_items(collection_role.get("principal")) | ||
|
|
||
|
|
||
| def collection_role_format_fields( | ||
| auth_client: globus_sdk.AuthClient, | ||
| collection_role: dict[str, t.Any], | ||
| ) -> list[Field]: | ||
| """ | ||
| The standard list of fields to render for a collection role. | ||
| :param auth_client: An AuthClient, used to resolve principal URNs. | ||
| :param collection_role: The collection role assignment to format | ||
| """ | ||
| principal = CollectionRoleFormatter(auth_client, collection_role) | ||
|
|
||
| return [ | ||
| Field("ID", "id"), | ||
| Field("Role", "role"), | ||
| Field("Principal", "principal", formatter=principal), | ||
| ] | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,85 @@ | ||
| from __future__ import annotations | ||
|
|
||
| import typing as t | ||
| import uuid | ||
|
|
||
| import click | ||
| import globus_sdk | ||
|
|
||
| from globus_cli.commands.collection.role._fields import collection_role_format_fields | ||
| from globus_cli.login_manager import LoginManager | ||
| from globus_cli.parsing import collection_id_arg, command | ||
| from globus_cli.termio import display | ||
| from globus_cli.utils import resolve_principal_urn | ||
|
|
||
| _VALID_ROLES = t.Literal[ | ||
| "access_manager", "activity_manager", "activity_monitor", "administrator" | ||
| ] | ||
|
|
||
|
|
||
| @command("create") | ||
| @collection_id_arg | ||
| @click.argument("ROLE", type=click.Choice(t.get_args(_VALID_ROLES)), metavar="ROLE") | ||
| @click.argument("PRINCIPAL", type=str, required=False) | ||
|
Member
There was a problem hiding this comment. I have two notes on this being an optional argument. The first is a general "CLI feel" item: if it's optional, I think it should be an option, e.g. The second is... Should this be optional? I'm not convinced that's the correct way to represent the GCS API. I think defaulting to the current user is a very surprising behavior: since the API itself is for manipulating roles, the current user must have some role already. In all likelihood, a user running role create is creating roles for other users and groups. In summary:
Contributor
There was a problem hiding this comment. Agreed on the first point, option = optional; argument = required. The second point is a change that I suggested to James. I'll ask in the gcs devchat; seems like a pretty domain specific usage question.
Member
There was a problem hiding this comment.
Maybe this is where our perspectives diverge. I don't think it's purely domain specific. I think we should consider this in the context of the following commands:
Of these,
Contributor
There was a problem hiding this comment. gcs weighed in, let's go back to your original behavior James (no defaulting to the current user). Sorry for the flip-flop. From Jason:
|
||
| @click.option( | ||
| "--principal-type", | ||
| type=click.Choice(["identity", "group"]), | ||
| help="Qualifier to specify what type of principal (identity or group) is provided.", | ||
| ) | ||
|
Member
There was a problem hiding this comment. Note For any other reviewers, this That said, I'm very much interested in developing a better interface for passing these inputs. I think we could design something as a replacement which takes URNs, short-forms like |
||
| @LoginManager.requires_login("transfer") | ||
| def create_command( | ||
| login_manager: LoginManager, | ||
| *, | ||
| collection_id: uuid.UUID, | ||
| role: t.Literal[ | ||
| "access_manager", "activity_manager", "activity_monitor", "administrator" | ||
| ], | ||
| principal: str | None, | ||
| principal_type: t.Literal["identity", "group"] | None, | ||
| ) -> None: | ||
| """ | ||
| Create a role assignment on a Collection. | ||
| ROLE must be one of: | ||
| "administrator", | ||
| "access_manager", | ||
| "activity_manager", | ||
| "activity_monitor" | ||
| If a PRINCIPAL value is not provided the primary identity of the logged in user will | ||
| be used. | ||
| If a PRINCIPAL value is provided, it must be a username, UUID, or URN associated | ||
| with a globus identity or group. | ||
| If UUID, use `--principal-type` to specify the type (defaults to "identity"). | ||
| """ | ||
|
|
||
| gcs_client = login_manager.get_gcs_client(collection_id=collection_id) | ||
| auth_client = login_manager.get_auth_client() | ||
|
|
||
| # If Principal argument isn't provided, determine user's primary identity | ||
| if principal is None: | ||
| userinfo = auth_client.userinfo() | ||
| principal = userinfo["sub"] | ||
|
|
||
| # Format the principal into a URN | ||
| principal_urn = resolve_principal_urn( | ||
| auth_client=auth_client, | ||
| principal_type=principal_type, | ||
| principal=principal, | ||
| ) | ||
|
|
||
| res = gcs_client.create_role( | ||
| globus_sdk.GCSRoleDocument( | ||
| DATA_TYPE="role#1.0.0", | ||
| collection=collection_id, | ||
| role=role, | ||
| principal=principal_urn, | ||
| ) | ||
| ) | ||
|
|
||
| fields = collection_role_format_fields(auth_client, res.data) | ||
|
|
||
| display(res, text_mode=display.RECORD, fields=fields) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| from globus_sdk.testing import load_response_set | ||
|
|
||
|
|
||
| def test_successful_gcs_collection_role_creation( | ||
| run_line, | ||
| add_gcs_login, | ||
| get_identities_mocker, | ||
| ): | ||
| # setup data for the collection_id -> endpoint_id lookup | ||
| # and create dummy credentials for the test to run against that GCS | ||
| meta = load_response_set("cli.collection_operations").metadata | ||
|
|
||
| collection_id = meta["mapped_collection_id"] | ||
| endpoint_id = meta["endpoint_id"] | ||
| role = meta["role"] | ||
| role_id = meta["role_id"] | ||
| user_id = meta["identity_id"] | ||
| add_gcs_login(endpoint_id) | ||
|
|
||
| role = "activity_monitor" | ||
|
|
||
| # Mock the Get Identities API (Auth) | ||
| # so that CLI output rendering can show a username | ||
| user_meta = get_identities_mocker.configure_one(id=user_id).metadata | ||
| username = user_meta["username"] | ||
|
|
||
| # now test the command and confirm that a successful role creation is reported | ||
| run_line( | ||
| ["globus", "gcs", "collection", "role", "create", collection_id, role, user_id], | ||
| search_stdout=[ | ||
| ("ID", role_id), | ||
| ("Role", role), | ||
| ("Principal", username), | ||
| ], | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear to me why this additional formatter exists. Could you help me understand why it's here?
By my understanding, all three of these calls pass strings, which are then iterated on with an attempt to parse each character in those strings:
globus-cli/src/globus_cli/termio/formatters/auth.py
Lines 61 to 68 in e7d2804
Since no single character will successfully parse as a principal URN, I'd expect the three calls to do nothing.
I see it used below in a field where it is formatting the
principalfield in a response. Would aPrincipalURNFormatter, which is what was in use inrole listprior to this PR, not suffice?