From ca990817795a8c907523a7155c2897d294e2a553 Mon Sep 17 00:00:00 2001 From: Daniyal Arshad Date: Fri, 27 Dec 2024 21:18:10 -0500 Subject: [PATCH 01/10] Add new workspace_variables query API - Add migration for new user_provided field in workspaces table - Add new type for workspace_variables - Add workspace_variables field to Workspace type - Add new WorkspaceVariableTypeEnum Changelog: changed EE: true --- ...dd_user_provided_to_workspace_variables.rb | 9 + ...kfill_user_provided_workspace_variables.rb | 51 ++++ db/schema_migrations/20241227205133 | 1 + db/schema_migrations/20241227233633 | 1 + db/structure.sql | 1 + doc/api/graphql/reference/index.md | 54 ++++ .../workspace_variables_finder.rb | 40 +++ .../admin_workspaces_resolver.rb | 27 +- .../workspace_variables_resolver.rb | 49 ++++ .../remote_development/workspace_type.rb | 10 + .../workspace_variable_type.rb | 44 ++++ .../workspace_variable_type_enum.rb | 15 ++ ee/app/models/remote_development/workspace.rb | 3 + .../remote_development/workspace_variable.rb | 9 + .../create/workspace_variables.rb | 1 + .../remote_development/workspace_variables.rb | 1 + .../workspace_variables_finder_spec.rb | 238 ++++++++++++++++++ ee/spec/graphql/types/query_type_spec.rb | 1 + .../workspace_variable_type_spec.rb | 17 ++ .../create/workspace_variables_spec.rb | 2 + .../remote_development/integration_spec.rb | 15 +- ..._user_provided_workspace_variables_spec.rb | 116 +++++++++ 22 files changed, 692 insertions(+), 13 deletions(-) create mode 100644 db/migrate/20241227205133_add_user_provided_to_workspace_variables.rb create mode 100644 db/migrate/20241227233633_backfill_user_provided_workspace_variables.rb create mode 100644 db/schema_migrations/20241227205133 create mode 100644 db/schema_migrations/20241227233633 create mode 100644 ee/app/finders/remote_development/workspace_variables_finder.rb create mode 100644 ee/app/graphql/resolvers/remote_development/workspace_variables_resolver.rb create mode 100644 ee/app/graphql/types/remote_development/workspace_variable_type.rb create mode 100644 ee/app/graphql/types/remote_development/workspace_variable_type_enum.rb create mode 100644 ee/spec/finders/remote_development/workspace_variables_finder_spec.rb create mode 100644 ee/spec/graphql/types/remote_development/workspace_variable_type_spec.rb create mode 100644 spec/migrations/20241227233633_backfill_user_provided_workspace_variables_spec.rb diff --git a/db/migrate/20241227205133_add_user_provided_to_workspace_variables.rb b/db/migrate/20241227205133_add_user_provided_to_workspace_variables.rb new file mode 100644 index 00000000000000..962a1e95c616cf --- /dev/null +++ b/db/migrate/20241227205133_add_user_provided_to_workspace_variables.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +class AddUserProvidedToWorkspaceVariables < Gitlab::Database::Migration[2.2] + milestone '17.8' + + def change + add_column :workspace_variables, :user_provided, :boolean, null: false, default: false + end +end diff --git a/db/migrate/20241227233633_backfill_user_provided_workspace_variables.rb b/db/migrate/20241227233633_backfill_user_provided_workspace_variables.rb new file mode 100644 index 00000000000000..c24ae8e5161e47 --- /dev/null +++ b/db/migrate/20241227233633_backfill_user_provided_workspace_variables.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +class BackfillUserProvidedWorkspaceVariables < Gitlab::Database::Migration[2.2] + milestone '17.8' + + restrict_gitlab_migration gitlab_schema: :gitlab_main + + class WorkspaceVariable < MigrationRecord + self.table_name = :workspace_variables + + include EachBatch + end + + BATCH_SIZE = 100 + + # Static workspace variables that get created on workspace creation + # Reference: /ee/lib/remote_development/workspace_operations/create/workspace_variables.rb + WORKSPACE_STATIC_VARIABLES = %w[ + GIT_CONFIG_COUNT + GIT_CONFIG_KEY_0 + GIT_CONFIG_VALUE_0 + GIT_CONFIG_KEY_1 + GIT_CONFIG_VALUE_1 + GIT_CONFIG_KEY_2 + GIT_CONFIG_VALUE_2 + GL_GIT_CREDENTIAL_STORE_FILE_PATH + GL_TOKEN_FILE_PATH + GL_WORKSPACE_DOMAIN_TEMPLATE + GL_EDITOR_EXTENSIONS_GALLERY_SERVICE_URL + GL_EDITOR_EXTENSIONS_GALLERY_ITEM_URL + GL_EDITOR_EXTENSIONS_GALLERY_RESOURCE_URL_TEMPLATE + GITLAB_WORKFLOW_INSTANCE_URL + GITLAB_WORKFLOW_TOKEN_FILE + ].freeze + + def up + WorkspaceVariable.reset_column_information + + WorkspaceVariable.where.not(key: WORKSPACE_STATIC_VARIABLES) + .each_batch(of: BATCH_SIZE) do |batch| + batch.update_all(user_provided: true) + end + end + + def down + WorkspaceVariable.reset_column_information + + # Column is NOT NULL DEFAULT 0, so setting back to default + WorkspaceVariable.update_all(user_provided: false) + end +end diff --git a/db/schema_migrations/20241227205133 b/db/schema_migrations/20241227205133 new file mode 100644 index 00000000000000..d085df2fb451df --- /dev/null +++ b/db/schema_migrations/20241227205133 @@ -0,0 +1 @@ +58154f7eb7459696337b9c4c3caed522982a65c98d7008204c22aefd492e86ff \ No newline at end of file diff --git a/db/schema_migrations/20241227233633 b/db/schema_migrations/20241227233633 new file mode 100644 index 00000000000000..805e7c7e5c1c30 --- /dev/null +++ b/db/schema_migrations/20241227233633 @@ -0,0 +1 @@ +26a106689473a03e95608a79eba275fdb7867ff235b62857dc8457d740b9b709 \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index ec51c6336aeec4..ce302baf8b52e4 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -23254,6 +23254,7 @@ CREATE TABLE workspace_variables ( encrypted_value bytea NOT NULL, encrypted_value_iv bytea NOT NULL, project_id bigint, + user_provided boolean DEFAULT false NOT NULL, CONSTRAINT check_5545042100 CHECK ((char_length(key) <= 255)), CONSTRAINT check_ed95da8691 CHECK ((project_id IS NOT NULL)) ); diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index 3d68d696080446..2620708c744d16 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -18344,6 +18344,29 @@ The edge type for [`Workspace`](#workspace). | `cursor` | [`String!`](#string) | A cursor for use in pagination. | | `node` | [`Workspace`](#workspace) | The item at the end of the edge. | +#### `WorkspaceVariableConnection` + +The connection type for [`WorkspaceVariable`](#workspacevariable). + +##### Fields + +| Name | Type | Description | +| ---- | ---- | ----------- | +| `edges` | [`[WorkspaceVariableEdge]`](#workspacevariableedge) | A list of edges. | +| `nodes` | [`[WorkspaceVariable]`](#workspacevariable) | A list of nodes. | +| `pageInfo` | [`PageInfo!`](#pageinfo) | Information to aid in pagination. | + +#### `WorkspaceVariableEdge` + +The edge type for [`WorkspaceVariable`](#workspacevariable). + +##### Fields + +| Name | Type | Description | +| ---- | ---- | ----------- | +| `cursor` | [`String!`](#string) | A cursor for use in pagination. | +| `node` | [`WorkspaceVariable`](#workspacevariable) | The item at the end of the edge. | + ## Object types Object types represent the resources that the GitLab GraphQL API can return. @@ -38476,8 +38499,25 @@ Represents a remote development workspace. | `updatedAt` | [`Time!`](#time) | Timestamp of the last update to any mutable workspace property. | | `url` | [`String!`](#string) | URL of the workspace. | | `user` | [`UserCore!`](#usercore) | Owner of the workspace. | +| `workspaceVariables` **{warning-solid}** | [`WorkspaceVariableConnection`](#workspacevariableconnection) | **Introduced** in GitLab 17.8. **Status**: Experiment. User defined variables associated with the workspace. | | `workspacesAgentConfigVersion` **{warning-solid}** | [`Int!`](#int) | **Introduced** in GitLab 17.6. **Status**: Experiment. Version of the associated WorkspacesAgentConfig for the workspace. | +### `WorkspaceVariable` + +Represents a remote development workspace variable. + +#### Fields + +| Name | Type | Description | +| ---- | ---- | ----------- | +| `createdAt` **{warning-solid}** | [`Time!`](#time) | **Introduced** in GitLab 17.8. **Status**: Experiment. Timestamp of when the workspace variable was created. | +| `id` **{warning-solid}** | [`RemoteDevelopmentWorkspaceVariableID!`](#remotedevelopmentworkspacevariableid) | **Introduced** in GitLab 17.8. **Status**: Experiment. Global ID of the workspace variable. | +| `key` **{warning-solid}** | [`String`](#string) | **Introduced** in GitLab 17.8. **Status**: Experiment. Name of the workspace variable. | +| `projectId` **{warning-solid}** | [`ID!`](#id) | **Introduced** in GitLab 17.8. **Status**: Experiment. ID of the project that contains the devfile for the workspace. | +| `updatedAt` **{warning-solid}** | [`Time!`](#time) | **Introduced** in GitLab 17.8. **Status**: Experiment. Timestamp of when the workspace variable was updated. | +| `value` **{warning-solid}** | [`String`](#string) | **Introduced** in GitLab 17.8. **Status**: Experiment. Value of the workspace variable. | +| `variableType` **{warning-solid}** | [`WorkspaceVariableType`](#workspacevariabletype) | **Introduced** in GitLab 17.8. **Status**: Experiment. Type of the workspace variable. | + ### `WorkspacesAgentConfig` Represents a workspaces agent config. @@ -42177,6 +42217,14 @@ Enum for the type of the variable to be injected in a workspace. | ----- | ----------- | | `ENVIRONMENT` | Name type. | +### `WorkspaceVariableType` + +Enum for the type of the variable injected in a workspace. + +| Value | Description | +| ----- | ----------- | +| `ENVIRONMENT` | Name type. | + ## Scalar types Scalar values are atomic values, and do not have fields of their own. @@ -43149,6 +43197,12 @@ A `RemoteDevelopmentWorkspaceID` is a global ID. It is encoded as a string. An example `RemoteDevelopmentWorkspaceID` is: `"gid://gitlab/RemoteDevelopment::Workspace/1"`. +### `RemoteDevelopmentWorkspaceVariableID` + +A `RemoteDevelopmentWorkspaceVariableID` is a global ID. It is encoded as a string. + +An example `RemoteDevelopmentWorkspaceVariableID` is: `"gid://gitlab/RemoteDevelopment::WorkspaceVariable/1"`. + ### `RemoteDevelopmentWorkspacesAgentConfigID` A `RemoteDevelopmentWorkspacesAgentConfigID` is a global ID. It is encoded as a string. diff --git a/ee/app/finders/remote_development/workspace_variables_finder.rb b/ee/app/finders/remote_development/workspace_variables_finder.rb new file mode 100644 index 00000000000000..20f72aad6cd863 --- /dev/null +++ b/ee/app/finders/remote_development/workspace_variables_finder.rb @@ -0,0 +1,40 @@ +# frozen_string_literal: true + +module RemoteDevelopment + class WorkspaceVariablesFinder + # Executes a query to find user provided workspace variables. + # + # @param [User, QA::Resource::User] current_user The user making the request. Must have permission + # to access workspaces. + # @param [Array] ids A list of specific WorkspaceVariable IDs to filter by (optional). + # @param [Array] project_ids A list of Project IDs to filter by (optional). + # @param [Array] workspace_ids A list of Workspace IDs to filter by (optional). + # @return [ActiveRecord::Relation] + # A collection of filtered RemoteDevelopment::WorkspaceVariable records ordered by ID descending. + def self.execute(current_user:, ids: [], project_ids: [], workspace_ids: []) + return WorkspaceVariable.none unless current_user.can?(:access_workspaces_feature) + + filter_arguments = { + ids: ids, + project_ids: project_ids, + workspace_ids: workspace_ids + } + + filter_argument_types = { + ids: Integer, + project_ids: Integer, + workspace_ids: Integer + }.freeze + + FilterArgumentValidator.validate_filter_argument_types!(filter_argument_types, filter_arguments) + FilterArgumentValidator.validate_at_least_one_filter_argument_provided!(**filter_arguments) + + collection_proxy = WorkspaceVariable.user_provided + collection_proxy = collection_proxy.id_in(ids) if ids.present? + collection_proxy = collection_proxy.by_project_ids(project_ids) if project_ids.present? + collection_proxy = collection_proxy.by_workspace_ids(workspace_ids) if workspace_ids.present? + + collection_proxy.order_id_desc + end + end +end diff --git a/ee/app/graphql/resolvers/remote_development/admin_workspaces_resolver.rb b/ee/app/graphql/resolvers/remote_development/admin_workspaces_resolver.rb index ff856113f96377..c9fa3e61d08997 100644 --- a/ee/app/graphql/resolvers/remote_development/admin_workspaces_resolver.rb +++ b/ee/app/graphql/resolvers/remote_development/admin_workspaces_resolver.rb @@ -4,6 +4,7 @@ module Resolvers module RemoteDevelopment class AdminWorkspacesResolver < ::Resolvers::BaseResolver include ResolvesIds + include LooksAhead # NOTE: We are intentionally not including Gitlab::Graphql::Authorize::AuthorizeResource, because this resolver # is currently only authorized at the instance admin level for all workspaces in the instance via the @@ -11,6 +12,8 @@ class AdminWorkspacesResolver < ::Resolvers::BaseResolver # Also, including Gitlab::Graphql::Authorize::AuthorizeResource would mix in a many methods related to # "resource" and "object" which are not applicable in this resolver, so we avoid including it and keep # the dependencies of this class more minimal. + # + extras [:lookahead] type Types::RemoteDevelopment::WorkspaceType.connection_type, null: true @@ -40,7 +43,7 @@ class AdminWorkspacesResolver < ::Resolvers::BaseResolver required: false, description: 'Filter workspaces by actual states.' - def resolve(**args) + def resolve_with_lookahead(**args) # rubocop:disable Graphql/ResourceNotAvailableError -- We are intentionally not including Gitlab::Graphql::Authorize::AuthorizeResource - see note at top of class unless License.feature_available?(:remote_development) raise ::Gitlab::Graphql::Errors::ResourceNotAvailable, @@ -51,19 +54,27 @@ def resolve(**args) return ::RemoteDevelopment::Workspace.none unless can_read_all_workspaces? begin - ::RemoteDevelopment::WorkspacesFinder.execute( - current_user: current_user, - ids: resolve_ids(args[:ids]).map(&:to_i), - user_ids: resolve_ids(args[:user_ids]).map(&:to_i), - project_ids: resolve_ids(args[:project_ids]).map(&:to_i), - agent_ids: resolve_ids(args[:agent_ids]).map(&:to_i), - actual_states: args[:actual_states] || args[:include_actual_states] || [] + apply_lookahead( + ::RemoteDevelopment::WorkspacesFinder.execute( + current_user: current_user, + ids: resolve_ids(args[:ids]).map(&:to_i), + user_ids: resolve_ids(args[:user_ids]).map(&:to_i), + project_ids: resolve_ids(args[:project_ids]).map(&:to_i), + agent_ids: resolve_ids(args[:agent_ids]).map(&:to_i), + actual_states: args[:actual_states] || args[:include_actual_states] || [] + ) ) rescue ArgumentError => e raise ::Gitlab::Graphql::Errors::ArgumentError, e.message end end + def preloads + { + workspace_variables: [:user_provided_workspace_variables] + } + end + private def can_read_all_workspaces? diff --git a/ee/app/graphql/resolvers/remote_development/workspace_variables_resolver.rb b/ee/app/graphql/resolvers/remote_development/workspace_variables_resolver.rb new file mode 100644 index 00000000000000..2bc34695a17ccb --- /dev/null +++ b/ee/app/graphql/resolvers/remote_development/workspace_variables_resolver.rb @@ -0,0 +1,49 @@ +# frozen_string_literal: true + +module Resolvers + module RemoteDevelopment + class WorkspaceVariablesResolver < ::Resolvers::BaseResolver + include LooksAhead + include ResolvesIds + include Gitlab::Graphql::Authorize::AuthorizeResource + + type Types::RemoteDevelopment::WorkspaceVariableType.connection_type, null: true + + argument :ids, [::Types::GlobalIDType[::RemoteDevelopment::WorkspaceVariable]], + required: false, + experiment: { milestone: '17.8' }, + description: + 'Filter workspace variable by workspace variable GlobalIDs.' \ + 'For example, `["gid://gitlab/RemoteDevelopment::WorkspaceVariable/1"]`.' + + argument :project_ids, [::Types::GlobalIDType[Project]], + required: false, + experiment: { milestone: '17.8' }, + description: 'Filter workspaces variables by project GlobalIDs.' + + argument :workspace_ids, [::Types::GlobalIDType[::RemoteDevelopment::Workspace]], + required: false, + experiment: { milestone: '17.8' }, + description: 'Filter workspaces by workspace GlobalIDs.' + + def resolve_with_lookahead(**args) + unless License.feature_available?(:remote_development) + raise_resource_not_available_error! "'remote_development' licensed feature is not available" + end + + ::RemoteDevelopment::WorkspaceVariablesFinder.execute( + current_user: current_user, + ids: resolve_ids(args[:ids]).map(&:to_i), + project_ids: resolve_ids(args[:project_ids]).map(&:to_i), + workspace_ids: resolve_ids(args[:workspace_ids]).map(&:to_i) + ) + end + + def preloads + { + workspace: [:workspace] + } + end + end + end +end diff --git a/ee/app/graphql/types/remote_development/workspace_type.rb b/ee/app/graphql/types/remote_development/workspace_type.rb index 503ec2459d6386..742ff190e10567 100644 --- a/ee/app/graphql/types/remote_development/workspace_type.rb +++ b/ee/app/graphql/types/remote_development/workspace_type.rb @@ -101,6 +101,12 @@ class WorkspaceType < ::Types::BaseObject field :updated_at, Types::TimeType, null: false, description: 'Timestamp of the last update to any mutable workspace property.' + field :workspace_variables, + ::Types::RemoteDevelopment::WorkspaceVariableType.connection_type, + null: true, + experiment: { milestone: '17.8' }, + description: 'User defined variables associated with the workspace.' + def project_id "gid://gitlab/Project/#{object.project_id}" end @@ -108,6 +114,10 @@ def project_id def editor 'webide' end + + def workspace_variables + object.user_provided_workspace_variables if object.is_a?(::RemoteDevelopment::Workspace) + end end end end diff --git a/ee/app/graphql/types/remote_development/workspace_variable_type.rb b/ee/app/graphql/types/remote_development/workspace_variable_type.rb new file mode 100644 index 00000000000000..b9fcdcaddb7e0f --- /dev/null +++ b/ee/app/graphql/types/remote_development/workspace_variable_type.rb @@ -0,0 +1,44 @@ +# frozen_string_literal: true + +module Types + module RemoteDevelopment + class WorkspaceVariableType < ::Types::BaseObject + graphql_name 'WorkspaceVariable' + description 'Represents a remote development workspace variable' + + authorize :read_workspace_variable + + field :id, ::Types::GlobalIDType[::RemoteDevelopment::WorkspaceVariable], + experiment: { milestone: '17.8' }, + null: false, description: 'Global ID of the workspace variable.' + + field :project_id, GraphQL::Types::ID, + experiment: { milestone: '17.8' }, + null: false, description: 'ID of the project that contains the devfile for the workspace.' + + field :key, GraphQL::Types::String, + experiment: { milestone: '17.8' }, + null: true, description: 'Name of the workspace variable.' + + field :value, GraphQL::Types::String, + experiment: { milestone: '17.8' }, + null: true, description: 'Value of the workspace variable.' + + field :variable_type, Types::RemoteDevelopment::WorkspaceVariableTypeEnum, + experiment: { milestone: '17.8' }, + null: true, description: 'Type of the workspace variable.' + + field :created_at, Types::TimeType, + experiment: { milestone: '17.8' }, + null: false, description: 'Timestamp of when the workspace variable was created.' + + field :updated_at, Types::TimeType, + experiment: { milestone: '17.8' }, + null: false, description: 'Timestamp of when the workspace variable was updated.' + + def project_id + "gid://gitlab/Project/#{object.project_id}" + end + end + end +end diff --git a/ee/app/graphql/types/remote_development/workspace_variable_type_enum.rb b/ee/app/graphql/types/remote_development/workspace_variable_type_enum.rb new file mode 100644 index 00000000000000..0689166ba8edeb --- /dev/null +++ b/ee/app/graphql/types/remote_development/workspace_variable_type_enum.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +module Types + module RemoteDevelopment + class WorkspaceVariableTypeEnum < BaseEnum + graphql_name 'WorkspaceVariableType' + description 'Enum for the type of the variable injected in a workspace.' + + from_rails_enum( + ::RemoteDevelopment::Enums::Workspace::WORKSPACE_VARIABLE_TYPES_FOR_GRAPHQL, + description: "#{%(name).capitalize} type." + ) + end + end +end diff --git a/ee/app/models/remote_development/workspace.rb b/ee/app/models/remote_development/workspace.rb index 1acb86e716c7d1..df096ae2bf29f0 100644 --- a/ee/app/models/remote_development/workspace.rb +++ b/ee/app/models/remote_development/workspace.rb @@ -23,6 +23,9 @@ class Workspace < ApplicationRecord # noinspection RailsParamDefResolve - https://handbook.gitlab.com/handbook/tools-and-tips/editors-and-ides/jetbrains-ides/tracked-jetbrains-issues/#ruby-31542 has_one :remote_development_agent_config, through: :agent, source: :remote_development_agent_config has_many :workspace_variables, class_name: 'RemoteDevelopment::WorkspaceVariable', inverse_of: :workspace + has_many :user_provided_workspace_variables, -> { + user_provided + }, class_name: 'RemoteDevelopment::WorkspaceVariable', inverse_of: :workspace validates :user, presence: true validates :agent, presence: true diff --git a/ee/app/models/remote_development/workspace_variable.rb b/ee/app/models/remote_development/workspace_variable.rb index 30466b091159b6..890ce302128cac 100644 --- a/ee/app/models/remote_development/workspace_variable.rb +++ b/ee/app/models/remote_development/workspace_variable.rb @@ -2,6 +2,8 @@ module RemoteDevelopment class WorkspaceVariable < ApplicationRecord + include Sortable + belongs_to :workspace, class_name: 'RemoteDevelopment::Workspace', inverse_of: :workspace_variables validates :variable_type, presence: true, inclusion: { @@ -19,6 +21,13 @@ class WorkspaceVariable < ApplicationRecord where(variable_type: RemoteDevelopment::Enums::Workspace::WORKSPACE_VARIABLE_TYPES[:file]) } + scope :by_workspace_ids, ->(ids) { where(workspace_id: ids) } + scope :by_project_ids, ->(ids) { where(project_id: ids) } + + scope :user_provided, -> { + where(user_provided: true) + } + attr_encrypted :value, mode: :per_attribute_iv, key: ::Settings.attr_encrypted_db_key_base_32, diff --git a/ee/lib/remote_development/workspace_operations/create/workspace_variables.rb b/ee/lib/remote_development/workspace_operations/create/workspace_variables.rb index 3a4ade8e84986c..495d88e89755b3 100644 --- a/ee/lib/remote_development/workspace_operations/create/workspace_variables.rb +++ b/ee/lib/remote_development/workspace_operations/create/workspace_variables.rb @@ -155,6 +155,7 @@ def self.variables( key: variable.fetch(:key), value: variable.fetch(:value), variable_type: variable.fetch(:type), + user_provided: true, workspace_id: workspace_id } end diff --git a/ee/spec/factories/remote_development/workspace_variables.rb b/ee/spec/factories/remote_development/workspace_variables.rb index 04b4ed3dcbffb1..a4b50e6d09b1a3 100644 --- a/ee/spec/factories/remote_development/workspace_variables.rb +++ b/ee/spec/factories/remote_development/workspace_variables.rb @@ -6,6 +6,7 @@ key { 'my_key' } value { 'my_value' } + user_provided { false } variable_type { RemoteDevelopment::Enums::Workspace::WORKSPACE_VARIABLE_TYPES[:file] } end end diff --git a/ee/spec/finders/remote_development/workspace_variables_finder_spec.rb b/ee/spec/finders/remote_development/workspace_variables_finder_spec.rb new file mode 100644 index 00000000000000..6f982fd54e471a --- /dev/null +++ b/ee/spec/finders/remote_development/workspace_variables_finder_spec.rb @@ -0,0 +1,238 @@ +# frozen_string_literal: true + +require "spec_helper" + +RSpec.describe RemoteDevelopment::WorkspaceVariablesFinder, feature_category: :workspaces do + include ::RemoteDevelopment::WorkspaceOperations::States + + let_it_be(:current_user) { create(:user) } + let_it_be(:other_user) { create(:user) } + + let_it_be(:cluster_admin_user) { create(:user) } + let_it_be(:agent_a) do + create(:ee_cluster_agent, :with_existing_workspaces_agent_config, created_by_user: cluster_admin_user) + end + + let_it_be(:agent_b) do + create(:ee_cluster_agent, :with_existing_workspaces_agent_config, created_by_user: cluster_admin_user) + end + + let_it_be(:project_a) { create(:project, :public) } + let_it_be(:project_b) { create(:project, :public) } + + let_it_be(:workspace_a) do + create(:workspace, user: current_user, updated_at: 2.days.ago, project: project_a, + actual_state: ::RemoteDevelopment::WorkspaceOperations::States::RUNNING, agent: agent_a + ) + end + + let_it_be(:other_users_workspace) do + create(:workspace, user: other_user, project: project_b, + actual_state: ::RemoteDevelopment::WorkspaceOperations::States::TERMINATED, agent: agent_b + ) + end + + let_it_be(:workspace_a_static_variable) do + create(:workspace_variable, + workspace_id: workspace_a.id, + key: 'GIT_CONFIG_COUNT', + value: 'static_var', + variable_type: 0, + user_provided: false + ) + end + + let_it_be(:workspace_a_user_variable_1) do + create(:workspace_variable, + workspace_id: workspace_a.id, + project_id: project_a.id, + key: 'USER_VAR_1', + value: 'user_var_1', + variable_type: 0, + user_provided: true + ) + end + + let_it_be(:workspace_a_user_variable_2) do + create(:workspace_variable, + workspace_id: workspace_a.id, + project_id: project_a.id, + key: 'USER_VAR_2', + value: 'user_var_2', + variable_type: 0, + user_provided: true + ) + end + + let_it_be(:other_user_workspace_variable) do + create(:workspace_variable, + workspace_id: other_users_workspace.id, + project_id: project_b.id, + key: 'OTHER_USER_VAR', + value: 'other_user_var', + variable_type: 0, + user_provided: true + ) + end + + subject(:collection_proxy) do + described_class.execute(current_user: current_user, **filter_arguments) + end + + before do + stub_licensed_features(remote_development: true) + allow(::RemoteDevelopment::FilterArgumentValidator).to receive_messages(validate_filter_argument_types!: true, + validate_at_least_one_filter_argument_provided!: true) + end + + describe "user provided workspace variables" do + context "when workspace has both static and user provided variables" do + let(:filter_arguments) { { workspace_ids: [workspace_a.id] } } + + it "returns only the user provided variables", :unlimited_max_formatted_output_length do + expect(collection_proxy).to contain_exactly(workspace_a_user_variable_1, workspace_a_user_variable_2) + expect(collection_proxy).not_to include(workspace_a_static_variable) + end + end + end + + describe "filter arguments" do + context "with ids argument" do + let(:filter_arguments) { { ids: [workspace_a_user_variable_1.id] } } + + it "returns only current user's workspace variables matching the specified IDs" do + expect(collection_proxy).to contain_exactly(workspace_a_user_variable_1) + expect(collection_proxy).not_to include(workspace_a_user_variable_2) + expect(collection_proxy).not_to include(other_user_workspace_variable) + end + end + + context "with project_ids argument" do + let(:filter_arguments) { { project_ids: [project_a.id] } } + + it "returns only workspace variables matching the specified project IDs" do + expect(collection_proxy).to contain_exactly(workspace_a_user_variable_1, workspace_a_user_variable_2) + end + end + + context "with workspace_ids argument" do + let(:filter_arguments) { { workspace_ids: [workspace_a.id] } } + + it "returns only workspace variables matching the specified agent IDs" do + expect(collection_proxy).to contain_exactly(workspace_a_user_variable_1, workspace_a_user_variable_2) + end + end + + context "with multiple arguments" do + let(:filter_arguments) do + { + ids: [workspace_a_user_variable_1.id, workspace_a_user_variable_2.id, other_user_workspace_variable.id, + workspace_a_static_variable.id], + project_ids: [project_a.id, project_b.id], + workspace_ids: [workspace_a.id, other_users_workspace.id] + } + end + + it "handles multiple arguments and still returns all workspaces which match all filter arguments", + :unlimited_max_formatted_output_length do + expect(collection_proxy).to contain_exactly(workspace_a_user_variable_1, workspace_a_user_variable_2, + other_user_workspace_variable) + end + end + + context "with extra empty filter arguments" do + let(:filter_arguments) do + { + ids: [workspace_a_user_variable_1.id], + project_ids: [], + workspace_ids: [] + } + end + + it "still uses existing filter arguments" do + expect(collection_proxy).to contain_exactly(workspace_a_user_variable_1) + end + end + end + + describe "ordering" do + context "with workspace_ids argument" do + let(:filter_arguments) { { workspace_ids: [workspace_a.id] } } + + it "returns workspace variables sorted by last updated time (most recent first)" do + expected_collection_proxy = [workspace_a_user_variable_2, workspace_a_user_variable_1] + expect(collection_proxy).to eq(expected_collection_proxy) + end + end + end + + describe "validations" do + context "when no filter arguments are provided" do + let(:filter_arguments) { {} } + + before do + allow(::RemoteDevelopment::FilterArgumentValidator).to receive( + :validate_at_least_one_filter_argument_provided! + ).and_raise(ArgumentError.new("At least one filter argument must be provided")) + end + + it "raises an ArgumentError" do + expect { collection_proxy }.to raise_error(ArgumentError, "At least one filter argument must be provided") + end + end + + context "when an invalid filter argument type is provided" do + let(:expected_exception_message) do + "'ids' must be an Array of 'Integer', " \ + "'project_ids' must be an Array of 'Integer', " \ + "'workspace_ids' must be an Array of 'Integer'" + end + + before do + allow(::RemoteDevelopment::FilterArgumentValidator).to receive( + :validate_filter_argument_types! + ).and_raise(RuntimeError.new(expected_exception_message)) + end + + context "when argument is not an array" do + let(:filter_arguments) do + { + ids: 1, + project_ids: 1, + workspace_ids: 1 + } + end + + it "raises an RuntimeError", :unlimited_max_formatted_output_length do + expect { collection_proxy.to_a }.to raise_error(RuntimeError, expected_exception_message) + end + end + + context "when array content is wrong type" do + let(:filter_arguments) do + { + ids: %w[a b], + project_ids: %w[a b], + workspace_ids: %w[a b] + } + end + + it "raises an RuntimeError", :unlimited_max_formatted_output_length do + expect { collection_proxy.to_a }.to raise_error(RuntimeError, expected_exception_message) + end + end + end + + context "when current_user does not have access_workspaces_feature ability (anonymous user)" do + let(:filter_arguments) { { ids: [workspace_a.id] } } + + before do + allow(current_user).to receive(:can?).with(:access_workspaces_feature).and_return(false) + end + + it "returns none" do + expect(collection_proxy).to be_blank + end + end + end +end diff --git a/ee/spec/graphql/types/query_type_spec.rb b/ee/spec/graphql/types/query_type_spec.rb index c555d49af29a59..bc2faca200e723 100644 --- a/ee/spec/graphql/types/query_type_spec.rb +++ b/ee/spec/graphql/types/query_type_spec.rb @@ -36,6 +36,7 @@ :vulnerability, :workspace, :workspaces, + :workspace_variables, :instance_external_audit_event_destinations, :instance_google_cloud_logging_configurations, :audit_events_instance_amazon_s3_configurations, diff --git a/ee/spec/graphql/types/remote_development/workspace_variable_type_spec.rb b/ee/spec/graphql/types/remote_development/workspace_variable_type_spec.rb new file mode 100644 index 00000000000000..8c503e98883dae --- /dev/null +++ b/ee/spec/graphql/types/remote_development/workspace_variable_type_spec.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe GitlabSchema.types['WorkspaceVariable'], feature_category: :workspaces do + let(:fields) do + %i[ + id project_id workspace key value variable_type created_at updated_at + ] + end + + specify { expect(described_class.graphql_name).to eq('WorkspaceVariable') } + + specify { expect(described_class).to have_graphql_fields(fields) } + + specify { expect(described_class).to require_graphql_authorizations(:read_workspace_variable) } +end diff --git a/ee/spec/lib/remote_development/workspace_operations/create/workspace_variables_spec.rb b/ee/spec/lib/remote_development/workspace_operations/create/workspace_variables_spec.rb index 5cf2f3451c86c3..23386173597e5a 100644 --- a/ee/spec/lib/remote_development/workspace_operations/create/workspace_variables_spec.rb +++ b/ee/spec/lib/remote_development/workspace_operations/create/workspace_variables_spec.rb @@ -142,12 +142,14 @@ { key: "VAR1", value: "value 1", + user_provided: true, variable_type: RemoteDevelopment::Enums::Workspace::WORKSPACE_VARIABLE_TYPES[:environment], workspace_id: workspace_id }, { key: "/path/to/file", value: "value 2", + user_provided: true, variable_type: RemoteDevelopment::Enums::Workspace::WORKSPACE_VARIABLE_TYPES[:file], workspace_id: workspace_id } diff --git a/ee/spec/requests/remote_development/integration_spec.rb b/ee/spec/requests/remote_development/integration_spec.rb index 184059b152b594..15e8f0e201d833 100644 --- a/ee/spec/requests/remote_development/integration_spec.rb +++ b/ee/spec/requests/remote_development/integration_spec.rb @@ -241,11 +241,16 @@ def do_create_workspace # NOTE: We convert the actual records into hashes and sort them as a hash rather than ordering in # ActiveRecord, to account for platform- or db-specific sorting differences. types = RemoteDevelopment::Enums::Workspace::WORKSPACE_VARIABLE_TYPES - all_actual_vars = - RemoteDevelopment::WorkspaceVariable - .where(workspace: workspace) - .map { |v| { key: v.key, type: types.invert[v.variable_type], value: v.value } } - .sort_by { |v| v[:key] } + all_actual_vars = RemoteDevelopment::WorkspaceVariable.where(workspace: workspace) + + actual_user_provided_vars = all_actual_vars.find_all { |var| var.user_provided == true } + + all_actual_vars = all_actual_vars.map { |v| { key: v.key, type: types.invert[v.variable_type], value: v.value } } + .sort_by { |v| v[:key] } + + # Check that user provided variables had their flag set correctly. + expect(actual_user_provided_vars.count).to eq(user_provided_variables.count) + expect(actual_user_provided_vars[0][:key]).to eq(user_provided_variables[0][:key]) # Check just keys first, to get an easy failure message if a new key has been added expect(all_actual_vars.pluck(:key)).to match_array(all_expected_vars.pluck(:key)) diff --git a/spec/migrations/20241227233633_backfill_user_provided_workspace_variables_spec.rb b/spec/migrations/20241227233633_backfill_user_provided_workspace_variables_spec.rb new file mode 100644 index 00000000000000..d37cef71becfff --- /dev/null +++ b/spec/migrations/20241227233633_backfill_user_provided_workspace_variables_spec.rb @@ -0,0 +1,116 @@ +# frozen_string_literal: true + +require 'spec_helper' +require_migration! + +RSpec.describe BackfillUserProvidedWorkspaceVariables, feature_category: :workspaces do + let(:user) { table(:users).create!(name: 'test-user', email: 'test@example.com', projects_limit: 5) } + let(:organization) { table(:organizations).create!(name: 'test-org', path: 'default') } + let(:namespace) { table(:namespaces).create!(name: 'name', path: 'path', organization_id: organization.id) } + let(:workspace_variables) { table(:workspace_variables) } + + let(:project) do + table(:projects).create!(namespace_id: namespace.id, project_namespace_id: namespace.id, + organization_id: organization.id) + end + + let!(:personal_access_token) do + table(:personal_access_tokens).create!( + user_id: user.id, + name: 'token_name', + organization_id: organization.id, + expires_at: Time.now + ) + end + + let(:cluster_agent) { table(:cluster_agents).create!(name: 'remotedev', project_id: project.id) } + + let!(:agent) do + table(:workspaces_agent_configs).create!( + cluster_agent_id: cluster_agent.id, + enabled: true, + dns_zone: 'test.workspace.me', + project_id: project.id + ) + end + + let!(:agent_config_version) do + table(:workspaces_agent_config_versions).create!( + project_id: project.id, + item_id: agent.id, + item_type: 'RemoteDevelopment::WorkspacesAgentConfig', + event: 'create' + ) + end + + let!(:workspace) do + table(:workspaces).create!( + user_id: user.id, + project_id: project.id, + cluster_agent_id: cluster_agent.id, + desired_state_updated_at: Time.now, + responded_to_agent_at: Time.now, + name: 'workspace-1', + namespace: 'workspace_1_namespace', + desired_state: 'Running', + actual_state: 'Running', + devfile_ref: 'devfile-ref', + devfile_path: 'devfile-path', + devfile: 'devfile', + processed_devfile: 'processed_dev_file', + url: 'workspace-url', + deployment_resource_version: 'v1', + personal_access_token_id: personal_access_token.id, + max_hours_before_termination: 5760, + workspaces_agent_config_version: agent_config_version.id, + desired_config_generator_version: 3 + ) + end + + let!(:workspace_static_file_variable) do + workspace_variables.create!( + workspace_id: workspace.id, + project_id: project.id, + key: 'gl_token', + variable_type: 1, + encrypted_value: 'encrypted_value', + encrypted_value_iv: 'encrypted_value_iv' + ) + end + + let!(:workspace_static_env_variable) do + workspace_variables.create!( + workspace_id: workspace.id, + project_id: project.id, + key: 'GIT_CONFIG_COUNT', + variable_type: 0, + encrypted_value: 'encrypted_value', + encrypted_value_iv: 'encrypted_value_iv' + ) + end + + let!(:workspace_user_provided_variable) do + workspace_variables.create!( + workspace_id: workspace.id, + project_id: project.id, + key: 'variable_key', + variable_type: 0, + encrypted_value: 'encrypted_value', + encrypted_value_iv: 'encrypted_value_iv' + ) + end + + it 'sets user_provided to true for all non-static environment variables in the table' do + reversible_migration do |migration| + migration.before -> { + expect(workspace_variables.pluck(:user_provided)).to all(be false) + } + + migration.after -> { + user_provided_variable = workspace_variables.where.not(key: described_class::WORKSPACE_STATIC_VARIABLES).first + + expect(user_provided_variable.user_provided).to be(true) + } + end + end +end -- GitLab From f320a061a334b8ecdb2dbccae24faa2f16e66f8f Mon Sep 17 00:00:00 2001 From: Daniyal Arshad Date: Tue, 7 Jan 2025 12:16:22 -0500 Subject: [PATCH 02/10] Optimize workspace_variables query to avoid N+1 - Apply lookahead in the workspaces resolvers - Refactor resolvers, introduce base resolver - Update specs --- ...kfill_user_provided_workspace_variables.rb | 5 +- doc/api/graphql/reference/index.md | 3 +- .../workspace_variables_finder.rb | 40 --- .../admin_workspaces_resolver.rb | 33 +-- .../cluster_agent/workspaces_resolver.rb | 36 +-- .../workspace_variables_resolver.rb | 49 ---- .../workspaces_base_resolver.rb | 44 ++++ .../remote_development/workspaces_resolver.rb | 38 +-- .../remote_development/workspace_type.rb | 6 +- .../workspace_variable_type.rb | 10 +- ee/app/models/remote_development/workspace.rb | 2 +- .../workspace_variable_policy.rb | 9 + .../workspace_variables_finder_spec.rb | 238 ------------------ ee/spec/graphql/types/query_type_spec.rb | 1 - .../remote_development/workspace_type_spec.rb | 1 + .../workspace_variable_type_spec.rb | 4 +- .../workspace_variable_policy_spec.rb | 38 +++ ..._user_provided_workspace_variables_spec.rb | 5 +- 18 files changed, 132 insertions(+), 430 deletions(-) delete mode 100644 ee/app/finders/remote_development/workspace_variables_finder.rb delete mode 100644 ee/app/graphql/resolvers/remote_development/workspace_variables_resolver.rb create mode 100644 ee/app/graphql/resolvers/remote_development/workspaces_base_resolver.rb create mode 100644 ee/app/policies/remote_development/workspace_variable_policy.rb delete mode 100644 ee/spec/finders/remote_development/workspace_variables_finder_spec.rb create mode 100644 ee/spec/policies/remote_development/workspace_variable_policy_spec.rb diff --git a/db/migrate/20241227233633_backfill_user_provided_workspace_variables.rb b/db/migrate/20241227233633_backfill_user_provided_workspace_variables.rb index c24ae8e5161e47..c677ea9020a0a0 100644 --- a/db/migrate/20241227233633_backfill_user_provided_workspace_variables.rb +++ b/db/migrate/20241227233633_backfill_user_provided_workspace_variables.rb @@ -13,6 +13,9 @@ class WorkspaceVariable < MigrationRecord BATCH_SIZE = 100 + # Since file type is not supported for user variables + VARIABLE_ENV_TYPE = 0 + # Static workspace variables that get created on workspace creation # Reference: /ee/lib/remote_development/workspace_operations/create/workspace_variables.rb WORKSPACE_STATIC_VARIABLES = %w[ @@ -36,7 +39,7 @@ class WorkspaceVariable < MigrationRecord def up WorkspaceVariable.reset_column_information - WorkspaceVariable.where.not(key: WORKSPACE_STATIC_VARIABLES) + WorkspaceVariable.where(variable_type: VARIABLE_ENV_TYPE).where.not(key: WORKSPACE_STATIC_VARIABLES) .each_batch(of: BATCH_SIZE) do |batch| batch.update_all(user_provided: true) end diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index 2620708c744d16..e7e7db43f6ba99 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -21331,7 +21331,7 @@ four standard [pagination arguments](#pagination-arguments): | ---- | ---- | ----------- | | `actualStates` | [`[String!]`](#string) | Filter workspaces by actual states. | | `ids` | [`[RemoteDevelopmentWorkspaceID!]`](#remotedevelopmentworkspaceid) | Filter workspaces by workspace GlobalIDs. For example, `["gid://gitlab/RemoteDevelopment::Workspace/1"]`. | -| `projectIds` | [`[ProjectID!]`](#projectid) | Filter workspaces by project GlobalID. | +| `projectIds` | [`[ProjectID!]`](#projectid) | Filter workspaces by project GlobalIDs. | ### `ClusterAgentActivityEvent` @@ -38513,7 +38513,6 @@ Represents a remote development workspace variable. | `createdAt` **{warning-solid}** | [`Time!`](#time) | **Introduced** in GitLab 17.8. **Status**: Experiment. Timestamp of when the workspace variable was created. | | `id` **{warning-solid}** | [`RemoteDevelopmentWorkspaceVariableID!`](#remotedevelopmentworkspacevariableid) | **Introduced** in GitLab 17.8. **Status**: Experiment. Global ID of the workspace variable. | | `key` **{warning-solid}** | [`String`](#string) | **Introduced** in GitLab 17.8. **Status**: Experiment. Name of the workspace variable. | -| `projectId` **{warning-solid}** | [`ID!`](#id) | **Introduced** in GitLab 17.8. **Status**: Experiment. ID of the project that contains the devfile for the workspace. | | `updatedAt` **{warning-solid}** | [`Time!`](#time) | **Introduced** in GitLab 17.8. **Status**: Experiment. Timestamp of when the workspace variable was updated. | | `value` **{warning-solid}** | [`String`](#string) | **Introduced** in GitLab 17.8. **Status**: Experiment. Value of the workspace variable. | | `variableType` **{warning-solid}** | [`WorkspaceVariableType`](#workspacevariabletype) | **Introduced** in GitLab 17.8. **Status**: Experiment. Type of the workspace variable. | diff --git a/ee/app/finders/remote_development/workspace_variables_finder.rb b/ee/app/finders/remote_development/workspace_variables_finder.rb deleted file mode 100644 index 20f72aad6cd863..00000000000000 --- a/ee/app/finders/remote_development/workspace_variables_finder.rb +++ /dev/null @@ -1,40 +0,0 @@ -# frozen_string_literal: true - -module RemoteDevelopment - class WorkspaceVariablesFinder - # Executes a query to find user provided workspace variables. - # - # @param [User, QA::Resource::User] current_user The user making the request. Must have permission - # to access workspaces. - # @param [Array] ids A list of specific WorkspaceVariable IDs to filter by (optional). - # @param [Array] project_ids A list of Project IDs to filter by (optional). - # @param [Array] workspace_ids A list of Workspace IDs to filter by (optional). - # @return [ActiveRecord::Relation] - # A collection of filtered RemoteDevelopment::WorkspaceVariable records ordered by ID descending. - def self.execute(current_user:, ids: [], project_ids: [], workspace_ids: []) - return WorkspaceVariable.none unless current_user.can?(:access_workspaces_feature) - - filter_arguments = { - ids: ids, - project_ids: project_ids, - workspace_ids: workspace_ids - } - - filter_argument_types = { - ids: Integer, - project_ids: Integer, - workspace_ids: Integer - }.freeze - - FilterArgumentValidator.validate_filter_argument_types!(filter_argument_types, filter_arguments) - FilterArgumentValidator.validate_at_least_one_filter_argument_provided!(**filter_arguments) - - collection_proxy = WorkspaceVariable.user_provided - collection_proxy = collection_proxy.id_in(ids) if ids.present? - collection_proxy = collection_proxy.by_project_ids(project_ids) if project_ids.present? - collection_proxy = collection_proxy.by_workspace_ids(workspace_ids) if workspace_ids.present? - - collection_proxy.order_id_desc - end - end -end diff --git a/ee/app/graphql/resolvers/remote_development/admin_workspaces_resolver.rb b/ee/app/graphql/resolvers/remote_development/admin_workspaces_resolver.rb index c9fa3e61d08997..36924e2860db12 100644 --- a/ee/app/graphql/resolvers/remote_development/admin_workspaces_resolver.rb +++ b/ee/app/graphql/resolvers/remote_development/admin_workspaces_resolver.rb @@ -2,34 +2,20 @@ module Resolvers module RemoteDevelopment - class AdminWorkspacesResolver < ::Resolvers::BaseResolver - include ResolvesIds - include LooksAhead - + class AdminWorkspacesResolver < WorkspacesBaseResolver # NOTE: We are intentionally not including Gitlab::Graphql::Authorize::AuthorizeResource, because this resolver # is currently only authorized at the instance admin level for all workspaces in the instance via the # `:read_all_workspaces` ability, so it's not necessary (or performant) to authorize individual workspaces. # Also, including Gitlab::Graphql::Authorize::AuthorizeResource would mix in a many methods related to # "resource" and "object" which are not applicable in this resolver, so we avoid including it and keep # the dependencies of this class more minimal. - # - extras [:lookahead] type Types::RemoteDevelopment::WorkspaceType.connection_type, null: true - argument :ids, [::Types::GlobalIDType[::RemoteDevelopment::Workspace]], - required: false, - description: - 'Filter workspaces by workspace GlobalIDs. For example, `["gid://gitlab/RemoteDevelopment::Workspace/1"]`.' - argument :user_ids, [::Types::GlobalIDType[Project]], required: false, description: 'Filter workspaces by user GlobalIDs.' - argument :project_ids, [::Types::GlobalIDType[Project]], - required: false, - description: 'Filter workspaces by project GlobalIDs.' - argument :agent_ids, [::Types::GlobalIDType[::Clusters::Agent]], required: false, description: 'Filter workspaces by agent GlobalIDs.' @@ -39,18 +25,7 @@ class AdminWorkspacesResolver < ::Resolvers::BaseResolver deprecated: { reason: 'Use actual_states instead', milestone: '16.7' }, description: 'Filter workspaces by actual states.' - argument :actual_states, [GraphQL::Types::String], - required: false, - description: 'Filter workspaces by actual states.' - def resolve_with_lookahead(**args) - # rubocop:disable Graphql/ResourceNotAvailableError -- We are intentionally not including Gitlab::Graphql::Authorize::AuthorizeResource - see note at top of class - unless License.feature_available?(:remote_development) - raise ::Gitlab::Graphql::Errors::ResourceNotAvailable, - "'remote_development' licensed feature is not available" - end - # rubocop:enable Graphql/ResourceNotAvailableError - return ::RemoteDevelopment::Workspace.none unless can_read_all_workspaces? begin @@ -69,12 +44,6 @@ def resolve_with_lookahead(**args) end end - def preloads - { - workspace_variables: [:user_provided_workspace_variables] - } - end - private def can_read_all_workspaces? diff --git a/ee/app/graphql/resolvers/remote_development/cluster_agent/workspaces_resolver.rb b/ee/app/graphql/resolvers/remote_development/cluster_agent/workspaces_resolver.rb index 68c04db7e4506d..f5556295e06a11 100644 --- a/ee/app/graphql/resolvers/remote_development/cluster_agent/workspaces_resolver.rb +++ b/ee/app/graphql/resolvers/remote_development/cluster_agent/workspaces_resolver.rb @@ -3,40 +3,24 @@ module Resolvers module RemoteDevelopment module ClusterAgent - class WorkspacesResolver < ::Resolvers::BaseResolver - include ResolvesIds + class WorkspacesResolver < WorkspacesBaseResolver include Gitlab::Graphql::Authorize::AuthorizeResource type Types::RemoteDevelopment::WorkspaceType.connection_type, null: true authorize :admin_cluster authorizes_object! - argument :ids, [::Types::GlobalIDType[::RemoteDevelopment::Workspace]], - required: false, - description: - 'Filter workspaces by workspace GlobalIDs. For example, `["gid://gitlab/RemoteDevelopment::Workspace/1"]`.' - - argument :project_ids, [::Types::GlobalIDType[Project]], - required: false, - description: 'Filter workspaces by project GlobalID.' - - argument :actual_states, [GraphQL::Types::String], - required: false, - description: 'Filter workspaces by actual states.' - alias_method :agent, :object - def resolve(**args) - unless License.feature_available?(:remote_development) - raise_resource_not_available_error! "'remote_development' licensed feature is not available" - end - - ::RemoteDevelopment::WorkspacesFinder.execute( - current_user: current_user, - agent_ids: [agent.id], - ids: resolve_ids(args[:ids]).map(&:to_i), - project_ids: resolve_ids(args[:project_ids]).map(&:to_i), - actual_states: args[:actual_states] || [] + def resolve_with_lookahead(**args) + apply_lookahead( + ::RemoteDevelopment::WorkspacesFinder.execute( + current_user: current_user, + agent_ids: [agent.id], + ids: resolve_ids(args[:ids]).map(&:to_i), + project_ids: resolve_ids(args[:project_ids]).map(&:to_i), + actual_states: args[:actual_states] || [] + ) ) end end diff --git a/ee/app/graphql/resolvers/remote_development/workspace_variables_resolver.rb b/ee/app/graphql/resolvers/remote_development/workspace_variables_resolver.rb deleted file mode 100644 index 2bc34695a17ccb..00000000000000 --- a/ee/app/graphql/resolvers/remote_development/workspace_variables_resolver.rb +++ /dev/null @@ -1,49 +0,0 @@ -# frozen_string_literal: true - -module Resolvers - module RemoteDevelopment - class WorkspaceVariablesResolver < ::Resolvers::BaseResolver - include LooksAhead - include ResolvesIds - include Gitlab::Graphql::Authorize::AuthorizeResource - - type Types::RemoteDevelopment::WorkspaceVariableType.connection_type, null: true - - argument :ids, [::Types::GlobalIDType[::RemoteDevelopment::WorkspaceVariable]], - required: false, - experiment: { milestone: '17.8' }, - description: - 'Filter workspace variable by workspace variable GlobalIDs.' \ - 'For example, `["gid://gitlab/RemoteDevelopment::WorkspaceVariable/1"]`.' - - argument :project_ids, [::Types::GlobalIDType[Project]], - required: false, - experiment: { milestone: '17.8' }, - description: 'Filter workspaces variables by project GlobalIDs.' - - argument :workspace_ids, [::Types::GlobalIDType[::RemoteDevelopment::Workspace]], - required: false, - experiment: { milestone: '17.8' }, - description: 'Filter workspaces by workspace GlobalIDs.' - - def resolve_with_lookahead(**args) - unless License.feature_available?(:remote_development) - raise_resource_not_available_error! "'remote_development' licensed feature is not available" - end - - ::RemoteDevelopment::WorkspaceVariablesFinder.execute( - current_user: current_user, - ids: resolve_ids(args[:ids]).map(&:to_i), - project_ids: resolve_ids(args[:project_ids]).map(&:to_i), - workspace_ids: resolve_ids(args[:workspace_ids]).map(&:to_i) - ) - end - - def preloads - { - workspace: [:workspace] - } - end - end - end -end diff --git a/ee/app/graphql/resolvers/remote_development/workspaces_base_resolver.rb b/ee/app/graphql/resolvers/remote_development/workspaces_base_resolver.rb new file mode 100644 index 00000000000000..355f3e69c44063 --- /dev/null +++ b/ee/app/graphql/resolvers/remote_development/workspaces_base_resolver.rb @@ -0,0 +1,44 @@ +# frozen_string_literal: true + +module Resolvers + module RemoteDevelopment + class WorkspacesBaseResolver < ::Resolvers::BaseResolver + include ResolvesIds + include LooksAhead + + extras [:lookahead] + + type Types::RemoteDevelopment::WorkspaceType.connection_type, null: true + + argument :ids, [::Types::GlobalIDType[::RemoteDevelopment::Workspace]], + required: false, + description: + 'Filter workspaces by workspace GlobalIDs. For example, `["gid://gitlab/RemoteDevelopment::Workspace/1"]`.' + + argument :project_ids, [::Types::GlobalIDType[Project]], + required: false, + description: 'Filter workspaces by project GlobalIDs.' + + argument :actual_states, [GraphQL::Types::String], + required: false, + description: 'Filter workspaces by actual states.' + + def ready?(**args) + # rubocop:disable Graphql/ResourceNotAvailableError -- Gitlab::Graphql::Authorize::AuthorizeResource is not included + unless License.feature_available?(:remote_development) + raise ::Gitlab::Graphql::Errors::ResourceNotAvailable, + "'remote_development' licensed feature is not available" + end + # rubocop:enable Graphql/ResourceNotAvailableError + + super + end + + def preloads + { + workspace_variables: [:user_provided_workspace_variables] + } + end + end + end +end diff --git a/ee/app/graphql/resolvers/remote_development/workspaces_resolver.rb b/ee/app/graphql/resolvers/remote_development/workspaces_resolver.rb index c8fd4f3d76efee..078f7c3979f974 100644 --- a/ee/app/graphql/resolvers/remote_development/workspaces_resolver.rb +++ b/ee/app/graphql/resolvers/remote_development/workspaces_resolver.rb @@ -2,21 +2,11 @@ module Resolvers module RemoteDevelopment - class WorkspacesResolver < ::Resolvers::BaseResolver - include ResolvesIds + class WorkspacesResolver < WorkspacesBaseResolver include Gitlab::Graphql::Authorize::AuthorizeResource type Types::RemoteDevelopment::WorkspaceType.connection_type, null: true - argument :ids, [::Types::GlobalIDType[::RemoteDevelopment::Workspace]], - required: false, - description: - 'Filter workspaces by workspace GlobalIDs. For example, `["gid://gitlab/RemoteDevelopment::Workspace/1"]`.' - - argument :project_ids, [::Types::GlobalIDType[Project]], - required: false, - description: 'Filter workspaces by project GlobalIDs.' - argument :agent_ids, [::Types::GlobalIDType[::Clusters::Agent]], required: false, description: 'Filter workspaces by agent GlobalIDs.' @@ -26,24 +16,18 @@ class WorkspacesResolver < ::Resolvers::BaseResolver deprecated: { reason: 'Use actual_states instead', milestone: '16.7' }, description: 'Filter workspaces by actual states.' - argument :actual_states, [GraphQL::Types::String], - required: false, - description: 'Filter workspaces by actual states.' - - def resolve(**args) - unless License.feature_available?(:remote_development) - raise_resource_not_available_error! "'remote_development' licensed feature is not available" - end - + def resolve_with_lookahead(**args) # noinspection RubyNilAnalysis - This is because the superclass #current_user uses #[], which can return nil # TODO: Change the superclass to use context.fetch(:current_user) instead of context[:current_user] - ::RemoteDevelopment::WorkspacesFinder.execute( - current_user: current_user, - user_ids: [current_user.id], - ids: resolve_ids(args[:ids]).map(&:to_i), - project_ids: resolve_ids(args[:project_ids]).map(&:to_i), - agent_ids: resolve_ids(args[:agent_ids]).map(&:to_i), - actual_states: args[:actual_states] || args[:include_actual_states] || [] + apply_lookahead( + ::RemoteDevelopment::WorkspacesFinder.execute( + current_user: current_user, + user_ids: [current_user.id], + ids: resolve_ids(args[:ids]).map(&:to_i), + project_ids: resolve_ids(args[:project_ids]).map(&:to_i), + agent_ids: resolve_ids(args[:agent_ids]).map(&:to_i), + actual_states: args[:actual_states] || args[:include_actual_states] || [] + ) ) end end diff --git a/ee/app/graphql/types/remote_development/workspace_type.rb b/ee/app/graphql/types/remote_development/workspace_type.rb index 742ff190e10567..b8f386ea19fbea 100644 --- a/ee/app/graphql/types/remote_development/workspace_type.rb +++ b/ee/app/graphql/types/remote_development/workspace_type.rb @@ -105,7 +105,7 @@ class WorkspaceType < ::Types::BaseObject ::Types::RemoteDevelopment::WorkspaceVariableType.connection_type, null: true, experiment: { milestone: '17.8' }, - description: 'User defined variables associated with the workspace.' + description: 'User defined variables associated with the workspace.', method: :user_provided_workspace_variables def project_id "gid://gitlab/Project/#{object.project_id}" @@ -114,10 +114,6 @@ def project_id def editor 'webide' end - - def workspace_variables - object.user_provided_workspace_variables if object.is_a?(::RemoteDevelopment::Workspace) - end end end end diff --git a/ee/app/graphql/types/remote_development/workspace_variable_type.rb b/ee/app/graphql/types/remote_development/workspace_variable_type.rb index b9fcdcaddb7e0f..11c0c1817ed7b4 100644 --- a/ee/app/graphql/types/remote_development/workspace_variable_type.rb +++ b/ee/app/graphql/types/remote_development/workspace_variable_type.rb @@ -6,16 +6,12 @@ class WorkspaceVariableType < ::Types::BaseObject graphql_name 'WorkspaceVariable' description 'Represents a remote development workspace variable' - authorize :read_workspace_variable + authorize :read_workspace field :id, ::Types::GlobalIDType[::RemoteDevelopment::WorkspaceVariable], experiment: { milestone: '17.8' }, null: false, description: 'Global ID of the workspace variable.' - field :project_id, GraphQL::Types::ID, - experiment: { milestone: '17.8' }, - null: false, description: 'ID of the project that contains the devfile for the workspace.' - field :key, GraphQL::Types::String, experiment: { milestone: '17.8' }, null: true, description: 'Name of the workspace variable.' @@ -39,6 +35,10 @@ class WorkspaceVariableType < ::Types::BaseObject def project_id "gid://gitlab/Project/#{object.project_id}" end + + def variable_type + ::RemoteDevelopment::Enums::Workspace::WORKSPACE_VARIABLE_TYPES_FOR_GRAPHQL.key(object.variable_type) + end end end end diff --git a/ee/app/models/remote_development/workspace.rb b/ee/app/models/remote_development/workspace.rb index df096ae2bf29f0..16ae00c721c691 100644 --- a/ee/app/models/remote_development/workspace.rb +++ b/ee/app/models/remote_development/workspace.rb @@ -24,7 +24,7 @@ class Workspace < ApplicationRecord has_one :remote_development_agent_config, through: :agent, source: :remote_development_agent_config has_many :workspace_variables, class_name: 'RemoteDevelopment::WorkspaceVariable', inverse_of: :workspace has_many :user_provided_workspace_variables, -> { - user_provided + user_provided.order_id_desc }, class_name: 'RemoteDevelopment::WorkspaceVariable', inverse_of: :workspace validates :user, presence: true diff --git a/ee/app/policies/remote_development/workspace_variable_policy.rb b/ee/app/policies/remote_development/workspace_variable_policy.rb new file mode 100644 index 00000000000000..3096600b19ffe0 --- /dev/null +++ b/ee/app/policies/remote_development/workspace_variable_policy.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +module RemoteDevelopment + class WorkspaceVariablePolicy < BasePolicy + alias_method :workspace_variable, :subject + + delegate { workspace_variable.workspace } + end +end diff --git a/ee/spec/finders/remote_development/workspace_variables_finder_spec.rb b/ee/spec/finders/remote_development/workspace_variables_finder_spec.rb deleted file mode 100644 index 6f982fd54e471a..00000000000000 --- a/ee/spec/finders/remote_development/workspace_variables_finder_spec.rb +++ /dev/null @@ -1,238 +0,0 @@ -# frozen_string_literal: true - -require "spec_helper" - -RSpec.describe RemoteDevelopment::WorkspaceVariablesFinder, feature_category: :workspaces do - include ::RemoteDevelopment::WorkspaceOperations::States - - let_it_be(:current_user) { create(:user) } - let_it_be(:other_user) { create(:user) } - - let_it_be(:cluster_admin_user) { create(:user) } - let_it_be(:agent_a) do - create(:ee_cluster_agent, :with_existing_workspaces_agent_config, created_by_user: cluster_admin_user) - end - - let_it_be(:agent_b) do - create(:ee_cluster_agent, :with_existing_workspaces_agent_config, created_by_user: cluster_admin_user) - end - - let_it_be(:project_a) { create(:project, :public) } - let_it_be(:project_b) { create(:project, :public) } - - let_it_be(:workspace_a) do - create(:workspace, user: current_user, updated_at: 2.days.ago, project: project_a, - actual_state: ::RemoteDevelopment::WorkspaceOperations::States::RUNNING, agent: agent_a - ) - end - - let_it_be(:other_users_workspace) do - create(:workspace, user: other_user, project: project_b, - actual_state: ::RemoteDevelopment::WorkspaceOperations::States::TERMINATED, agent: agent_b - ) - end - - let_it_be(:workspace_a_static_variable) do - create(:workspace_variable, - workspace_id: workspace_a.id, - key: 'GIT_CONFIG_COUNT', - value: 'static_var', - variable_type: 0, - user_provided: false - ) - end - - let_it_be(:workspace_a_user_variable_1) do - create(:workspace_variable, - workspace_id: workspace_a.id, - project_id: project_a.id, - key: 'USER_VAR_1', - value: 'user_var_1', - variable_type: 0, - user_provided: true - ) - end - - let_it_be(:workspace_a_user_variable_2) do - create(:workspace_variable, - workspace_id: workspace_a.id, - project_id: project_a.id, - key: 'USER_VAR_2', - value: 'user_var_2', - variable_type: 0, - user_provided: true - ) - end - - let_it_be(:other_user_workspace_variable) do - create(:workspace_variable, - workspace_id: other_users_workspace.id, - project_id: project_b.id, - key: 'OTHER_USER_VAR', - value: 'other_user_var', - variable_type: 0, - user_provided: true - ) - end - - subject(:collection_proxy) do - described_class.execute(current_user: current_user, **filter_arguments) - end - - before do - stub_licensed_features(remote_development: true) - allow(::RemoteDevelopment::FilterArgumentValidator).to receive_messages(validate_filter_argument_types!: true, - validate_at_least_one_filter_argument_provided!: true) - end - - describe "user provided workspace variables" do - context "when workspace has both static and user provided variables" do - let(:filter_arguments) { { workspace_ids: [workspace_a.id] } } - - it "returns only the user provided variables", :unlimited_max_formatted_output_length do - expect(collection_proxy).to contain_exactly(workspace_a_user_variable_1, workspace_a_user_variable_2) - expect(collection_proxy).not_to include(workspace_a_static_variable) - end - end - end - - describe "filter arguments" do - context "with ids argument" do - let(:filter_arguments) { { ids: [workspace_a_user_variable_1.id] } } - - it "returns only current user's workspace variables matching the specified IDs" do - expect(collection_proxy).to contain_exactly(workspace_a_user_variable_1) - expect(collection_proxy).not_to include(workspace_a_user_variable_2) - expect(collection_proxy).not_to include(other_user_workspace_variable) - end - end - - context "with project_ids argument" do - let(:filter_arguments) { { project_ids: [project_a.id] } } - - it "returns only workspace variables matching the specified project IDs" do - expect(collection_proxy).to contain_exactly(workspace_a_user_variable_1, workspace_a_user_variable_2) - end - end - - context "with workspace_ids argument" do - let(:filter_arguments) { { workspace_ids: [workspace_a.id] } } - - it "returns only workspace variables matching the specified agent IDs" do - expect(collection_proxy).to contain_exactly(workspace_a_user_variable_1, workspace_a_user_variable_2) - end - end - - context "with multiple arguments" do - let(:filter_arguments) do - { - ids: [workspace_a_user_variable_1.id, workspace_a_user_variable_2.id, other_user_workspace_variable.id, - workspace_a_static_variable.id], - project_ids: [project_a.id, project_b.id], - workspace_ids: [workspace_a.id, other_users_workspace.id] - } - end - - it "handles multiple arguments and still returns all workspaces which match all filter arguments", - :unlimited_max_formatted_output_length do - expect(collection_proxy).to contain_exactly(workspace_a_user_variable_1, workspace_a_user_variable_2, - other_user_workspace_variable) - end - end - - context "with extra empty filter arguments" do - let(:filter_arguments) do - { - ids: [workspace_a_user_variable_1.id], - project_ids: [], - workspace_ids: [] - } - end - - it "still uses existing filter arguments" do - expect(collection_proxy).to contain_exactly(workspace_a_user_variable_1) - end - end - end - - describe "ordering" do - context "with workspace_ids argument" do - let(:filter_arguments) { { workspace_ids: [workspace_a.id] } } - - it "returns workspace variables sorted by last updated time (most recent first)" do - expected_collection_proxy = [workspace_a_user_variable_2, workspace_a_user_variable_1] - expect(collection_proxy).to eq(expected_collection_proxy) - end - end - end - - describe "validations" do - context "when no filter arguments are provided" do - let(:filter_arguments) { {} } - - before do - allow(::RemoteDevelopment::FilterArgumentValidator).to receive( - :validate_at_least_one_filter_argument_provided! - ).and_raise(ArgumentError.new("At least one filter argument must be provided")) - end - - it "raises an ArgumentError" do - expect { collection_proxy }.to raise_error(ArgumentError, "At least one filter argument must be provided") - end - end - - context "when an invalid filter argument type is provided" do - let(:expected_exception_message) do - "'ids' must be an Array of 'Integer', " \ - "'project_ids' must be an Array of 'Integer', " \ - "'workspace_ids' must be an Array of 'Integer'" - end - - before do - allow(::RemoteDevelopment::FilterArgumentValidator).to receive( - :validate_filter_argument_types! - ).and_raise(RuntimeError.new(expected_exception_message)) - end - - context "when argument is not an array" do - let(:filter_arguments) do - { - ids: 1, - project_ids: 1, - workspace_ids: 1 - } - end - - it "raises an RuntimeError", :unlimited_max_formatted_output_length do - expect { collection_proxy.to_a }.to raise_error(RuntimeError, expected_exception_message) - end - end - - context "when array content is wrong type" do - let(:filter_arguments) do - { - ids: %w[a b], - project_ids: %w[a b], - workspace_ids: %w[a b] - } - end - - it "raises an RuntimeError", :unlimited_max_formatted_output_length do - expect { collection_proxy.to_a }.to raise_error(RuntimeError, expected_exception_message) - end - end - end - - context "when current_user does not have access_workspaces_feature ability (anonymous user)" do - let(:filter_arguments) { { ids: [workspace_a.id] } } - - before do - allow(current_user).to receive(:can?).with(:access_workspaces_feature).and_return(false) - end - - it "returns none" do - expect(collection_proxy).to be_blank - end - end - end -end diff --git a/ee/spec/graphql/types/query_type_spec.rb b/ee/spec/graphql/types/query_type_spec.rb index bc2faca200e723..c555d49af29a59 100644 --- a/ee/spec/graphql/types/query_type_spec.rb +++ b/ee/spec/graphql/types/query_type_spec.rb @@ -36,7 +36,6 @@ :vulnerability, :workspace, :workspaces, - :workspace_variables, :instance_external_audit_event_destinations, :instance_google_cloud_logging_configurations, :audit_events_instance_amazon_s3_configurations, diff --git a/ee/spec/graphql/types/remote_development/workspace_type_spec.rb b/ee/spec/graphql/types/remote_development/workspace_type_spec.rb index 973a9109514834..0ce217e3e87cf1 100644 --- a/ee/spec/graphql/types/remote_development/workspace_type_spec.rb +++ b/ee/spec/graphql/types/remote_development/workspace_type_spec.rb @@ -10,6 +10,7 @@ url editor devfile_ref devfile_path devfile_web_url devfile processed_devfile project_ref deployment_resource_version desired_config_generator_version workspaces_agent_config_version force_include_all_resources created_at updated_at + workspace_variables ] end diff --git a/ee/spec/graphql/types/remote_development/workspace_variable_type_spec.rb b/ee/spec/graphql/types/remote_development/workspace_variable_type_spec.rb index 8c503e98883dae..5d08923b18aef0 100644 --- a/ee/spec/graphql/types/remote_development/workspace_variable_type_spec.rb +++ b/ee/spec/graphql/types/remote_development/workspace_variable_type_spec.rb @@ -5,7 +5,7 @@ RSpec.describe GitlabSchema.types['WorkspaceVariable'], feature_category: :workspaces do let(:fields) do %i[ - id project_id workspace key value variable_type created_at updated_at + id key value variable_type created_at updated_at ] end @@ -13,5 +13,5 @@ specify { expect(described_class).to have_graphql_fields(fields) } - specify { expect(described_class).to require_graphql_authorizations(:read_workspace_variable) } + specify { expect(described_class).to require_graphql_authorizations(:read_workspace) } end diff --git a/ee/spec/policies/remote_development/workspace_variable_policy_spec.rb b/ee/spec/policies/remote_development/workspace_variable_policy_spec.rb new file mode 100644 index 00000000000000..b6270928834e00 --- /dev/null +++ b/ee/spec/policies/remote_development/workspace_variable_policy_spec.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe RemoteDevelopment::WorkspaceVariablePolicy, feature_category: :workspaces do + let_it_be(:user, refind: true) { create(:user) } + let_it_be(:agent_project, refind: true) { create(:project, creator: user) } + let_it_be(:agent, refind: true) do + create(:ee_cluster_agent, :with_existing_workspaces_agent_config, project: agent_project) + end + + let_it_be(:workspace_project, refind: true) { create(:project, creator: user) } + let_it_be(:workspace, refind: true) do + create(:workspace, project: workspace_project, agent: agent, user: user) + end + + let_it_be(:workspace_variable) { create(:workspace_variable, workspace: workspace) } + + before do + stub_licensed_features(remote_development: true) + end + + def permissions(user, workspace_variable) + described_class.new(user, workspace_variable) + end + + context 'when user can read a workspace' do + it 'allows reading the corrosponding workspace variable' do + expect(permissions(user, workspace_variable)).to be_allowed(:read_workspace) + end + end + + context 'when user can not read a workspace' do + it 'disallows reading the corrosponding workspace variable' do + expect(permissions(create(:user), workspace_variable)).to be_disallowed(:read_workspace) + end + end +end diff --git a/spec/migrations/20241227233633_backfill_user_provided_workspace_variables_spec.rb b/spec/migrations/20241227233633_backfill_user_provided_workspace_variables_spec.rb index d37cef71becfff..552fc7ae3ee30c 100644 --- a/spec/migrations/20241227233633_backfill_user_provided_workspace_variables_spec.rb +++ b/spec/migrations/20241227233633_backfill_user_provided_workspace_variables_spec.rb @@ -107,8 +107,11 @@ } migration.after -> { - user_provided_variable = workspace_variables.where.not(key: described_class::WORKSPACE_STATIC_VARIABLES).first + user_provided_variable = workspace_variables.where(variable_type: described_class::VARIABLE_ENV_TYPE) + .where.not(key: described_class::WORKSPACE_STATIC_VARIABLES).first + expect(workspace_static_file_variable.user_provided).to be(false) + expect(workspace_static_env_variable.user_provided).to be(false) expect(user_provided_variable.user_provided).to be(true) } end -- GitLab From ceda4e4ce770d51d76a6ca52c5bace1319c29d69 Mon Sep 17 00:00:00 2001 From: Daniyal Arshad Date: Thu, 9 Jan 2025 18:44:59 -0500 Subject: [PATCH 03/10] Add request specs and address review comments --- ...dd_user_provided_to_workspace_variables.rb | 2 +- ...kfill_user_provided_workspace_variables.rb | 6 +- .../workspaces_base_resolver.rb | 2 +- .../workspace_variable_type.rb | 4 - ee/app/models/remote_development/workspace.rb | 3 +- .../create/workspace_variables.rb | 4 +- .../api/graphql/remote_development/README.md | 6 + .../workspace_variables/shared.rb | 118 ++++++++++++++++++ .../workspace_variables/with_no_args_spec.rb | 22 ++++ .../remote_development/integration_spec.rb | 6 +- scripts/verify-tff-mapping | 3 + ..._user_provided_workspace_variables_spec.rb | 4 +- tests.yml | 4 + 13 files changed, 167 insertions(+), 17 deletions(-) create mode 100644 ee/spec/requests/api/graphql/remote_development/workspace_variables/shared.rb create mode 100644 ee/spec/requests/api/graphql/remote_development/workspace_variables/with_no_args_spec.rb diff --git a/db/migrate/20241227205133_add_user_provided_to_workspace_variables.rb b/db/migrate/20241227205133_add_user_provided_to_workspace_variables.rb index 962a1e95c616cf..5a9c99c86d6820 100644 --- a/db/migrate/20241227205133_add_user_provided_to_workspace_variables.rb +++ b/db/migrate/20241227205133_add_user_provided_to_workspace_variables.rb @@ -4,6 +4,6 @@ class AddUserProvidedToWorkspaceVariables < Gitlab::Database::Migration[2.2] milestone '17.8' def change - add_column :workspace_variables, :user_provided, :boolean, null: false, default: false + add_column :workspace_variables, :user_provided, :boolean, null: false, default: false, if_not_exists: true end end diff --git a/db/migrate/20241227233633_backfill_user_provided_workspace_variables.rb b/db/migrate/20241227233633_backfill_user_provided_workspace_variables.rb index c677ea9020a0a0..37efb7d767f596 100644 --- a/db/migrate/20241227233633_backfill_user_provided_workspace_variables.rb +++ b/db/migrate/20241227233633_backfill_user_provided_workspace_variables.rb @@ -16,9 +16,9 @@ class WorkspaceVariable < MigrationRecord # Since file type is not supported for user variables VARIABLE_ENV_TYPE = 0 - # Static workspace variables that get created on workspace creation + # Internal workspace variables that get created on workspace creation # Reference: /ee/lib/remote_development/workspace_operations/create/workspace_variables.rb - WORKSPACE_STATIC_VARIABLES = %w[ + WORKSPACE_INTERNAL_VARIABLES = %w[ GIT_CONFIG_COUNT GIT_CONFIG_KEY_0 GIT_CONFIG_VALUE_0 @@ -39,7 +39,7 @@ class WorkspaceVariable < MigrationRecord def up WorkspaceVariable.reset_column_information - WorkspaceVariable.where(variable_type: VARIABLE_ENV_TYPE).where.not(key: WORKSPACE_STATIC_VARIABLES) + WorkspaceVariable.where(variable_type: VARIABLE_ENV_TYPE).where.not(key: WORKSPACE_INTERNAL_VARIABLES) .each_batch(of: BATCH_SIZE) do |batch| batch.update_all(user_provided: true) end diff --git a/ee/app/graphql/resolvers/remote_development/workspaces_base_resolver.rb b/ee/app/graphql/resolvers/remote_development/workspaces_base_resolver.rb index 355f3e69c44063..8c93bb0bc72810 100644 --- a/ee/app/graphql/resolvers/remote_development/workspaces_base_resolver.rb +++ b/ee/app/graphql/resolvers/remote_development/workspaces_base_resolver.rb @@ -36,7 +36,7 @@ def ready?(**args) def preloads { - workspace_variables: [:user_provided_workspace_variables] + user_provided_workspace_variables: [:user_provided_workspace_variables] } end end diff --git a/ee/app/graphql/types/remote_development/workspace_variable_type.rb b/ee/app/graphql/types/remote_development/workspace_variable_type.rb index 11c0c1817ed7b4..174dd4ee1eb6d4 100644 --- a/ee/app/graphql/types/remote_development/workspace_variable_type.rb +++ b/ee/app/graphql/types/remote_development/workspace_variable_type.rb @@ -32,10 +32,6 @@ class WorkspaceVariableType < ::Types::BaseObject experiment: { milestone: '17.8' }, null: false, description: 'Timestamp of when the workspace variable was updated.' - def project_id - "gid://gitlab/Project/#{object.project_id}" - end - def variable_type ::RemoteDevelopment::Enums::Workspace::WORKSPACE_VARIABLE_TYPES_FOR_GRAPHQL.key(object.variable_type) end diff --git a/ee/app/models/remote_development/workspace.rb b/ee/app/models/remote_development/workspace.rb index 16ae00c721c691..5c87145e2032c8 100644 --- a/ee/app/models/remote_development/workspace.rb +++ b/ee/app/models/remote_development/workspace.rb @@ -23,8 +23,9 @@ class Workspace < ApplicationRecord # noinspection RailsParamDefResolve - https://handbook.gitlab.com/handbook/tools-and-tips/editors-and-ides/jetbrains-ides/tracked-jetbrains-issues/#ruby-31542 has_one :remote_development_agent_config, through: :agent, source: :remote_development_agent_config has_many :workspace_variables, class_name: 'RemoteDevelopment::WorkspaceVariable', inverse_of: :workspace + # Currently we only support :environement type for user provided variables has_many :user_provided_workspace_variables, -> { - user_provided.order_id_desc + user_provided.with_variable_type_environment.order_id_desc }, class_name: 'RemoteDevelopment::WorkspaceVariable', inverse_of: :workspace validates :user, presence: true diff --git a/ee/lib/remote_development/workspace_operations/create/workspace_variables.rb b/ee/lib/remote_development/workspace_operations/create/workspace_variables.rb index 495d88e89755b3..4d65e083a73185 100644 --- a/ee/lib/remote_development/workspace_operations/create/workspace_variables.rb +++ b/ee/lib/remote_development/workspace_operations/create/workspace_variables.rb @@ -44,7 +44,7 @@ def self.variables( resource_url_template: String => vscode_extensions_gallery_resource_url_template, } - static_variables = [ + internal_variables = [ { key: File.basename(RemoteDevelopment::WorkspaceOperations::FileMounts::GITLAB_TOKEN_FILE), value: personal_access_token_value, @@ -160,7 +160,7 @@ def self.variables( } end - static_variables + user_provided_variables + internal_variables + user_provided_variables end end end diff --git a/ee/spec/requests/api/graphql/remote_development/README.md b/ee/spec/requests/api/graphql/remote_development/README.md index b7e0d69d659c31..4db343d3eb5db2 100644 --- a/ee/spec/requests/api/graphql/remote_development/README.md +++ b/ee/spec/requests/api/graphql/remote_development/README.md @@ -51,6 +51,12 @@ Here are the related spec folders for the fields (in alphabetical order by resol - Resolver source file for `tests.yml` and `verify-tff-mapping`: `ee/app/graphql/resolvers/remote_development/namespace/workspaces_resolver.rb` - Notes: This is the same resolver used by `Query.currentUser.workspaces` +- GraphQL Field: `Query.workspace.workspaceVariables` + - Spec folder: `ee/spec/requests/api/graphql/remote_development/workspace_variables` + - API docs: https://docs.gitlab.com/ee/api/graphql/reference/index.html##workspacevariableconnection + - Resolver source file for `tests.yml` and `verify-tff-mapping`: `ee/app/graphql/resolvers/remote_development/namespace/workspaces_resolver.rb` + - Notes: This is the same resolver used by `Query.currentUser.workspaces` + - GraphQL Field: `Query.currentUser.workspaces` - Spec folder: `ee/spec/requests/api/graphql/remote_development/current_user/workspaces` - API docs: https://docs.gitlab.com/ee/api/graphql/reference/index.html#currentuserworkspaces diff --git a/ee/spec/requests/api/graphql/remote_development/workspace_variables/shared.rb b/ee/spec/requests/api/graphql/remote_development/workspace_variables/shared.rb new file mode 100644 index 00000000000000..69752cabcbfa0e --- /dev/null +++ b/ee/spec/requests/api/graphql/remote_development/workspace_variables/shared.rb @@ -0,0 +1,118 @@ +# frozen_string_literal: true + +require_relative '../shared' + +RSpec.shared_context 'with multiple workspace_variables created' do + include_context 'with unauthorized workspace created' + + let_it_be(:workspace, reload: true) { create(:workspace, name: 'matching-workspace') } + + let(:id) { workspace.to_global_id.to_s } + let(:args) { { id: id } } + + let_it_be(:workspace_a_internal_variable) do + create(:workspace_variable, + workspace_id: workspace.id, + key: 'GIT_CONFIG_COUNT', + value: 'internal_var', + variable_type: 0, + user_provided: false + ) + end + + let_it_be(:workspace_user_env_variable) do + create(:workspace_variable, + workspace_id: workspace.id, + key: 'GIT_CONFIG_KEY_1', + value: 'user_var_1', + variable_type: 0, + user_provided: true + ) + end + + let_it_be(:workspace_user_file_variable) do + create(:workspace_variable, + workspace_id: workspace.id, + key: 'CONFIG_FILE', + value: 'user_var_1', + variable_type: 1, + user_provided: true + ) + end + + let_it_be(:unauthorized_workspace_variable) do + create(:workspace_variable, + workspace_id: unauthorized_workspace.id, + key: 'OTHER_VAR', + value: 'other_var', + variable_type: 0, + user_provided: true + ) + end +end + +RSpec.shared_context 'for a Query.workspace.workspaceVariables query' do + include GraphqlHelpers + + include_context 'with multiple workspace_variables created' + include_context "with authorized user as developer on workspace's project" + + let(:query) do + fields = all_graphql_fields_for('WorkspaceVariable') + + graphql_query_for( + :workspace, + args, + query_nodes(:workspace_variables, fields) + ) + end + + subject(:actual_workspace_variables) { graphql_dig_at(graphql_data, :workspace, :workspace_variables, :nodes) } +end + +RSpec.shared_examples 'query returns an array containing all non-internal variables associated with a workspace' do + before do + post_graphql(query, current_user: current_user) + end + + it 'includes only the expected workspace_variables', :unlimited_max_formatted_output_length do + expect(workspace_variable_keys).not_to include(unauthorized_workspace_variable.key) + expect(workspace_variable_keys).not_to include(workspace_user_file_variable.key) + expect(workspace_variable_keys).not_to include(workspace_a_internal_variable.key) + + expect(workspace_variable_keys).to include(workspace_user_env_variable.key) + expect(workspace_variable_values).to include(workspace_user_env_variable.value) + end +end + +RSpec.shared_examples 'multiple workspace_variables query' do |authorized_user_is_admin: false| + include_context 'in licensed environment' + + let(:workspace_variable_keys) { subject.pluck("key") } + let(:workspace_variable_values) { subject.pluck("value") } + + context 'when user is authorized' do + include_context 'with authorized user as current user' + + it_behaves_like 'query is a working graphql query' + it_behaves_like 'query returns an array containing all non-internal variables associated with a workspace' + + unless authorized_user_is_admin + context 'when the user requests a workspace that they are not authorized for' do + let(:id) { global_id_of(unauthorized_workspace) } + + it_behaves_like 'query is a working graphql query' + it_behaves_like 'query returns blank' + end + end + end + + context 'when user is not authorized' do + include_context 'with unauthorized user as current user' + + it_behaves_like 'query is a working graphql query' + it_behaves_like 'query returns blank' + end + + it_behaves_like 'query in unlicensed environment' +end diff --git a/ee/spec/requests/api/graphql/remote_development/workspace_variables/with_no_args_spec.rb b/ee/spec/requests/api/graphql/remote_development/workspace_variables/with_no_args_spec.rb new file mode 100644 index 00000000000000..36565bb7702bb8 --- /dev/null +++ b/ee/spec/requests/api/graphql/remote_development/workspace_variables/with_no_args_spec.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +require 'spec_helper' +require_relative './shared' + +RSpec.describe 'Query.workspace.workspaceVariables (with no arguments)', feature_category: :workspaces do + include_context 'for a Query.workspace.workspaceVariables query' + + context 'with non-admin user' do + let_it_be(:authorized_user) { workspace.user } + let_it_be(:unauthorized_user) { create(:user) } + + it_behaves_like 'multiple workspace_variables query' + end + + context 'with admin user' do + let_it_be(:authorized_user) { create(:admin) } + let_it_be(:unauthorized_user) { create(:user) } + + it_behaves_like 'multiple workspace_variables query', authorized_user_is_admin: true + end +end diff --git a/ee/spec/requests/remote_development/integration_spec.rb b/ee/spec/requests/remote_development/integration_spec.rb index 15e8f0e201d833..401c72b7c652a0 100644 --- a/ee/spec/requests/remote_development/integration_spec.rb +++ b/ee/spec/requests/remote_development/integration_spec.rb @@ -87,7 +87,7 @@ ] end - let(:expected_static_variables) do + let(:expected_internal_variables) do # rubocop:disable Layout/LineLength -- keep them on one line for easier readability and editability [ { key: "GIT_CONFIG_COUNT", type: :environment, value: "3" }, @@ -237,7 +237,7 @@ def do_create_workspace expect(actual_processed_devfile.fetch(:components)).to eq(expected_processed_devfile.fetch(:components)) expect(actual_processed_devfile).to eq(expected_processed_devfile) - all_expected_vars = (expected_static_variables + user_provided_variables).sort_by { |v| v[:key] } + all_expected_vars = (expected_internal_variables + user_provided_variables).sort_by { |v| v[:key] } # NOTE: We convert the actual records into hashes and sort them as a hash rather than ordering in # ActiveRecord, to account for platform- or db-specific sorting differences. types = RemoteDevelopment::Enums::Workspace::WORKSPACE_VARIABLE_TYPES @@ -260,7 +260,7 @@ def do_create_workspace actual_without_regexes = all_actual_vars.reject { |v| v[:key] == "gl_token" } expect(expected_without_regexes).to match(actual_without_regexes) - expected_gl_token_value = expected_static_variables.find { |var| var[:key] == "gl_token" }[:value] + expected_gl_token_value = expected_internal_variables.find { |var| var[:key] == "gl_token" }[:value] actual_gl_token_value = all_actual_vars.find { |var| var[:key] == "gl_token" }[:value] expect(actual_gl_token_value).to match(expected_gl_token_value) diff --git a/scripts/verify-tff-mapping b/scripts/verify-tff-mapping index 0bd94ee6f6f642..46ee017bc4d5dd 100755 --- a/scripts/verify-tff-mapping +++ b/scripts/verify-tff-mapping @@ -482,6 +482,7 @@ tests = [ ee/spec/requests/api/graphql/remote_development/workspaces/with_ids_arg_spec.rb ee/spec/requests/api/graphql/remote_development/workspaces/with_no_args_spec.rb ee/spec/requests/api/graphql/remote_development/workspaces/with_project_ids_arg_spec.rb + ee/spec/requests/api/graphql/remote_development/workspace_variables/with_no_args_spec.rb ] }, { @@ -502,6 +503,7 @@ tests = [ ee/spec/requests/api/graphql/remote_development/cluster_agent/workspaces/with_ids_arg_spec.rb ee/spec/requests/api/graphql/remote_development/cluster_agent/workspaces/with_no_args_spec.rb ee/spec/requests/api/graphql/remote_development/cluster_agent/workspaces/with_project_ids_arg_spec.rb + ee/spec/requests/api/graphql/remote_development/workspace_variables/with_no_args_spec.rb ] }, { @@ -536,6 +538,7 @@ tests = [ ee/spec/requests/api/graphql/remote_development/current_user/workspaces/with_ids_arg_spec.rb ee/spec/requests/api/graphql/remote_development/current_user/workspaces/with_no_args_spec.rb ee/spec/requests/api/graphql/remote_development/current_user/workspaces/with_project_ids_arg_spec.rb + ee/spec/requests/api/graphql/remote_development/workspace_variables/with_no_args_spec.rb ] }, diff --git a/spec/migrations/20241227233633_backfill_user_provided_workspace_variables_spec.rb b/spec/migrations/20241227233633_backfill_user_provided_workspace_variables_spec.rb index 552fc7ae3ee30c..a8e13cd1d13309 100644 --- a/spec/migrations/20241227233633_backfill_user_provided_workspace_variables_spec.rb +++ b/spec/migrations/20241227233633_backfill_user_provided_workspace_variables_spec.rb @@ -100,7 +100,7 @@ ) end - it 'sets user_provided to true for all non-static environment variables in the table' do + it 'sets user_provided to true for all non-internal environment variables in the table' do reversible_migration do |migration| migration.before -> { expect(workspace_variables.pluck(:user_provided)).to all(be false) @@ -108,7 +108,7 @@ migration.after -> { user_provided_variable = workspace_variables.where(variable_type: described_class::VARIABLE_ENV_TYPE) - .where.not(key: described_class::WORKSPACE_STATIC_VARIABLES).first + .where.not(key: described_class::WORKSPACE_INTERNAL_VARIABLES).first expect(workspace_static_file_variable.user_provided).to be(false) expect(workspace_static_env_variable.user_provided).to be(false) diff --git a/tests.yml b/tests.yml index 6c4a2fa01686e3..6752157134166c 100644 --- a/tests.yml +++ b/tests.yml @@ -128,6 +128,7 @@ mapping: test: - 'ee/spec/requests/api/graphql/remote_development/workspace/*_spec.rb' - 'ee/spec/requests/api/graphql/remote_development/workspaces/*_spec.rb' + - 'ee/spec/requests/api/graphql/remote_development/workspace_variables/*_spec.rb' - source: 'ee/app/graphql/resolvers/remote_development/cluster_agent/remote_development_agent_config_resolver\.rb' test: @@ -140,10 +141,12 @@ mapping: - source: 'ee/app/graphql/resolvers/remote_development/cluster_agent/workspaces_resolver\.rb' test: - 'ee/spec/requests/api/graphql/remote_development/cluster_agent/workspaces/*_spec.rb' + - 'ee/spec/requests/api/graphql/remote_development/workspace_variables/*_spec.rb' - source: 'ee/app/graphql/resolvers/remote_development/workspaces_resolver\.rb' test: - 'ee/spec/requests/api/graphql/remote_development/current_user/workspaces/*_spec.rb' + - 'ee/spec/requests/api/graphql/remote_development/workspace_variables/*_spec.rb' - source: 'ee/app/graphql/resolvers/remote_development/namespace/cluster_agents_resolver\.rb' test: @@ -154,6 +157,7 @@ mapping: test: - 'ee/spec/requests/api/graphql/remote_development/current_user/workspaces/*_spec.rb' - 'ee/spec/requests/api/graphql/remote_development/workspace/*_spec.rb' + - 'ee/spec/requests/api/graphql/remote_development/workspace_variables/*_spec.rb' ## END Remote development GraphQL resolvers -- GitLab From 54a4d05bade73d2ef612fbe8c151da28a5ad273e Mon Sep 17 00:00:00 2001 From: Daniyal Arshad Date: Fri, 10 Jan 2025 00:38:18 -0500 Subject: [PATCH 04/10] Address review comments by Chad --- doc/api/graphql/reference/index.md | 15 ++--- ee/app/graphql/ee/types/query_type.rb | 2 +- ...solver.rb => workspaces_admin_resolver.rb} | 2 +- .../workspace_variable_input.rb | 7 ++- .../workspace_variable_type.rb | 8 +-- .../api/graphql/remote_development/README.md | 4 +- .../workspace_variables/shared.rb | 10 ++++ scripts/verify-tff-mapping | 60 ++++++++++++++----- tests.yml | 23 ++++++- 9 files changed, 98 insertions(+), 33 deletions(-) rename ee/app/graphql/resolvers/remote_development/{admin_workspaces_resolver.rb => workspaces_admin_resolver.rb} (97%) diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index e7e7db43f6ba99..4bee910c651255 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -38510,12 +38510,12 @@ Represents a remote development workspace variable. | Name | Type | Description | | ---- | ---- | ----------- | -| `createdAt` **{warning-solid}** | [`Time!`](#time) | **Introduced** in GitLab 17.8. **Status**: Experiment. Timestamp of when the workspace variable was created. | -| `id` **{warning-solid}** | [`RemoteDevelopmentWorkspaceVariableID!`](#remotedevelopmentworkspacevariableid) | **Introduced** in GitLab 17.8. **Status**: Experiment. Global ID of the workspace variable. | -| `key` **{warning-solid}** | [`String`](#string) | **Introduced** in GitLab 17.8. **Status**: Experiment. Name of the workspace variable. | -| `updatedAt` **{warning-solid}** | [`Time!`](#time) | **Introduced** in GitLab 17.8. **Status**: Experiment. Timestamp of when the workspace variable was updated. | -| `value` **{warning-solid}** | [`String`](#string) | **Introduced** in GitLab 17.8. **Status**: Experiment. Value of the workspace variable. | -| `variableType` **{warning-solid}** | [`WorkspaceVariableType`](#workspacevariabletype) | **Introduced** in GitLab 17.8. **Status**: Experiment. Type of the workspace variable. | +| `createdAt` | [`Time!`](#time) | Timestamp of when the workspace variable was created. | +| `id` | [`RemoteDevelopmentWorkspaceVariableID!`](#remotedevelopmentworkspacevariableid) | Global ID of the workspace variable. | +| `key` | [`String`](#string) | Key of the workspace variable. | +| `updatedAt` | [`Time!`](#time) | Timestamp of when the workspace variable was updated. | +| `value` | [`String`](#string) | Value of the workspace variable. | +| `variableType` | [`WorkspaceVariableType`](#workspacevariabletype) | Type of the workspace variable. | ### `WorkspacesAgentConfig` @@ -45688,5 +45688,6 @@ Attributes for defining a variable to be injected in a workspace. | Name | Type | Description | | ---- | ---- | ----------- | | `key` | [`String!`](#string) | Key of the variable. | -| `type` | [`WorkspaceVariableInputType!`](#workspacevariableinputtype) | Type of the variable to be injected in a workspace. | +| `type` **{warning-solid}** | [`WorkspaceVariableInputType`](#workspacevariableinputtype) | **Deprecated:** Use `variableType` instead. Deprecated in GitLab 17.8. | | `value` | [`String!`](#string) | Value of the variable. | +| `variableType` | [`WorkspaceVariableType`](#workspacevariabletype) | Type of the variable to be injected in a workspace. | diff --git a/ee/app/graphql/ee/types/query_type.rb b/ee/app/graphql/ee/types/query_type.rb index feb21cf019d43c..90669245b3e1ef 100644 --- a/ee/app/graphql/ee/types/query_type.rb +++ b/ee/app/graphql/ee/types/query_type.rb @@ -98,7 +98,7 @@ module QueryType field :workspaces, ::Types::RemoteDevelopment::WorkspaceType.connection_type, null: true, - resolver: ::Resolvers::RemoteDevelopment::AdminWorkspacesResolver, + resolver: ::Resolvers::RemoteDevelopment::WorkspacesAdminResolver, description: 'Find workspaces across the entire instance. This field is only available to instance admins, ' \ 'it will return an empty result for all non-admins.' field :instance_external_audit_event_destinations, diff --git a/ee/app/graphql/resolvers/remote_development/admin_workspaces_resolver.rb b/ee/app/graphql/resolvers/remote_development/workspaces_admin_resolver.rb similarity index 97% rename from ee/app/graphql/resolvers/remote_development/admin_workspaces_resolver.rb rename to ee/app/graphql/resolvers/remote_development/workspaces_admin_resolver.rb index 36924e2860db12..3713040c94e040 100644 --- a/ee/app/graphql/resolvers/remote_development/admin_workspaces_resolver.rb +++ b/ee/app/graphql/resolvers/remote_development/workspaces_admin_resolver.rb @@ -2,7 +2,7 @@ module Resolvers module RemoteDevelopment - class AdminWorkspacesResolver < WorkspacesBaseResolver + class WorkspacesAdminResolver < WorkspacesBaseResolver # NOTE: We are intentionally not including Gitlab::Graphql::Authorize::AuthorizeResource, because this resolver # is currently only authorized at the instance admin level for all workspaces in the instance via the # `:read_all_workspaces` ability, so it's not necessary (or performant) to authorize individual workspaces. diff --git a/ee/app/graphql/types/remote_development/workspace_variable_input.rb b/ee/app/graphql/types/remote_development/workspace_variable_input.rb index 349bceaaac6060..25daa0d1a78c8f 100644 --- a/ee/app/graphql/types/remote_development/workspace_variable_input.rb +++ b/ee/app/graphql/types/remote_development/workspace_variable_input.rb @@ -15,8 +15,13 @@ class WorkspaceVariableInput < BaseInputObject format: { with: /\A[a-zA-Z0-9\-_.]+\z/, message: 'must contain only alphanumeric characters, -, _ or .' } } argument :type, Types::RemoteDevelopment::WorkspaceVariableInputTypeEnum, - description: 'Type of the variable to be injected in a workspace.' + required: false, + description: 'Type of the variable to be injected in a workspace.', + deprecated: { reason: 'Use `variableType` instead', milestone: '17.8' } argument :value, GraphQL::Types::String, description: 'Value of the variable.' + argument :variable_type, Types::RemoteDevelopment::WorkspaceVariableTypeEnum, + required: false, + description: 'Type of the variable to be injected in a workspace.' end end end diff --git a/ee/app/graphql/types/remote_development/workspace_variable_type.rb b/ee/app/graphql/types/remote_development/workspace_variable_type.rb index 174dd4ee1eb6d4..5f391020784860 100644 --- a/ee/app/graphql/types/remote_development/workspace_variable_type.rb +++ b/ee/app/graphql/types/remote_development/workspace_variable_type.rb @@ -9,27 +9,21 @@ class WorkspaceVariableType < ::Types::BaseObject authorize :read_workspace field :id, ::Types::GlobalIDType[::RemoteDevelopment::WorkspaceVariable], - experiment: { milestone: '17.8' }, null: false, description: 'Global ID of the workspace variable.' field :key, GraphQL::Types::String, - experiment: { milestone: '17.8' }, - null: true, description: 'Name of the workspace variable.' + null: true, description: 'Key of the workspace variable.' field :value, GraphQL::Types::String, - experiment: { milestone: '17.8' }, null: true, description: 'Value of the workspace variable.' field :variable_type, Types::RemoteDevelopment::WorkspaceVariableTypeEnum, - experiment: { milestone: '17.8' }, null: true, description: 'Type of the workspace variable.' field :created_at, Types::TimeType, - experiment: { milestone: '17.8' }, null: false, description: 'Timestamp of when the workspace variable was created.' field :updated_at, Types::TimeType, - experiment: { milestone: '17.8' }, null: false, description: 'Timestamp of when the workspace variable was updated.' def variable_type diff --git a/ee/spec/requests/api/graphql/remote_development/README.md b/ee/spec/requests/api/graphql/remote_development/README.md index 4db343d3eb5db2..9fbc5db7f6bee0 100644 --- a/ee/spec/requests/api/graphql/remote_development/README.md +++ b/ee/spec/requests/api/graphql/remote_development/README.md @@ -14,7 +14,7 @@ Here are the related spec folders for the fields (in alphabetical order by resol - GraphQL Field: `Query.workspaces` - Spec folder: `ee/spec/requests/api/graphql/remote_development/workspaces` - API docs: https://docs.gitlab.com/ee/api/graphql/reference/index.html#queryworkspaces - - Resolver source file for `tests.yml` and `verify-tff-mapping`: `ee/app/graphql/resolvers/remote_development/admin_workspaces_resolver.rb` + - Resolver source file for `tests.yml` and `verify-tff-mapping`: `ee/app/graphql/resolvers/remote_development/workspaces_admin_resolver.rb` - Notes: Only admins may use this field. - GraphQL Field: `Query.project.clusterAgent.remoteDevelopmentAgentConfig` @@ -81,6 +81,8 @@ Without this approach, achieving equivalent coverage across all of this same Gra If you add new spec files, you should update `tests.yml` and `scripts/verify-tff-mapping` accordingly. +Add entries for relevant types and resolvers in these files. + ## Why aren't all individual fields of graphql types tested in these specs? None of these other graphql API request specs test the actual fields because that’s not necessary. diff --git a/ee/spec/requests/api/graphql/remote_development/workspace_variables/shared.rb b/ee/spec/requests/api/graphql/remote_development/workspace_variables/shared.rb index 69752cabcbfa0e..0c8b04b69346d9 100644 --- a/ee/spec/requests/api/graphql/remote_development/workspace_variables/shared.rb +++ b/ee/spec/requests/api/graphql/remote_development/workspace_variables/shared.rb @@ -83,6 +83,15 @@ expect(workspace_variable_keys).to include(workspace_user_env_variable.key) expect(workspace_variable_values).to include(workspace_user_env_variable.value) end + + it 'includes only the correct type of workspace_variables' do + expect(workspace_variable_types).to include( + ::RemoteDevelopment::Enums::Workspace::WORKSPACE_VARIABLE_TYPES.key(0).to_s.upcase + ) + expect(workspace_variable_types).not_to include( + ::RemoteDevelopment::Enums::Workspace::WORKSPACE_VARIABLE_TYPES.key(1).to_s.upcase + ) + end end RSpec.shared_examples 'multiple workspace_variables query' do |authorized_user_is_admin: false| @@ -90,6 +99,7 @@ let(:workspace_variable_keys) { subject.pluck("key") } let(:workspace_variable_values) { subject.pluck("value") } + let(:workspace_variable_types) { subject.pluck("variableType") } context 'when user is authorized' do include_context 'with authorized user as current user' diff --git a/scripts/verify-tff-mapping b/scripts/verify-tff-mapping index 46ee017bc4d5dd..689f9ab1687621 100755 --- a/scripts/verify-tff-mapping +++ b/scripts/verify-tff-mapping @@ -472,19 +472,6 @@ tests = [ ## END Remote development GraphQL mutations ## BEGIN Remote development GraphQL resolvers (in alphabetical order by resolver source file path) - { - explanation: 'Map Remote Development GraphQL query root admin_workspaces_resolver.rb to request specs', - changed_file: 'ee/app/graphql/resolvers/remote_development/admin_workspaces_resolver.rb', - expected: %w[ - ee/spec/requests/api/graphql/remote_development/workspace/with_id_arg_spec.rb - ee/spec/requests/api/graphql/remote_development/workspaces/with_actual_states_arg_spec.rb - ee/spec/requests/api/graphql/remote_development/workspaces/with_agent_ids_arg_spec.rb - ee/spec/requests/api/graphql/remote_development/workspaces/with_ids_arg_spec.rb - ee/spec/requests/api/graphql/remote_development/workspaces/with_no_args_spec.rb - ee/spec/requests/api/graphql/remote_development/workspaces/with_project_ids_arg_spec.rb - ee/spec/requests/api/graphql/remote_development/workspace_variables/with_no_args_spec.rb - ] - }, { explanation: 'Map Remote Development GraphQL cluster_agent/remote_development_agent_config_resolver.rb to request specs', @@ -528,6 +515,19 @@ tests = [ ] # rubocop:enable Layout/LineLength }, + { + explanation: 'Map Remote Development GraphQL query root workspaces_admin_resolver.rb to request specs', + changed_file: 'ee/app/graphql/resolvers/remote_development/workspaces_admin_resolver.rb', + expected: %w[ + ee/spec/requests/api/graphql/remote_development/workspace/with_id_arg_spec.rb + ee/spec/requests/api/graphql/remote_development/workspaces/with_actual_states_arg_spec.rb + ee/spec/requests/api/graphql/remote_development/workspaces/with_agent_ids_arg_spec.rb + ee/spec/requests/api/graphql/remote_development/workspaces/with_ids_arg_spec.rb + ee/spec/requests/api/graphql/remote_development/workspaces/with_no_args_spec.rb + ee/spec/requests/api/graphql/remote_development/workspaces/with_project_ids_arg_spec.rb + ee/spec/requests/api/graphql/remote_development/workspace_variables/with_no_args_spec.rb + ] + }, { explanation: 'Map Remote Development GraphQL workspaces_resolver.rb to request specs', changed_file: 'ee/app/graphql/resolvers/remote_development/workspaces_resolver.rb', @@ -541,7 +541,9 @@ tests = [ ee/spec/requests/api/graphql/remote_development/workspace_variables/with_no_args_spec.rb ] }, + ## END Remote development GraphQL resolvers + ## BEGIN Remote development GraphQL types { explanation: 'Map Remote Development GraphQL query root workspace type resolver to request specs', changed_file: 'ee/app/graphql/types/remote_development/workspace_type.rb', @@ -550,7 +552,37 @@ tests = [ ee/spec/requests/api/graphql/remote_development/workspace/with_id_arg_spec.rb ] }, - ## END Remote development GraphQL resolvers + { + explanation: 'Map Remote Development GraphQL workspace_variable_type.rb to request specs', + changed_file: 'ee/app/graphql/types/remote_development/workspace_variable_type.rb', + expected: %w[ + ee/spec/graphql/types/remote_development/workspace_variable_type_spec.rb + ee/spec/requests/api/graphql/remote_development/workspace_variables/with_no_args_spec.rb + ] + }, + { + explanation: 'Map Remote Development GraphQL workspace_variable_input_type_enum.rb to request specs', + changed_file: 'ee/app/graphql/types/remote_development/workspace_variable_input.rb', + expected: %w[ + ee/spec/graphql/types/remote_development/workspace_variable_input_spec.rb + ee/spec/requests/api/graphql/mutations/remote_development/workspace_operations/create_spec.rb + ] + }, + { + explanation: 'Map Remote Development GraphQL workspace_variable_type_enum.rb to request specs', + changed_file: 'ee/app/graphql/types/remote_development/workspace_variable_type_enum.rb', + expected: %w[ + ee/spec/requests/api/graphql/remote_development/workspace_variables/with_no_args_spec.rb + ] + }, + { + explanation: 'Map Remote Development GraphQL workspace_variable_input_type_enum.rb to request specs', + changed_file: 'ee/app/graphql/types/remote_development/workspace_variable_input_type_enum.rb', + expected: %w[ + ee/spec/requests/api/graphql/mutations/remote_development/workspace_operations/create_spec.rb + ] + }, + ## END Remote development GraphQL types { explanation: 'https://gitlab.com/gitlab-org/gitlab/-/issues/466068#note_1987834618', diff --git a/tests.yml b/tests.yml index 6752157134166c..0c9b2463dc0c7a 100644 --- a/tests.yml +++ b/tests.yml @@ -124,7 +124,7 @@ mapping: test: 'spec/mailers/previews_spec.rb' ## BEGIN Remote development GraphQL resolvers (in alphabetical order by resolver source file path) - - source: 'ee/app/graphql/resolvers/remote_development/admin_workspaces_resolver\.rb' + - source: 'ee/app/graphql/resolvers/remote_development/workspaces_admin_resolver\.rb' test: - 'ee/spec/requests/api/graphql/remote_development/workspace/*_spec.rb' - 'ee/spec/requests/api/graphql/remote_development/workspaces/*_spec.rb' @@ -161,10 +161,31 @@ mapping: ## END Remote development GraphQL resolvers + ## BEGIN Remote development GraphQL types + - source: 'ee/app/graphql/types/remote_development/workspace_type\.rb' test: - 'ee/spec/requests/api/graphql/remote_development/workspace/with_id_arg_spec.rb' + - source: 'ee/app/graphql/types/remote_development/workspace_variable_input\.rb' + test: + - 'ee/spec/graphql/types/remote_development/workspace_variable_input_spec.rb' + - 'ee/spec/requests/api/graphql/mutations/remote_development/workspace_operations/create_spec.rb' + + - source: 'ee/app/graphql/types/remote_development/workspace_variable_type\.rb' + test: + - 'ee/spec/requests/api/graphql/remote_development/workspace_variables/with_no_args_spec.rb' + + - source: 'ee/app/graphql/types/remote_development/workspace_variable_type_enum\.rb' + test: + - 'ee/spec/requests/api/graphql/remote_development/workspace_variables/with_no_args_spec.rb' + + - source: 'ee/app/graphql/types/remote_development/workspace_variable_input_type_enum\.rb' + test: + - 'ee/spec/requests/api/graphql/mutations/remote_development/workspace_operations/create_spec.rb' + + ## END Remote development GraphQL types + # Usage metric schema changes should trigger validations for all metrics and tooling - source: 'config/metrics/schema/.*\.json' test: -- GitLab From a4b88eb58eef515310fa75cf914bcd2732f37b6e Mon Sep 17 00:00:00 2001 From: Daniyal Arshad Date: Fri, 10 Jan 2025 20:18:17 -0500 Subject: [PATCH 05/10] Address review comments by Luke --- doc/api/graphql/reference/index.md | 2 +- .../workspace_variable_input.rb | 4 ++++ .../workspace_variable_input_type_enum.rb | 4 ++++ .../workspace_variable_type.rb | 4 ---- .../workspace_variable_type_enum.rb | 11 ++++++---- ee/app/models/remote_development/workspace.rb | 2 +- .../workspace_variable_input_spec.rb | 1 + .../workspace_variable_policy_spec.rb | 7 ++++--- .../workspace_operations/create_spec.rb | 20 +++++++++++++++---- .../remote_development/integration_spec.rb | 2 +- 10 files changed, 39 insertions(+), 18 deletions(-) diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index 4bee910c651255..d58223ac254dfa 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -42222,7 +42222,7 @@ Enum for the type of the variable injected in a workspace. | Value | Description | | ----- | ----------- | -| `ENVIRONMENT` | Name type. | +| `ENVIRONMENT` | Environment type. | ## Scalar types diff --git a/ee/app/graphql/types/remote_development/workspace_variable_input.rb b/ee/app/graphql/types/remote_development/workspace_variable_input.rb index 25daa0d1a78c8f..d2103d0b964c1c 100644 --- a/ee/app/graphql/types/remote_development/workspace_variable_input.rb +++ b/ee/app/graphql/types/remote_development/workspace_variable_input.rb @@ -16,11 +16,15 @@ class WorkspaceVariableInput < BaseInputObject } argument :type, Types::RemoteDevelopment::WorkspaceVariableInputTypeEnum, required: false, + default_value: Types::RemoteDevelopment::WorkspaceVariableInputTypeEnum.environment, + replace_null_with_default: true, description: 'Type of the variable to be injected in a workspace.', deprecated: { reason: 'Use `variableType` instead', milestone: '17.8' } argument :value, GraphQL::Types::String, description: 'Value of the variable.' argument :variable_type, Types::RemoteDevelopment::WorkspaceVariableTypeEnum, required: false, + default_value: Types::RemoteDevelopment::WorkspaceVariableTypeEnum.environment, + replace_null_with_default: true, description: 'Type of the variable to be injected in a workspace.' end end diff --git a/ee/app/graphql/types/remote_development/workspace_variable_input_type_enum.rb b/ee/app/graphql/types/remote_development/workspace_variable_input_type_enum.rb index 88de95f8c27586..c01fd833febed7 100644 --- a/ee/app/graphql/types/remote_development/workspace_variable_input_type_enum.rb +++ b/ee/app/graphql/types/remote_development/workspace_variable_input_type_enum.rb @@ -10,6 +10,10 @@ class WorkspaceVariableInputTypeEnum < BaseEnum ::RemoteDevelopment::Enums::Workspace::WORKSPACE_VARIABLE_TYPES_FOR_GRAPHQL, description: "#{%(name).capitalize} type." ) + + def self.environment + enum[:environment] + end end end end diff --git a/ee/app/graphql/types/remote_development/workspace_variable_type.rb b/ee/app/graphql/types/remote_development/workspace_variable_type.rb index 5f391020784860..692bb5bc06abb2 100644 --- a/ee/app/graphql/types/remote_development/workspace_variable_type.rb +++ b/ee/app/graphql/types/remote_development/workspace_variable_type.rb @@ -25,10 +25,6 @@ class WorkspaceVariableType < ::Types::BaseObject field :updated_at, Types::TimeType, null: false, description: 'Timestamp of when the workspace variable was updated.' - - def variable_type - ::RemoteDevelopment::Enums::Workspace::WORKSPACE_VARIABLE_TYPES_FOR_GRAPHQL.key(object.variable_type) - end end end end diff --git a/ee/app/graphql/types/remote_development/workspace_variable_type_enum.rb b/ee/app/graphql/types/remote_development/workspace_variable_type_enum.rb index 0689166ba8edeb..17114b61d747f7 100644 --- a/ee/app/graphql/types/remote_development/workspace_variable_type_enum.rb +++ b/ee/app/graphql/types/remote_development/workspace_variable_type_enum.rb @@ -6,10 +6,13 @@ class WorkspaceVariableTypeEnum < BaseEnum graphql_name 'WorkspaceVariableType' description 'Enum for the type of the variable injected in a workspace.' - from_rails_enum( - ::RemoteDevelopment::Enums::Workspace::WORKSPACE_VARIABLE_TYPES_FOR_GRAPHQL, - description: "#{%(name).capitalize} type." - ) + ::RemoteDevelopment::Enums::Workspace::WORKSPACE_VARIABLE_TYPES.slice(:environment).each do |name, value| + value name.to_s.upcase, value: value, description: "#{name.to_s.capitalize} type." + end + + def self.environment + enum[:environment] + end end end end diff --git a/ee/app/models/remote_development/workspace.rb b/ee/app/models/remote_development/workspace.rb index 5c87145e2032c8..140669c53b39d7 100644 --- a/ee/app/models/remote_development/workspace.rb +++ b/ee/app/models/remote_development/workspace.rb @@ -23,7 +23,7 @@ class Workspace < ApplicationRecord # noinspection RailsParamDefResolve - https://handbook.gitlab.com/handbook/tools-and-tips/editors-and-ides/jetbrains-ides/tracked-jetbrains-issues/#ruby-31542 has_one :remote_development_agent_config, through: :agent, source: :remote_development_agent_config has_many :workspace_variables, class_name: 'RemoteDevelopment::WorkspaceVariable', inverse_of: :workspace - # Currently we only support :environement type for user provided variables + # Currently we only support :environment type for user provided variables has_many :user_provided_workspace_variables, -> { user_provided.with_variable_type_environment.order_id_desc }, class_name: 'RemoteDevelopment::WorkspaceVariable', inverse_of: :workspace diff --git a/ee/spec/graphql/types/remote_development/workspace_variable_input_spec.rb b/ee/spec/graphql/types/remote_development/workspace_variable_input_spec.rb index d78c46f788f1da..619fba35dd976c 100644 --- a/ee/spec/graphql/types/remote_development/workspace_variable_input_spec.rb +++ b/ee/spec/graphql/types/remote_development/workspace_variable_input_spec.rb @@ -8,6 +8,7 @@ key type value + variableType ] end diff --git a/ee/spec/policies/remote_development/workspace_variable_policy_spec.rb b/ee/spec/policies/remote_development/workspace_variable_policy_spec.rb index b6270928834e00..d32621a194028f 100644 --- a/ee/spec/policies/remote_development/workspace_variable_policy_spec.rb +++ b/ee/spec/policies/remote_development/workspace_variable_policy_spec.rb @@ -10,6 +10,7 @@ end let_it_be(:workspace_project, refind: true) { create(:project, creator: user) } + let_it_be(:workspace_project_developer) { create(:user, developer_of: workspace_project) } let_it_be(:workspace, refind: true) do create(:workspace, project: workspace_project, agent: agent, user: user) end @@ -25,14 +26,14 @@ def permissions(user, workspace_variable) end context 'when user can read a workspace' do - it 'allows reading the corrosponding workspace variable' do + it 'allows reading the corresponding workspace variable' do expect(permissions(user, workspace_variable)).to be_allowed(:read_workspace) end end context 'when user can not read a workspace' do - it 'disallows reading the corrosponding workspace variable' do - expect(permissions(create(:user), workspace_variable)).to be_disallowed(:read_workspace) + it 'disallows reading the corresponding workspace variable' do + expect(permissions(workspace_project_developer, workspace_variable)).to be_disallowed(:read_workspace) end end end diff --git a/ee/spec/requests/api/graphql/mutations/remote_development/workspace_operations/create_spec.rb b/ee/spec/requests/api/graphql/mutations/remote_development/workspace_operations/create_spec.rb index b86b8d572868aa..0340378c23a1e8 100644 --- a/ee/spec/requests/api/graphql/mutations/remote_development/workspace_operations/create_spec.rb +++ b/ee/spec/requests/api/graphql/mutations/remote_development/workspace_operations/create_spec.rb @@ -37,6 +37,20 @@ let(:desired_state) { RemoteDevelopment::WorkspaceOperations::States::RUNNING } + let(:mutation_expected_varaiables) do + [ + { key: 'VAR1', value: 'value 1', type: 'ENVIRONMENT', variable_type: 'ENVIRONMENT' }, + { key: 'VAR2', value: 'value 2', type: 'ENVIRONMENT', variable_type: 'ENVIRONMENT' } + ] + end + + let(:service_class_expected_variables) do + [ + { key: 'VAR1', value: 'value 1', type: 'ENVIRONMENT', variable_type: 0 }, + { key: 'VAR2', value: 'value 2', type: 'ENVIRONMENT', variable_type: 0 } + ] + end + let(:all_mutation_args) do { desired_state: desired_state, @@ -46,10 +60,7 @@ project_id: workspace_project.to_global_id.to_s, project_ref: 'main', devfile_path: '.devfile.yaml', - variables: [ - { key: 'VAR1', value: 'value 1', type: 'ENVIRONMENT' }, - { key: 'VAR2', value: 'value 2', type: 'ENVIRONMENT' } - ] + variables: mutation_expected_varaiables } end @@ -61,6 +72,7 @@ let(:expected_service_args) do params = all_mutation_args.except(:cluster_agent_id, :project_id) + params[:variables] = service_class_expected_variables params[:agent] = agent params[:user] = current_user params[:project] = workspace_project diff --git a/ee/spec/requests/remote_development/integration_spec.rb b/ee/spec/requests/remote_development/integration_spec.rb index 401c72b7c652a0..4a2fdb077c1580 100644 --- a/ee/spec/requests/remote_development/integration_spec.rb +++ b/ee/spec/requests/remote_development/integration_spec.rb @@ -243,7 +243,7 @@ def do_create_workspace types = RemoteDevelopment::Enums::Workspace::WORKSPACE_VARIABLE_TYPES all_actual_vars = RemoteDevelopment::WorkspaceVariable.where(workspace: workspace) - actual_user_provided_vars = all_actual_vars.find_all { |var| var.user_provided == true } + actual_user_provided_vars = all_actual_vars.select(&:user_provided) all_actual_vars = all_actual_vars.map { |v| { key: v.key, type: types.invert[v.variable_type], value: v.value } } .sort_by { |v| v[:key] } -- GitLab From 8ad69f4503446944a58f1f10affd446a7f3c1507 Mon Sep 17 00:00:00 2001 From: Daniyal Arshad Date: Fri, 10 Jan 2025 21:49:48 -0500 Subject: [PATCH 06/10] Update workspace_variable_policy and spec --- .../workspace_variable_type.rb | 2 +- .../workspace_variable_policy.rb | 4 +- .../workspace_variable_policy_spec.rb | 101 +++++++++++++++--- 3 files changed, 88 insertions(+), 19 deletions(-) diff --git a/ee/app/graphql/types/remote_development/workspace_variable_type.rb b/ee/app/graphql/types/remote_development/workspace_variable_type.rb index 692bb5bc06abb2..1510defdc877c8 100644 --- a/ee/app/graphql/types/remote_development/workspace_variable_type.rb +++ b/ee/app/graphql/types/remote_development/workspace_variable_type.rb @@ -6,7 +6,7 @@ class WorkspaceVariableType < ::Types::BaseObject graphql_name 'WorkspaceVariable' description 'Represents a remote development workspace variable' - authorize :read_workspace + authorize :read_workspace_variable field :id, ::Types::GlobalIDType[::RemoteDevelopment::WorkspaceVariable], null: false, description: 'Global ID of the workspace variable.' diff --git a/ee/app/policies/remote_development/workspace_variable_policy.rb b/ee/app/policies/remote_development/workspace_variable_policy.rb index 3096600b19ffe0..35f057527f0e2f 100644 --- a/ee/app/policies/remote_development/workspace_variable_policy.rb +++ b/ee/app/policies/remote_development/workspace_variable_policy.rb @@ -2,8 +2,8 @@ module RemoteDevelopment class WorkspaceVariablePolicy < BasePolicy - alias_method :workspace_variable, :subject + condition(:can_read_workspace) { can?(:read_workspace, @subject.workspace) } - delegate { workspace_variable.workspace } + rule { can_read_workspace }.enable :read_workspace_variable end end diff --git a/ee/spec/policies/remote_development/workspace_variable_policy_spec.rb b/ee/spec/policies/remote_development/workspace_variable_policy_spec.rb index d32621a194028f..46ce96303bc9b2 100644 --- a/ee/spec/policies/remote_development/workspace_variable_policy_spec.rb +++ b/ee/spec/policies/remote_development/workspace_variable_policy_spec.rb @@ -3,37 +3,106 @@ require 'spec_helper' RSpec.describe RemoteDevelopment::WorkspaceVariablePolicy, feature_category: :workspaces do - let_it_be(:user, refind: true) { create(:user) } - let_it_be(:agent_project, refind: true) { create(:project, creator: user) } + include AdminModeHelper + using RSpec::Parameterized::TableSyntax + + let_it_be(:agent_project_creator, refind: true) { create(:user) } + let_it_be(:agent_project, refind: true) { create(:project, creator: agent_project_creator) } let_it_be(:agent, refind: true) do create(:ee_cluster_agent, :with_existing_workspaces_agent_config, project: agent_project) end - let_it_be(:workspace_project, refind: true) { create(:project, creator: user) } - let_it_be(:workspace_project_developer) { create(:user, developer_of: workspace_project) } + let_it_be(:workspace_project_creator, refind: true) { create(:user) } + let_it_be(:workspace_project, refind: true) { create(:project, creator: workspace_project_creator) } + let_it_be(:workspace_owner, refind: true) { create(:user) } let_it_be(:workspace, refind: true) do - create(:workspace, project: workspace_project, agent: agent, user: user) + create(:workspace, project: workspace_project, agent: agent, user: workspace_owner) + end + + let_it_be(:workspace_variable, refind: true) do + create(:workspace_variable, workspace: workspace) end - let_it_be(:workspace_variable) { create(:workspace_variable, workspace: workspace) } + let_it_be(:admin_user, refind: true) { create(:admin) } + let_it_be(:non_admin_user, refind: true) { create(:user) } + # NOTE: The following need to be `let`, not `let_it_be`, because it uses a `let` declaration from the matrix + let(:user) { admin_mode ? admin_user : non_admin_user } + + let(:policy_class) { described_class } + + subject(:policy_instance) { policy_class.new(user, workspace_variable) } before do - stub_licensed_features(remote_development: true) - end + stub_licensed_features(remote_development: licensed) + enable_admin_mode!(user) if admin_mode + workspace.update!(user: user) if workspace_owner + agent_project.add_role(user, role_on_agent_project) unless role_on_agent_project == :none + agent_project.reload + # noinspection RubyResolve - https://handbook.gitlab.com/handbook/tools-and-tips/editors-and-ides/jetbrains-ides/tracked-jetbrains-issues/#ruby-31543 + workspace_project.add_role(user, role_on_workspace_project) unless role_on_workspace_project == :none + workspace_project.reload + user.reload - def permissions(user, workspace_variable) - described_class.new(user, workspace_variable) + debug = false # Set to true to enable debugging of policies, but change back to false before committing + debug_policies(user, workspace_variable, policy_class, ability) if debug end - context 'when user can read a workspace' do - it 'allows reading the corresponding workspace variable' do - expect(permissions(user, workspace_variable)).to be_allowed(:read_workspace) + shared_examples 'fixture sanity checks' do + # noinspection RubyResolve -- Rubymine is incorrectly resolving workspace_project as `QA::Resource::Project`. + it "has fixture sanity checks" do + expect(agent_project.creator_id).not_to eq(workspace_project.creator_id) + expect(agent_project.creator_id).not_to eq(user.id) + expect(workspace_project.creator_id).not_to eq(user.id) + expect(agent.created_by_user_id).not_to eq(workspace.user_id) + expect(workspace.user_id).not_to eq(user.id) unless workspace_owner end end - context 'when user can not read a workspace' do - it 'disallows reading the corresponding workspace variable' do - expect(permissions(workspace_project_developer, workspace_variable)).to be_disallowed(:read_workspace) + # rubocop:disable Layout/LineLength -- TableSyntax should not be split across lines + where(:admin, :admin_mode, :licensed, :workspace_owner, :role_on_workspace_project, :role_on_agent_project, :allowed) do + # @formatter:off - Turn off RubyMine autoformatting + + # admin | # admin_mode | # licensed | workspace_owner | role_on_workspace_project | role_on_agent_project | allowed # check + true | true | false | false | :none | :none | false # admin_mode enabled but not licensed: not allowed + false | false | false | true | :developer | :none | false # Workspace owner and project developer but not licensed: not allowed + false | false | true | true | :guest | :none | false # Workspace owner but project guest: not allowed + false | false | false | false | :none | :maintainer | false # Cluster agent admin but not licensed: not allowed + false | false | true | false | :none | :developer | false # Not a cluster agent admin (must be maintainer): not allowed + true | false | true | false | :none | :none | false # admin but admin_mode not enabled and licensed: not allowed + true | true | true | false | :none | :none | true # admin_mode enabled and licensed: allowed + false | false | true | true | :developer | :none | true # Workspace owner and project developer: allowed + false | false | true | false | :none | :maintainer | true # Cluster agent admin: allowed + + # @formatter:on + end + # rubocop:enable Layout/LineLength + + with_them do + # NOTE: Currently :read_workspace and :update_workspace abilities have identical rules, so we can test them with + # the same table checks. If their behavior diverges in the future, we'll need to duplicate the table checks. + + describe "read_workspace_variable ability" do + let(:ability) { :read_workspace_variable } + + it_behaves_like 'fixture sanity checks' + + it { is_expected.to(allowed ? be_allowed(:read_workspace_variable) : be_disallowed(:read_workspace_variable)) } end end + + # NOTE: Leaving this method here for future use. You can also set GITLAB_DEBUG_POLICIES=1. For more details, see: + # https://docs.gitlab.com/ee/development/permissions/custom_roles.html#refactoring-abilities + # This may be generalized in the future for use across all policy specs + # Issue: https://gitlab.com/gitlab-org/gitlab/-/issues/463453 + def debug_policies(user, workspace, policy_class, ability) + puts "\n\nPolicy debug for #{policy_class} policy:\n" + puts "user: #{user.username} (id: #{user.id}, admin: #{user.admin?}, " \ + "admin_mode: #{user && Gitlab::Auth::CurrentUserMode.new(user).admin_mode?}" \ + ")\n" + + policy = policy_class.new(user, workspace) + puts "debugging :#{ability} ability:\n\n" + pp policy.debug(ability) + puts "\n\n" + end end -- GitLab From 88e6e460fbe786465753acff041262216fc17855 Mon Sep 17 00:00:00 2001 From: Daniyal Arshad Date: Mon, 13 Jan 2025 15:38:40 -0500 Subject: [PATCH 07/10] Update migration milestone and move conditions in block --- ...1227205133_add_user_provided_to_workspace_variables.rb | 2 +- ...27233633_backfill_user_provided_workspace_variables.rb | 8 ++++---- .../remote_development/workspace_variable_type_spec.rb | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/db/migrate/20241227205133_add_user_provided_to_workspace_variables.rb b/db/migrate/20241227205133_add_user_provided_to_workspace_variables.rb index 5a9c99c86d6820..07c3759b69da98 100644 --- a/db/migrate/20241227205133_add_user_provided_to_workspace_variables.rb +++ b/db/migrate/20241227205133_add_user_provided_to_workspace_variables.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class AddUserProvidedToWorkspaceVariables < Gitlab::Database::Migration[2.2] - milestone '17.8' + milestone '17.9' def change add_column :workspace_variables, :user_provided, :boolean, null: false, default: false, if_not_exists: true diff --git a/db/migrate/20241227233633_backfill_user_provided_workspace_variables.rb b/db/migrate/20241227233633_backfill_user_provided_workspace_variables.rb index 37efb7d767f596..187625cea7b6ac 100644 --- a/db/migrate/20241227233633_backfill_user_provided_workspace_variables.rb +++ b/db/migrate/20241227233633_backfill_user_provided_workspace_variables.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class BackfillUserProvidedWorkspaceVariables < Gitlab::Database::Migration[2.2] - milestone '17.8' + milestone '17.9' restrict_gitlab_migration gitlab_schema: :gitlab_main @@ -39,9 +39,9 @@ class WorkspaceVariable < MigrationRecord def up WorkspaceVariable.reset_column_information - WorkspaceVariable.where(variable_type: VARIABLE_ENV_TYPE).where.not(key: WORKSPACE_INTERNAL_VARIABLES) - .each_batch(of: BATCH_SIZE) do |batch| - batch.update_all(user_provided: true) + WorkspaceVariable.each_batch(of: BATCH_SIZE) do |batch| + batch.where(variable_type: VARIABLE_ENV_TYPE).where.not(key: WORKSPACE_INTERNAL_VARIABLES) + .update_all(user_provided: true) end end diff --git a/ee/spec/graphql/types/remote_development/workspace_variable_type_spec.rb b/ee/spec/graphql/types/remote_development/workspace_variable_type_spec.rb index 5d08923b18aef0..8ac3350b5b8529 100644 --- a/ee/spec/graphql/types/remote_development/workspace_variable_type_spec.rb +++ b/ee/spec/graphql/types/remote_development/workspace_variable_type_spec.rb @@ -13,5 +13,5 @@ specify { expect(described_class).to have_graphql_fields(fields) } - specify { expect(described_class).to require_graphql_authorizations(:read_workspace) } + specify { expect(described_class).to require_graphql_authorizations(:read_workspace_variable) } end -- GitLab From 820f03300ee8fd522ea0db96c0e93418ff935bbf Mon Sep 17 00:00:00 2001 From: Daniyal Arshad Date: Tue, 14 Jan 2025 14:52:59 -0500 Subject: [PATCH 08/10] Update migration timestamp and update API docs --- ...0250114194544_add_user_provided_to_workspace_variables.rb} | 0 ...50114194610_backfill_user_provided_workspace_variables.rb} | 0 db/schema_migrations/20241227205133 | 1 - db/schema_migrations/20241227233633 | 1 - db/schema_migrations/20250114194544 | 1 + db/schema_migrations/20250114194610 | 1 + doc/api/graphql/reference/index.md | 4 ++-- .../types/remote_development/workspace_variable_input.rb | 2 +- .../types/remote_development/workspace_variable_type.rb | 2 +- 9 files changed, 6 insertions(+), 6 deletions(-) rename db/migrate/{20241227205133_add_user_provided_to_workspace_variables.rb => 20250114194544_add_user_provided_to_workspace_variables.rb} (100%) rename db/migrate/{20241227233633_backfill_user_provided_workspace_variables.rb => 20250114194610_backfill_user_provided_workspace_variables.rb} (100%) delete mode 100644 db/schema_migrations/20241227205133 delete mode 100644 db/schema_migrations/20241227233633 create mode 100644 db/schema_migrations/20250114194544 create mode 100644 db/schema_migrations/20250114194610 diff --git a/db/migrate/20241227205133_add_user_provided_to_workspace_variables.rb b/db/migrate/20250114194544_add_user_provided_to_workspace_variables.rb similarity index 100% rename from db/migrate/20241227205133_add_user_provided_to_workspace_variables.rb rename to db/migrate/20250114194544_add_user_provided_to_workspace_variables.rb diff --git a/db/migrate/20241227233633_backfill_user_provided_workspace_variables.rb b/db/migrate/20250114194610_backfill_user_provided_workspace_variables.rb similarity index 100% rename from db/migrate/20241227233633_backfill_user_provided_workspace_variables.rb rename to db/migrate/20250114194610_backfill_user_provided_workspace_variables.rb diff --git a/db/schema_migrations/20241227205133 b/db/schema_migrations/20241227205133 deleted file mode 100644 index d085df2fb451df..00000000000000 --- a/db/schema_migrations/20241227205133 +++ /dev/null @@ -1 +0,0 @@ -58154f7eb7459696337b9c4c3caed522982a65c98d7008204c22aefd492e86ff \ No newline at end of file diff --git a/db/schema_migrations/20241227233633 b/db/schema_migrations/20241227233633 deleted file mode 100644 index 805e7c7e5c1c30..00000000000000 --- a/db/schema_migrations/20241227233633 +++ /dev/null @@ -1 +0,0 @@ -26a106689473a03e95608a79eba275fdb7867ff235b62857dc8457d740b9b709 \ No newline at end of file diff --git a/db/schema_migrations/20250114194544 b/db/schema_migrations/20250114194544 new file mode 100644 index 00000000000000..1136a615f5baff --- /dev/null +++ b/db/schema_migrations/20250114194544 @@ -0,0 +1 @@ +79ded29b939ad063a58275fec52191c01161d48b89758b05f3dc3f15c41f018d \ No newline at end of file diff --git a/db/schema_migrations/20250114194610 b/db/schema_migrations/20250114194610 new file mode 100644 index 00000000000000..04cc0d3f4f3735 --- /dev/null +++ b/db/schema_migrations/20250114194610 @@ -0,0 +1 @@ +e57acf78acb8d7c507b2e4d611be22d6741f4a0e09970776bb86f2aca266b542 \ No newline at end of file diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index d58223ac254dfa..a9471a3e230500 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -38512,7 +38512,7 @@ Represents a remote development workspace variable. | ---- | ---- | ----------- | | `createdAt` | [`Time!`](#time) | Timestamp of when the workspace variable was created. | | `id` | [`RemoteDevelopmentWorkspaceVariableID!`](#remotedevelopmentworkspacevariableid) | Global ID of the workspace variable. | -| `key` | [`String`](#string) | Key of the workspace variable. | +| `key` | [`String`](#string) | Name of the workspace variable. | | `updatedAt` | [`Time!`](#time) | Timestamp of when the workspace variable was updated. | | `value` | [`String`](#string) | Value of the workspace variable. | | `variableType` | [`WorkspaceVariableType`](#workspacevariabletype) | Type of the workspace variable. | @@ -45687,7 +45687,7 @@ Attributes for defining a variable to be injected in a workspace. | Name | Type | Description | | ---- | ---- | ----------- | -| `key` | [`String!`](#string) | Key of the variable. | +| `key` | [`String!`](#string) | Name of the workspace variable. | | `type` **{warning-solid}** | [`WorkspaceVariableInputType`](#workspacevariableinputtype) | **Deprecated:** Use `variableType` instead. Deprecated in GitLab 17.8. | | `value` | [`String!`](#string) | Value of the variable. | | `variableType` | [`WorkspaceVariableType`](#workspacevariabletype) | Type of the variable to be injected in a workspace. | diff --git a/ee/app/graphql/types/remote_development/workspace_variable_input.rb b/ee/app/graphql/types/remote_development/workspace_variable_input.rb index d2103d0b964c1c..fcb37ce74be28b 100644 --- a/ee/app/graphql/types/remote_development/workspace_variable_input.rb +++ b/ee/app/graphql/types/remote_development/workspace_variable_input.rb @@ -9,7 +9,7 @@ class WorkspaceVariableInput < BaseInputObject # do not allow empty values. also validate that the key contains only alphanumeric characters, -, _ or . # https://kubernetes.io/docs/concepts/configuration/secret/#restriction-names-data argument :key, GraphQL::Types::String, - description: 'Key of the variable.', + description: 'Name of the workspace variable.', validates: { allow_blank: false, format: { with: /\A[a-zA-Z0-9\-_.]+\z/, message: 'must contain only alphanumeric characters, -, _ or .' } diff --git a/ee/app/graphql/types/remote_development/workspace_variable_type.rb b/ee/app/graphql/types/remote_development/workspace_variable_type.rb index 1510defdc877c8..3f31a303768699 100644 --- a/ee/app/graphql/types/remote_development/workspace_variable_type.rb +++ b/ee/app/graphql/types/remote_development/workspace_variable_type.rb @@ -12,7 +12,7 @@ class WorkspaceVariableType < ::Types::BaseObject null: false, description: 'Global ID of the workspace variable.' field :key, GraphQL::Types::String, - null: true, description: 'Key of the workspace variable.' + null: true, description: 'Name of the workspace variable.' field :value, GraphQL::Types::String, null: true, description: 'Value of the workspace variable.' -- GitLab From 4dbb559137576d45a78b49bb4d23220f30897046 Mon Sep 17 00:00:00 2001 From: Daniyal Arshad Date: Tue, 14 Jan 2025 16:18:37 -0500 Subject: [PATCH 09/10] Update migration spec --- ...14194610_backfill_user_provided_workspace_variables_spec.rb} | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename spec/migrations/{20241227233633_backfill_user_provided_workspace_variables_spec.rb => 20250114194610_backfill_user_provided_workspace_variables_spec.rb} (99%) diff --git a/spec/migrations/20241227233633_backfill_user_provided_workspace_variables_spec.rb b/spec/migrations/20250114194610_backfill_user_provided_workspace_variables_spec.rb similarity index 99% rename from spec/migrations/20241227233633_backfill_user_provided_workspace_variables_spec.rb rename to spec/migrations/20250114194610_backfill_user_provided_workspace_variables_spec.rb index a8e13cd1d13309..eaa67992b0ddcb 100644 --- a/spec/migrations/20241227233633_backfill_user_provided_workspace_variables_spec.rb +++ b/spec/migrations/20250114194610_backfill_user_provided_workspace_variables_spec.rb @@ -54,7 +54,7 @@ namespace: 'workspace_1_namespace', desired_state: 'Running', actual_state: 'Running', - devfile_ref: 'devfile-ref', + project_ref: 'devfile-ref', devfile_path: 'devfile-path', devfile: 'devfile', processed_devfile: 'processed_dev_file', -- GitLab From 48c06a9f5289844ccac75a02ccdcdcfcf92bb668 Mon Sep 17 00:00:00 2001 From: Daniyal Arshad Date: Wed, 15 Jan 2025 19:33:05 -0500 Subject: [PATCH 10/10] Update milestone to 17.9 in graphql --- doc/api/graphql/reference/index.md | 4 ++-- ee/app/graphql/types/remote_development/workspace_type.rb | 2 +- .../types/remote_development/workspace_variable_input.rb | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index a9471a3e230500..406cedcdcf5f2b 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -38499,7 +38499,7 @@ Represents a remote development workspace. | `updatedAt` | [`Time!`](#time) | Timestamp of the last update to any mutable workspace property. | | `url` | [`String!`](#string) | URL of the workspace. | | `user` | [`UserCore!`](#usercore) | Owner of the workspace. | -| `workspaceVariables` **{warning-solid}** | [`WorkspaceVariableConnection`](#workspacevariableconnection) | **Introduced** in GitLab 17.8. **Status**: Experiment. User defined variables associated with the workspace. | +| `workspaceVariables` **{warning-solid}** | [`WorkspaceVariableConnection`](#workspacevariableconnection) | **Introduced** in GitLab 17.9. **Status**: Experiment. User defined variables associated with the workspace. | | `workspacesAgentConfigVersion` **{warning-solid}** | [`Int!`](#int) | **Introduced** in GitLab 17.6. **Status**: Experiment. Version of the associated WorkspacesAgentConfig for the workspace. | ### `WorkspaceVariable` @@ -45688,6 +45688,6 @@ Attributes for defining a variable to be injected in a workspace. | Name | Type | Description | | ---- | ---- | ----------- | | `key` | [`String!`](#string) | Name of the workspace variable. | -| `type` **{warning-solid}** | [`WorkspaceVariableInputType`](#workspacevariableinputtype) | **Deprecated:** Use `variableType` instead. Deprecated in GitLab 17.8. | +| `type` **{warning-solid}** | [`WorkspaceVariableInputType`](#workspacevariableinputtype) | **Deprecated:** Use `variableType` instead. Deprecated in GitLab 17.9. | | `value` | [`String!`](#string) | Value of the variable. | | `variableType` | [`WorkspaceVariableType`](#workspacevariabletype) | Type of the variable to be injected in a workspace. | diff --git a/ee/app/graphql/types/remote_development/workspace_type.rb b/ee/app/graphql/types/remote_development/workspace_type.rb index b8f386ea19fbea..1a6f5a3135dbc2 100644 --- a/ee/app/graphql/types/remote_development/workspace_type.rb +++ b/ee/app/graphql/types/remote_development/workspace_type.rb @@ -104,7 +104,7 @@ class WorkspaceType < ::Types::BaseObject field :workspace_variables, ::Types::RemoteDevelopment::WorkspaceVariableType.connection_type, null: true, - experiment: { milestone: '17.8' }, + experiment: { milestone: '17.9' }, description: 'User defined variables associated with the workspace.', method: :user_provided_workspace_variables def project_id diff --git a/ee/app/graphql/types/remote_development/workspace_variable_input.rb b/ee/app/graphql/types/remote_development/workspace_variable_input.rb index fcb37ce74be28b..6238f566826fb8 100644 --- a/ee/app/graphql/types/remote_development/workspace_variable_input.rb +++ b/ee/app/graphql/types/remote_development/workspace_variable_input.rb @@ -19,7 +19,7 @@ class WorkspaceVariableInput < BaseInputObject default_value: Types::RemoteDevelopment::WorkspaceVariableInputTypeEnum.environment, replace_null_with_default: true, description: 'Type of the variable to be injected in a workspace.', - deprecated: { reason: 'Use `variableType` instead', milestone: '17.8' } + deprecated: { reason: 'Use `variableType` instead', milestone: '17.9' } argument :value, GraphQL::Types::String, description: 'Value of the variable.' argument :variable_type, Types::RemoteDevelopment::WorkspaceVariableTypeEnum, required: false, -- GitLab