[WS CLI] Make env --scope=user compatible with GitLab groups (#20279)

* [WS CLI] Make `env --scope=user` compatible with GitLab groups * Allow updating `*/*` scoped vars for backwards compat * Fix test races
gitpod-io · Oct 22, 2024 · 810f5e9 · 810f5e9
1 parent 45b822f
commit 810f5e9
Show file tree

Hide file tree

Showing 3 changed files with 48 additions and 13 deletions.
diff --git a/components/gitpod-cli/cmd/env.go b/components/gitpod-cli/cmd/env.go
@@ -34,8 +34,9 @@ var scope = string(envScopeRepo)
 type envScope string
 
 var (
-	envScopeRepo envScope = "repo"
-	envScopeUser envScope = "user"
+	envScopeRepo       envScope = "repo"
+	envScopeUser       envScope = "user"
+	envScopeLegacyUser envScope = "legacy-user"
 )
 
 func envScopeFromString(s string) envScope {
@@ -148,6 +149,11 @@ func connectToServer(ctx context.Context, options *connectToServerOptions) (*con
 
 	operations := "create/get/update/delete"
 	if options != nil && options.setEnvScope == envScopeUser {
+		// Updating user env vars requires a different token with a special scope
+		repositoryPattern = "*/**"
+		operations = "update"
+	}
+	if options != nil && options.setEnvScope == envScopeLegacyUser {
 		// Updating user env vars requires a different token with a special scope
 		repositoryPattern = "*/*"
 		operations = "update"
@@ -228,11 +234,25 @@ func setEnvs(ctx context.Context, setEnvScope envScope, args []string) error {
 			err = result.client.SetEnvVar(ctx, v)
 			if err != nil {
 				if ferr, ok := err.(*jsonrpc2.Error); ok && ferr.Code == http.StatusForbidden && setEnvScope == envScopeUser {
-					return fmt.Errorf(""+
-						"Can't automatically create env var `%s` for security reasons.\n"+
-						"Please create the var manually under %s/user/variables using Name=%s, Scope=*/*, Value=foobar", v.Name, result.gitpodHost, v.Name)
+					// If we tried updating an env var with */** and it doesn't exist, it may exist with the */* scope
+					options.setEnvScope = envScopeLegacyUser
+					result, err := connectToServer(ctx, &options)
+					if err != nil {
+						return err
+					}
+					defer result.client.Close()
+
+					v.RepositoryPattern = "*/*"
+					err = result.client.SetEnvVar(ctx, v)
+					if ferr, ok := err.(*jsonrpc2.Error); ok && ferr.Code == http.StatusForbidden {
+						fmt.Println(ferr.Message, ferr.Data)
+						return fmt.Errorf(""+
+							"Can't automatically create env var `%s` for security reasons.\n"+
+							"Please create the var manually under %s/user/variables using Name=%s, Scope=*/**, Value=foobar", v.Name, result.gitpodHost, v.Name)
+					}
+				} else {
+					return err
 				}
-				return err
 			}
 			printVar(v.Name, v.Value, exportEnvs)
 			return nil

diff --git a/components/server/src/workspace/workspace-service.spec.db.ts b/components/server/src/workspace/workspace-service.spec.db.ts
@@ -681,13 +681,15 @@ describe("WorkspaceService", async () => {
         const svc = container.get(WorkspaceService);
         const db = container.get<WorkspaceDB>(WorkspaceDB);
         const today = new Date();
-        const daysAgo = (days: number) => new Date(today.getTime() - 1000 * 60 * 60 * 24 * days);
+        const daysAgo = (days: number, from = today) => new Date(from.getTime() - 1000 * 60 * 60 * 24 * days);
 
         const ws = await createTestWorkspace(svc, org, owner, project);
-        await db.storeInstance({
+        await svc.updateDeletionEligibilityTime(owner.id, ws.id, true);
+        const instance = await db.storeInstance({
             id: v4(),
             workspaceId: ws.id,
-            creationTime: daysAgo(7).toISOString(),
+            // tomorrow, because as of #20271 deletion eligibility times can't go backwards and those are set when creating a ws
+            creationTime: daysAgo(-1).toISOString(),
             status: {
                 conditions: {},
                 phase: "stopped",
@@ -705,17 +707,19 @@ describe("WorkspaceService", async () => {
         const workspace = await svc.getWorkspace(owner.id, ws.id);
         expect(workspace).to.not.be.undefined;
         expect(workspace.workspace.deletionEligibilityTime).to.not.be.undefined;
-        expect(workspace.workspace.deletionEligibilityTime).to.eq(daysAgo(7 - 14).toISOString());
+        expect(workspace.workspace.deletionEligibilityTime).to.eq(
+            daysAgo(-14, new Date(instance.creationTime)).toISOString(),
+        );
     });
 
     it("should update the deletion eligibility time of a workspace with git changes", async () => {
         const svc = container.get(WorkspaceService);
         const db = container.get<WorkspaceDB>(WorkspaceDB);
         const today = new Date();
-        const daysAgo = (days: number) => new Date(today.getTime() - 1000 * 60 * 60 * 24 * days);
+        const daysAgo = (days: number, from = today) => new Date(from.getTime() - 1000 * 60 * 60 * 24 * days);
 
         const ws = await createTestWorkspace(svc, org, owner, project);
-        await db.storeInstance({
+        const instance = await db.storeInstance({
             id: v4(),
             workspaceId: ws.id,
             creationTime: daysAgo(7).toISOString(),
@@ -739,7 +743,9 @@ describe("WorkspaceService", async () => {
         const workspace = await svc.getWorkspace(owner.id, ws.id);
         expect(workspace).to.not.be.undefined;
         expect(workspace.workspace.deletionEligibilityTime).to.not.be.undefined;
-        expect(workspace.workspace.deletionEligibilityTime).to.eq(daysAgo(7 - 14 * 2).toISOString());
+        expect(workspace.workspace.deletionEligibilityTime).to.eq(
+            daysAgo(-14 * 2, new Date(instance.creationTime)).toISOString(),
+        );
     });
 
     it("should update the deletion eligibility time of a prebuild", async () => {

diff --git a/components/server/src/workspace/workspace-starter.ts b/components/server/src/workspace/workspace-starter.ts
@@ -1720,6 +1720,15 @@ export class WorkspaceStarter {
             );
         }
         // The only exception is "updates", which we allow to be made to all env vars (that exist).
+        scopes.push(
+            "resource:" +
+                ScopedResourceGuard.marshalResourceScope({
+                    kind: "envVar",
+                    subjectID: "*/**",
+                    operations: ["update"],
+                }),
+        );
+        // For updating environment variables created with */* instead of */**, we fall back to updating those
         scopes.push(
             "resource:" +
                 ScopedResourceGuard.marshalResourceScope({