diff --git a/app/models/ci/variable.rb b/app/models/ci/variable.rb index 0e2712086ca..1d9ee91a31c 100644 --- a/app/models/ci/variable.rb +++ b/app/models/ci/variable.rb @@ -18,8 +18,12 @@ module Ci belongs_to :project, class_name: '::Project', foreign_key: :gl_project_id - validates_presence_of :key validates_uniqueness_of :key, scope: :gl_project_id + validates :key, + presence: true, + length: { within: 0..255 }, + format: { with: /\A[a-zA-Z0-9_]+\z/, + message: "can contain only letters, digits and '_'." } attr_encrypted :value, mode: :per_attribute_iv_and_salt, key: Gitlab::Application.secrets.db_key_base end diff --git a/lib/api/variables.rb b/lib/api/variables.rb index b8bbcb6ce3b..cc038e5731d 100644 --- a/lib/api/variables.rb +++ b/lib/api/variables.rb @@ -22,19 +22,12 @@ module API # # Parameters: # id (required) - The ID of a project - # variable_id (required) - The ID OR `key` of variable to show; if variable_id contains only digits it's treated - # as ID other ways it's treated as `key` + # key (required) - The `key` of variable # Example Request: - # GET /projects/:id/variables/:variable_id - get ':id/variables/:variable_id' do - variable_id = params[:variable_id] - variables = user_project.variables - variables = - if variable_id.match(/^\d+$/) - variables.where(id: variable_id.to_i) - else - variables.where(key: variable_id) - end + # GET /projects/:id/variables/:key + get ':id/variables/:key' do + key = params[:key] + variables = user_project.variables.where(key: key) return not_found!('Variable') if variables.empty? @@ -45,8 +38,8 @@ module API # # Parameters: # id (required) - The ID of a project - # key (required) - The key of variable being created - # value (required) - The value of variable being created + # key (required) - The key of variable + # value (required) - The value of variable # Example Request: # POST /projects/:id/variables post ':id/variables' do @@ -63,17 +56,15 @@ module API # # Parameters: # id (required) - The ID of a project - # variable_id (required) - The ID of a variable - # key (optional) - new value for `key` field of variable - # value (optional) - new value for `value` field of variable + # key (optional) - The `key` of variable + # value (optional) - New value for `value` field of variable # Example Request: - # PUT /projects/:id/variables/:variable_id - put ':id/variables/:variable_id' do - variable = user_project.variables.where(id: params[:variable_id].to_i).first + # PUT /projects/:id/variables/:key + put ':id/variables/:key' do + variable = user_project.variables.where(key: params[:key]).first return not_found!('Variable') unless variable - variable.key = params[:key] variable.value = params[:value] variable.save! @@ -84,11 +75,11 @@ module API # # Parameters: # id (required) - The ID of a project - # variable_id (required) - The ID of a variable + # key (required) - The ID of a variable # Exanoke Reqyest: - # DELETE /projects/:id/variables/:variable_id - delete ':id/variables/:variable_id' do - variable = user_project.variables.where(id: params[:variable_id].to_i).first + # DELETE /projects/:id/variables/:key + delete ':id/variables/:key' do + variable = user_project.variables.where(key: params[:key]).first return not_found!('Variable') unless variable diff --git a/spec/requests/api/variables_spec.rb b/spec/requests/api/variables_spec.rb index bf0dd77473a..214d7d5a0cc 100644 --- a/spec/requests/api/variables_spec.rb +++ b/spec/requests/api/variables_spec.rb @@ -37,26 +37,17 @@ describe API::API, api: true do end end - describe 'GET /projects/:id/variables/:variable_id' do + describe 'GET /projects/:id/variables/:key' do context 'authorized user with proper permissions' do - it 'should return project variable details when ID is used as :variable_id' do - get api("/projects/#{project.id}/variables/#{variable.id}", user) - - expect(response.status).to eq(200) - expect(json_response['key']).to eq(variable.key) - expect(json_response['value']).to eq(variable.value) - end - - it 'should return project variable details when `key` is used as :variable_id' do + it 'should return project variable details' do get api("/projects/#{project.id}/variables/#{variable.key}", user) expect(response.status).to eq(200) - expect(json_response['id']).to eq(variable.id) expect(json_response['value']).to eq(variable.value) end it 'should responde with 404 Not Found if requesting non-existing variable' do - get api("/projects/#{project.id}/variables/9999", user) + get api("/projects/#{project.id}/variables/non_existing_variable", user) expect(response.status).to eq(404) end @@ -64,7 +55,7 @@ describe API::API, api: true do context 'authorized user with invalid permissions' do it 'should not return project variable details' do - get api("/projects/#{project.id}/variables/#{variable.id}", user2) + get api("/projects/#{project.id}/variables/#{variable.key}", user2) expect(response.status).to eq(403) end @@ -72,7 +63,7 @@ describe API::API, api: true do context 'unauthorized user' do it 'should not return project variable details' do - get api("/projects/#{project.id}/variables/#{variable.id}") + get api("/projects/#{project.id}/variables/#{variable.key}") expect(response.status).to eq(401) end @@ -117,26 +108,23 @@ describe API::API, api: true do end end - describe 'PUT /projects/:id/variables/:variable_id' do + describe 'PUT /projects/:id/variables/:key' do context 'authorized user with proper permissions' do it 'should update variable data' do initial_variable = project.variables.first - key_before = initial_variable.key value_before = initial_variable.value - put api("/projects/#{project.id}/variables/#{variable.id}", user), key: 'TEST_VARIABLE_1_UP', value: 'VALUE_1_UP' + put api("/projects/#{project.id}/variables/#{variable.key}", user), value: 'VALUE_1_UP' updated_variable = project.variables.first expect(response.status).to eq(200) - expect(key_before).to eq(variable.key) expect(value_before).to eq(variable.value) - expect(updated_variable.key).to eq('TEST_VARIABLE_1_UP') expect(updated_variable.value).to eq('VALUE_1_UP') end it 'should responde with 404 Not Found if requesting non-existing variable' do - put api("/projects/#{project.id}/variables/9999", user) + put api("/projects/#{project.id}/variables/non_existing_variable", user) expect(response.status).to eq(404) end @@ -144,7 +132,7 @@ describe API::API, api: true do context 'authorized user with invalid permissions' do it 'should not update variable' do - put api("/projects/#{project.id}/variables/#{variable.id}", user2) + put api("/projects/#{project.id}/variables/#{variable.key}", user2) expect(response.status).to eq(403) end @@ -152,24 +140,24 @@ describe API::API, api: true do context 'unauthorized user' do it 'should not update variable' do - put api("/projects/#{project.id}/variables/#{variable.id}") + put api("/projects/#{project.id}/variables/#{variable.key}") expect(response.status).to eq(401) end end end - describe 'DELETE /projects/:id/variables/:variable_id' do + describe 'DELETE /projects/:id/variables/:key' do context 'authorized user with proper permissions' do it 'should delete variable' do expect do - delete api("/projects/#{project.id}/variables/#{variable.id}", user) + delete api("/projects/#{project.id}/variables/#{variable.key}", user) end.to change{project.variables.count}.by(-1) expect(response.status).to eq(200) end it 'should responde with 404 Not Found if requesting non-existing variable' do - delete api("/projects/#{project.id}/variables/9999", user) + delete api("/projects/#{project.id}/variables/non_existing_variable", user) expect(response.status).to eq(404) end @@ -177,7 +165,7 @@ describe API::API, api: true do context 'authorized user with invalid permissions' do it 'should not delete variable' do - delete api("/projects/#{project.id}/variables/#{variable.id}", user2) + delete api("/projects/#{project.id}/variables/#{variable.key}", user2) expect(response.status).to eq(403) end @@ -185,7 +173,7 @@ describe API::API, api: true do context 'unauthorized user' do it 'should not delete variable' do - delete api("/projects/#{project.id}/variables/#{variable.id}") + delete api("/projects/#{project.id}/variables/#{variable.key}") expect(response.status).to eq(401) end