diff options
28 files changed, 312 insertions, 192 deletions
diff --git a/app/assets/stylesheets/framework/variables.scss b/app/assets/stylesheets/framework/variables.scss index 18716813c48..a1d5f6427f4 100644 --- a/app/assets/stylesheets/framework/variables.scss +++ b/app/assets/stylesheets/framework/variables.scss @@ -486,6 +486,7 @@ $jq-ui-default-color: #777; $label-gray-bg: #f8fafc; $label-inverse-bg: #333; $label-remove-border: rgba(0, 0, 0, .1); +$label-border-radius: 14px; /* * Lint diff --git a/app/assets/stylesheets/pages/issuable.scss b/app/assets/stylesheets/pages/issuable.scss index 90587b9425b..407c0afbac8 100644 --- a/app/assets/stylesheets/pages/issuable.scss +++ b/app/assets/stylesheets/pages/issuable.scss @@ -30,6 +30,7 @@ .color-label { padding: 6px 10px; + border-radius: $label-border-radius; } } diff --git a/app/assets/stylesheets/pages/labels.scss b/app/assets/stylesheets/pages/labels.scss index b1ccd644450..25c91203ff4 100644 --- a/app/assets/stylesheets/pages/labels.scss +++ b/app/assets/stylesheets/pages/labels.scss @@ -104,7 +104,8 @@ } .color-label { - padding: 3px 4px; + padding: 3px 7px; + border-radius: $label-border-radius; } .dropdown-labels-error { diff --git a/app/models/concerns/routable.rb b/app/models/concerns/routable.rb index d36bb9da296..1108a64c59e 100644 --- a/app/models/concerns/routable.rb +++ b/app/models/concerns/routable.rb @@ -7,6 +7,7 @@ module Routable has_one :route, as: :source, autosave: true, dependent: :destroy validates_associated :route + validates :route, presence: true before_validation :update_route_path, if: :full_path_changed? end @@ -28,17 +29,17 @@ module Routable order_sql = "(CASE WHEN #{binary} routes.path = #{connection.quote(path)} THEN 0 ELSE 1 END)" - where_paths_in([path]).reorder(order_sql).take + where_full_path_in([path]).reorder(order_sql).take end # Builds a relation to find multiple objects by their full paths. # # Usage: # - # Klass.where_paths_in(%w{gitlab-org/gitlab-ce gitlab-org/gitlab-ee}) + # Klass.where_full_path_in(%w{gitlab-org/gitlab-ce gitlab-org/gitlab-ee}) # # Returns an ActiveRecord::Relation. - def where_paths_in(paths) + def where_full_path_in(paths) wheres = [] cast_lower = Gitlab::Database.postgresql? diff --git a/app/models/user.rb b/app/models/user.rb index b9bb4a9e3f7..1bd28203523 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -304,10 +304,6 @@ class User < ActiveRecord::Base personal_access_token.user if personal_access_token end - def by_username_or_id(name_or_id) - find_by('users.username = ? OR users.id = ?', name_or_id.to_s, name_or_id.to_i) - end - # Returns a user for the given SSH key. def find_by_ssh_key_id(key_id) find_by(id: Key.unscoped.select(:user_id).where(id: key_id)) diff --git a/changelogs/unreleased/25482-fix-api-sudo.yml b/changelogs/unreleased/25482-fix-api-sudo.yml new file mode 100644 index 00000000000..4c11fe1622e --- /dev/null +++ b/changelogs/unreleased/25482-fix-api-sudo.yml @@ -0,0 +1,4 @@ +--- +title: 'API: Memoize the current_user so that sudo can work properly' +merge_request: 8017 +author: diff --git a/changelogs/unreleased/allow-more-filenames.yml b/changelogs/unreleased/allow-more-filenames.yml new file mode 100644 index 00000000000..7989f94e528 --- /dev/null +++ b/changelogs/unreleased/allow-more-filenames.yml @@ -0,0 +1,4 @@ +--- +title: Allow all alphanumeric characters in file names +merge_request: 8002 +author: winniehell diff --git a/changelogs/unreleased/api-simple-group-project.yml b/changelogs/unreleased/api-simple-group-project.yml new file mode 100644 index 00000000000..54c8de610a6 --- /dev/null +++ b/changelogs/unreleased/api-simple-group-project.yml @@ -0,0 +1,4 @@ +--- +title: 'API: Simple representation of group''s projects' +merge_request: 8060 +author: Robert Schilling diff --git a/db/migrate/20161212142807_add_lower_path_index_to_routes.rb b/db/migrate/20161212142807_add_lower_path_index_to_routes.rb new file mode 100644 index 00000000000..6958500306f --- /dev/null +++ b/db/migrate/20161212142807_add_lower_path_index_to_routes.rb @@ -0,0 +1,22 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class AddLowerPathIndexToRoutes < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + return unless Gitlab::Database.postgresql? + + execute 'CREATE INDEX CONCURRENTLY index_on_routes_lower_path ON routes (LOWER(path));' + end + + def down + return unless Gitlab::Database.postgresql? + + remove_index :routes, name: :index_on_routes_lower_path + end +end diff --git a/db/schema.rb b/db/schema.rb index 9c46f573719..ae47456084f 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20161202152035) do +ActiveRecord::Schema.define(version: 20161212142807) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" diff --git a/doc/api/groups.md b/doc/api/groups.md index 5e6f498c365..134d7bda22f 100644 --- a/doc/api/groups.md +++ b/doc/api/groups.md @@ -50,12 +50,17 @@ GET /groups/:id/projects Parameters: -- `archived` (optional) - if passed, limit by archived status -- `visibility` (optional) - if passed, limit by visibility `public`, `internal`, `private` -- `order_by` (optional) - Return requests ordered by `id`, `name`, `path`, `created_at`, `updated_at` or `last_activity_at` fields. Default is `created_at` -- `sort` (optional) - Return requests sorted in `asc` or `desc` order. Default is `desc` -- `search` (optional) - Return list of authorized projects according to a search criteria -- `ci_enabled_first` - Return projects ordered by ci_enabled flag. Projects with enabled GitLab CI go first +| Attribute | Type | Required | Description | +| --------- | ---- | -------- | ----------- | +| `id` | integer/string | yes | The ID or path of a group | +| `archived` | boolean | no | Limit by archived status | +| `visibility` | string | no | Limit by visibility `public`, `internal`, or `private` | +| `order_by` | string | no | Return projects ordered by `id`, `name`, `path`, `created_at`, `updated_at`, or `last_activity_at` fields. Default is `created_at` | +| `sort` | string | no | Return projects sorted in `asc` or `desc` order. Default is `desc` | +| `search` | string | no | Return list of authorized projects matching the search criteria | +| `simple` | boolean | no | Return only the ID, URL, name, and path of each project | + +Example response: ```json [ diff --git a/features/admin/settings.feature b/features/admin/settings.feature deleted file mode 100644 index e38eea2cfed..00000000000 --- a/features/admin/settings.feature +++ /dev/null @@ -1,19 +0,0 @@ -@admin -Feature: Admin Settings - Background: - Given I sign in as an admin - And I visit admin settings page - - Scenario: Change application settings - When I modify settings and save form - Then I should see application settings saved - - Scenario: Change Slack Service Template settings - When I click on "Service Templates" - And I click on "Slack" service - And I fill out Slack settings - Then I check all events and submit form - And I should see service template settings saved - Then I click on "Slack" service - And I should see all checkboxes checked - And I should see Slack settings saved diff --git a/features/steps/shared/paths.rb b/features/steps/shared/paths.rb index 2bd8ea745e4..d36eff5cf16 100644 --- a/features/steps/shared/paths.rb +++ b/features/steps/shared/paths.rb @@ -203,10 +203,6 @@ module SharedPaths visit admin_teams_path end - step 'I visit admin settings page' do - visit admin_application_settings_path - end - step 'I visit spam logs page' do visit admin_spam_logs_path end diff --git a/lib/api/groups.rb b/lib/api/groups.rb index 105d3ee342e..9b9d3df7435 100644 --- a/lib/api/groups.rb +++ b/lib/api/groups.rb @@ -125,13 +125,16 @@ module API default: 'created_at', desc: 'Return projects ordered by field' optional :sort, type: String, values: %w[asc desc], default: 'desc', desc: 'Return projects sorted in ascending and descending order' + optional :simple, type: Boolean, default: false, + desc: 'Return only the ID, URL, name, and path of each project' use :pagination end get ":id/projects" do group = find_group!(params[:id]) projects = GroupProjectsFinder.new(group).execute(current_user) projects = filter_projects(projects) - present paginate(projects), with: Entities::Project, user: current_user + entity = params[:simple] ? Entities::BasicProjectDetails : Entities::Project + present paginate(projects), with: entity, user: current_user end desc 'Transfer a project to the group namespace. Available only for admin.' do diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index f03d8da732e..746849ef4c0 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -7,67 +7,23 @@ module API SUDO_HEADER = "HTTP_SUDO" SUDO_PARAM = :sudo - def private_token - params[PRIVATE_TOKEN_PARAM] || env[PRIVATE_TOKEN_HEADER] - end - - def warden - env['warden'] - end - - # Check the Rails session for valid authentication details - # - # Until CSRF protection is added to the API, disallow this method for - # state-changing endpoints - def find_user_from_warden - warden.try(:authenticate) if %w[GET HEAD].include?(env['REQUEST_METHOD']) - end - def declared_params(options = {}) options = { include_parent_namespaces: false }.merge(options) declared(params, options).to_h.symbolize_keys end - def find_user_by_private_token - token = private_token - return nil unless token.present? - - User.find_by_authentication_token(token) || User.find_by_personal_access_token(token) - end - def current_user - @current_user ||= find_user_by_private_token - @current_user ||= doorkeeper_guard - @current_user ||= find_user_from_warden - - unless @current_user && Gitlab::UserAccess.new(@current_user).allowed? - return nil - end + return @current_user if defined?(@current_user) - identifier = sudo_identifier + @current_user = initial_current_user - if identifier - # We check for private_token because we cannot allow PAT to be used - forbidden!('Must be admin to use sudo') unless @current_user.is_admin? - forbidden!('Private token must be specified in order to use sudo') unless private_token_used? - - @impersonator = @current_user - @current_user = User.by_username_or_id(identifier) - not_found!("No user id or username for: #{identifier}") if @current_user.nil? - end + sudo! @current_user end - def sudo_identifier - identifier ||= params[SUDO_PARAM] || env[SUDO_HEADER] - - # Regex for integers - if !!(identifier =~ /\A[0-9]+\z/) - identifier.to_i - else - identifier - end + def sudo? + initial_current_user != current_user end def user_project @@ -78,6 +34,14 @@ module API @available_labels ||= LabelsFinder.new(current_user, project_id: user_project.id).execute end + def find_user(id) + if id =~ /^\d+$/ + User.find_by(id: id) + else + User.find_by(username: id) + end + end + def find_project(id) if id =~ /^\d+$/ Project.find_by(id: id) @@ -343,6 +307,69 @@ module API private + def private_token + params[PRIVATE_TOKEN_PARAM] || env[PRIVATE_TOKEN_HEADER] + end + + def warden + env['warden'] + end + + # Check the Rails session for valid authentication details + # + # Until CSRF protection is added to the API, disallow this method for + # state-changing endpoints + def find_user_from_warden + warden.try(:authenticate) if %w[GET HEAD].include?(env['REQUEST_METHOD']) + end + + def find_user_by_private_token + token = private_token + return nil unless token.present? + + User.find_by_authentication_token(token) || User.find_by_personal_access_token(token) + end + + def initial_current_user + return @initial_current_user if defined?(@initial_current_user) + + @initial_current_user ||= find_user_by_private_token + @initial_current_user ||= doorkeeper_guard + @initial_current_user ||= find_user_from_warden + + unless @initial_current_user && Gitlab::UserAccess.new(@initial_current_user).allowed? + @initial_current_user = nil + end + + @initial_current_user + end + + def sudo! + return unless sudo_identifier + return unless initial_current_user + + unless initial_current_user.is_admin? + forbidden!('Must be admin to use sudo') + end + + # Only private tokens should be used for the SUDO feature + unless private_token == initial_current_user.private_token + forbidden!('Private token must be specified in order to use sudo') + end + + sudoed_user = find_user(sudo_identifier) + + if sudoed_user + @current_user = sudoed_user + else + not_found!("No user id or username for: #{sudo_identifier}") + end + end + + def sudo_identifier + @sudo_identifier ||= params[SUDO_PARAM] || env[SUDO_HEADER] + end + def add_pagination_headers(paginated_data) header 'X-Total', paginated_data.total_count.to_s header 'X-Total-Pages', paginated_data.total_pages.to_s @@ -375,10 +402,6 @@ module API links.join(', ') end - def private_token_used? - private_token == @current_user.private_token - end - def secret_token Gitlab::Shell.secret_token end diff --git a/lib/api/users.rb b/lib/api/users.rb index 1dab799dd61..c7db2d71017 100644 --- a/lib/api/users.rb +++ b/lib/api/users.rb @@ -353,7 +353,7 @@ module API success Entities::UserPublic end get do - present current_user, with: @impersonator ? Entities::UserWithPrivateToken : Entities::UserPublic + present current_user, with: sudo? ? Entities::UserWithPrivateToken : Entities::UserPublic end desc "Get the currently authenticated user's SSH keys" do diff --git a/lib/banzai/filter/abstract_reference_filter.rb b/lib/banzai/filter/abstract_reference_filter.rb index d904a8bd4ae..fd74eeaebe7 100644 --- a/lib/banzai/filter/abstract_reference_filter.rb +++ b/lib/banzai/filter/abstract_reference_filter.rb @@ -248,7 +248,7 @@ module Banzai end def projects_relation_for_paths(paths) - Project.where_paths_in(paths).includes(:namespace) + Project.where_full_path_in(paths).includes(:namespace) end # Returns projects for the given paths. diff --git a/lib/gitlab/regex.rb b/lib/gitlab/regex.rb index a06cf6a989c..d9d1e3cccca 100644 --- a/lib/gitlab/regex.rb +++ b/lib/gitlab/regex.rb @@ -61,7 +61,7 @@ module Gitlab end def file_name_regex - @file_name_regex ||= /\A[a-zA-Z0-9_\-\.\@]*\z/.freeze + @file_name_regex ||= /\A[[[:alnum:]]_\-\.\@]*\z/.freeze end def file_name_regex_message @@ -69,7 +69,7 @@ module Gitlab end def file_path_regex - @file_path_regex ||= /\A[a-zA-Z0-9_\-\.\/\@]*\z/.freeze + @file_path_regex ||= /\A[[[:alnum:]]_\-\.\/\@]*\z/.freeze end def file_path_regex_message diff --git a/lib/tasks/migrate/setup_postgresql.rake b/lib/tasks/migrate/setup_postgresql.rake index 141a0b74ec0..f5caca3ddbf 100644 --- a/lib/tasks/migrate/setup_postgresql.rake +++ b/lib/tasks/migrate/setup_postgresql.rake @@ -1,8 +1,12 @@ +require Rails.root.join('lib/gitlab/database') +require Rails.root.join('lib/gitlab/database/migration_helpers') require Rails.root.join('db/migrate/20151007120511_namespaces_projects_path_lower_indexes') require Rails.root.join('db/migrate/20151008110232_add_users_lower_username_email_indexes') +require Rails.root.join('db/migrate/20161212142807_add_lower_path_index_to_routes') desc 'GitLab | Sets up PostgreSQL' task setup_postgresql: :environment do NamespacesProjectsPathLowerIndexes.new.up AddUsersLowerUsernameEmailIndexes.new.up + AddLowerPathIndexToRoutes.new.up end diff --git a/spec/factories/groups.rb b/spec/factories/groups.rb index ebd3595ea64..ece6beb9fa9 100644 --- a/spec/factories/groups.rb +++ b/spec/factories/groups.rb @@ -19,5 +19,9 @@ FactoryGirl.define do trait :access_requestable do request_access_enabled true end + + trait :nested do + parent factory: :group + end end end diff --git a/features/steps/admin/settings.rb b/spec/features/admin/admin_settings_spec.rb index 11dc7f580f0..8cd66f189be 100644 --- a/features/steps/admin/settings.rb +++ b/spec/features/admin/admin_settings_spec.rb @@ -1,62 +1,53 @@ -class Spinach::Features::AdminSettings < Spinach::FeatureSteps - include SharedAuthentication - include SharedPaths - include SharedAdmin - include Gitlab::CurrentSettings +require 'spec_helper' - step 'I modify settings and save form' do +feature 'Admin updates settings', feature: true do + before(:each) do + login_as :admin + visit admin_application_settings_path + end + + scenario 'Change application settings' do uncheck 'Gravatar enabled' fill_in 'Home page URL', with: 'https://about.gitlab.com/' fill_in 'Help page text', with: 'Example text' click_button 'Save' - end - step 'I should see application settings saved' do expect(current_application_settings.gravatar_enabled).to be_falsey expect(current_application_settings.home_page_url).to eq "https://about.gitlab.com/" expect(page).to have_content "Application settings saved successfully" end - step 'I click on "Service Templates"' do + scenario 'Change Slack Service template settings' do click_link 'Service Templates' - end - - step 'I click on "Slack" service' do click_link 'Slack' - end - - step 'I check all events and submit form' do - page.check('Active') - page.check('Push') - page.check('Tag push') - page.check('Note') - page.check('Issue') - page.check('Merge request') - page.check('Build') - page.check('Pipeline') - click_on 'Save' - end - - step 'I fill out Slack settings' do fill_in 'Webhook', with: 'http://localhost' fill_in 'Username', with: 'test_user' fill_in 'service_push_channel', with: '#test_channel' page.check('Notify only broken builds') - end - step 'I should see service template settings saved' do + check_all_events + click_on 'Save' + expect(page).to have_content 'Application settings saved successfully' - end - step 'I should see all checkboxes checked' do + click_link 'Slack' + page.all('input[type=checkbox]').each do |checkbox| expect(checkbox).to be_checked end - end - - step 'I should see Slack settings saved' do expect(find_field('Webhook').value).to eq 'http://localhost' expect(find_field('Username').value).to eq 'test_user' expect(find('#service_push_channel').value).to eq '#test_channel' end + + def check_all_events + page.check('Active') + page.check('Push') + page.check('Tag push') + page.check('Note') + page.check('Issue') + page.check('Merge request') + page.check('Build') + page.check('Pipeline') + end end diff --git a/spec/features/projects/files/creating_a_file_spec.rb b/spec/features/projects/files/creating_a_file_spec.rb new file mode 100644 index 00000000000..ae448706130 --- /dev/null +++ b/spec/features/projects/files/creating_a_file_spec.rb @@ -0,0 +1,44 @@ +require 'spec_helper' + +feature 'User wants to create a file', feature: true do + include WaitForAjax + + let(:project) { create(:project) } + let(:user) { create(:user) } + + background do + project.team << [user, :master] + login_as user + visit namespace_project_new_blob_path(project.namespace, project, project.default_branch) + end + + def submit_new_file(options) + file_name = find('#file_name') + file_name.set options[:file_name] || 'README.md' + + file_content = find('#file-content') + file_content.set options[:file_content] || 'Some content' + + click_button 'Commit Changes' + end + + scenario 'file name contains Chinese characters' do + submit_new_file(file_name: '测试.md') + expect(page).to have_content 'The file has been successfully created.' + end + + scenario 'directory name contains Chinese characters' do + submit_new_file(file_name: '中文/测试.md') + expect(page).to have_content 'The file has been successfully created.' + end + + scenario 'file name contains invalid characters' do + submit_new_file(file_name: '\\') + expect(page).to have_content 'Your changes could not be committed, because the file name can contain only' + end + + scenario 'file name contains directory traversal' do + submit_new_file(file_name: '../README.md') + expect(page).to have_content 'Your changes could not be committed, because the file name cannot include directory traversal.' + end +end diff --git a/spec/models/concerns/routable_spec.rb b/spec/models/concerns/routable_spec.rb index 0acefc0c1d5..b556135532f 100644 --- a/spec/models/concerns/routable_spec.rb +++ b/spec/models/concerns/routable_spec.rb @@ -3,6 +3,10 @@ require 'spec_helper' describe Group, 'Routable' do let!(:group) { create(:group) } + describe 'Validations' do + it { is_expected.to validate_presence_of(:route) } + end + describe 'Associations' do it { is_expected.to have_one(:route).dependent(:destroy) } end @@ -35,16 +39,16 @@ describe Group, 'Routable' do it { expect(described_class.find_by_full_path('unknown')).to eq(nil) } end - describe '.where_paths_in' do + describe '.where_full_path_in' do context 'without any paths' do it 'returns an empty relation' do - expect(described_class.where_paths_in([])).to eq([]) + expect(described_class.where_full_path_in([])).to eq([]) end end context 'without any valid paths' do it 'returns an empty relation' do - expect(described_class.where_paths_in(%w[unknown])).to eq([]) + expect(described_class.where_full_path_in(%w[unknown])).to eq([]) end end @@ -52,13 +56,13 @@ describe Group, 'Routable' do let!(:nested_group) { create(:group, parent: group) } it 'returns the projects matching the paths' do - result = described_class.where_paths_in([group.to_param, nested_group.to_param]) + result = described_class.where_full_path_in([group.to_param, nested_group.to_param]) expect(result).to contain_exactly(group, nested_group) end it 'returns projects regardless of the casing of paths' do - result = described_class.where_paths_in([group.to_param.upcase, nested_group.to_param.upcase]) + result = described_class.where_full_path_in([group.to_param.upcase, nested_group.to_param.upcase]) expect(result).to contain_exactly(group, nested_group) end diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 1613a586a2c..850b1a3cf1e 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -271,4 +271,11 @@ describe Group, models: true do expect(group.web_url).to include("groups/#{group.name}") end end + + describe 'nested group' do + subject { create(:group, :nested) } + + it { is_expected.to be_valid } + it { expect(subject.parent).to be_kind_of(Group) } + end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index bad6ed9e146..8b20ee81614 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -727,17 +727,6 @@ describe User, models: true do end end - describe 'by_username_or_id' do - let(:user1) { create(:user, username: 'foo') } - - it "gets the correct user" do - expect(User.by_username_or_id(user1.id)).to eq(user1) - expect(User.by_username_or_id('foo')).to eq(user1) - expect(User.by_username_or_id(-1)).to be_nil - expect(User.by_username_or_id('bar')).to be_nil - end - end - describe '.find_by_ssh_key_id' do context 'using an existing SSH key ID' do let(:user) { create(:user) } diff --git a/spec/requests/api/groups_spec.rb b/spec/requests/api/groups_spec.rb index 15647b262b6..a75ba824e85 100644 --- a/spec/requests/api/groups_spec.rb +++ b/spec/requests/api/groups_spec.rb @@ -243,17 +243,28 @@ describe API::Groups, api: true do expect(json_response.length).to eq(2) project_names = json_response.map { |proj| proj['name' ] } expect(project_names).to match_array([project1.name, project3.name]) + expect(json_response.first['default_branch']).to be_present + end + + it "returns the group's projects with simple representation" do + get api("/groups/#{group1.id}/projects", user1), simple: true + + expect(response).to have_http_status(200) + expect(json_response.length).to eq(2) + project_names = json_response.map { |proj| proj['name' ] } + expect(project_names).to match_array([project1.name, project3.name]) + expect(json_response.first['default_branch']).not_to be_present end it 'filters the groups projects' do - public_projet = create(:project, :public, path: 'test1', group: group1) + public_project = create(:project, :public, path: 'test1', group: group1) get api("/groups/#{group1.id}/projects", user1), visibility: 'public' expect(response).to have_http_status(200) expect(json_response).to be_an(Array) expect(json_response.length).to eq(1) - expect(json_response.first['name']).to eq(public_projet.name) + expect(json_response.first['name']).to eq(public_project.name) end it "does not return a non existing group" do diff --git a/spec/requests/api/api_helpers_spec.rb b/spec/requests/api/helpers_spec.rb index 3f34309f419..4035fd97af5 100644 --- a/spec/requests/api/api_helpers_spec.rb +++ b/spec/requests/api/helpers_spec.rb @@ -2,7 +2,6 @@ require 'spec_helper' describe API::Helpers, api: true do include API::Helpers - include ApiHelpers include SentryHelper let(:user) { create(:user) } @@ -13,18 +12,18 @@ describe API::Helpers, api: true do let(:env) { { 'REQUEST_METHOD' => 'GET' } } let(:request) { Rack::Request.new(env) } - def set_env(token_usr, identifier) + def set_env(user_or_token, identifier) clear_env clear_param - env[API::Helpers::PRIVATE_TOKEN_HEADER] = token_usr.private_token - env[API::Helpers::SUDO_HEADER] = identifier + env[API::Helpers::PRIVATE_TOKEN_HEADER] = user_or_token.respond_to?(:private_token) ? user_or_token.private_token : user_or_token + env[API::Helpers::SUDO_HEADER] = identifier.to_s end - def set_param(token_usr, identifier) + def set_param(user_or_token, identifier) clear_env clear_param - params[API::Helpers::PRIVATE_TOKEN_PARAM] = token_usr.private_token - params[API::Helpers::SUDO_PARAM] = identifier + params[API::Helpers::PRIVATE_TOKEN_PARAM] = user_or_token.respond_to?(:private_token) ? user_or_token.private_token : user_or_token + params[API::Helpers::SUDO_PARAM] = identifier.to_s end def clear_env @@ -163,6 +162,13 @@ describe API::Helpers, api: true do expect(current_user).to eq(user) end + it 'memoize the current_user: sudo permissions are not run against the sudoed user' do + set_env(admin, user.id) + + expect(current_user).to eq(user) + expect(current_user).to eq(user) + end + it 'handles sudo to oneself' do set_env(admin, admin.id) @@ -294,33 +300,48 @@ describe API::Helpers, api: true do end end - describe '.sudo_identifier' do - it "returns integers when input is an int" do - set_env(admin, '123') - expect(sudo_identifier).to eq(123) - set_env(admin, '0001234567890') - expect(sudo_identifier).to eq(1234567890) - - set_param(admin, '123') - expect(sudo_identifier).to eq(123) - set_param(admin, '0001234567890') - expect(sudo_identifier).to eq(1234567890) + describe '.sudo?' do + context 'when no sudo env or param is passed' do + before do + doorkeeper_guard_returns(nil) + end + + it 'returns false' do + expect(sudo?).to be_falsy + end end - it "returns string when input is an is not an int" do - set_env(admin, '12.30') - expect(sudo_identifier).to eq("12.30") - set_env(admin, 'hello') - expect(sudo_identifier).to eq('hello') - set_env(admin, ' 123') - expect(sudo_identifier).to eq(' 123') - - set_param(admin, '12.30') - expect(sudo_identifier).to eq("12.30") - set_param(admin, 'hello') - expect(sudo_identifier).to eq('hello') - set_param(admin, ' 123') - expect(sudo_identifier).to eq(' 123') + context 'when sudo env or param is passed', 'user is not an admin' do + before do + set_env(user, '123') + end + + it 'returns an 403 Forbidden' do + expect { sudo? }.to raise_error '403 - {"message"=>"403 Forbidden - Must be admin to use sudo"}' + end + end + + context 'when sudo env or param is passed', 'user is admin' do + context 'personal access token is used' do + before do + personal_access_token = create(:personal_access_token, user: admin) + set_env(personal_access_token.token, user.id) + end + + it 'returns an 403 Forbidden' do + expect { sudo? }.to raise_error '403 - {"message"=>"403 Forbidden - Private token must be specified in order to use sudo"}' + end + end + + context 'private access token is used' do + before do + set_env(admin.private_token, user.id) + end + + it 'returns true' do + expect(sudo?).to be_truthy + end + end end end diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb index c37dbfa0a33..9e317f3a7e9 100644 --- a/spec/requests/api/users_spec.rb +++ b/spec/requests/api/users_spec.rb @@ -651,13 +651,12 @@ describe API::Users, api: true do end describe "GET /user" do - let(:personal_access_token) { create(:personal_access_token, user: user) } - let(:private_token) { user.private_token } + let(:personal_access_token) { create(:personal_access_token, user: user).token } context 'with regular user' do context 'with personal access token' do it 'returns 403 without private token when sudo is defined' do - get api("/user?private_token=#{personal_access_token.token}&sudo=#{user.id}") + get api("/user?private_token=#{personal_access_token}&sudo=123") expect(response).to have_http_status(403) end @@ -665,7 +664,7 @@ describe API::Users, api: true do context 'with private token' do it 'returns 403 without private token when sudo defined' do - get api("/user?private_token=#{private_token}&sudo=#{user.id}") + get api("/user?private_token=#{user.private_token}&sudo=123") expect(response).to have_http_status(403) end @@ -676,40 +675,44 @@ describe API::Users, api: true do expect(response).to have_http_status(200) expect(response).to match_response_schema('user/public') + expect(json_response['id']).to eq(user.id) end end context 'with admin' do - let(:user) { create(:admin) } + let(:admin_personal_access_token) { create(:personal_access_token, user: admin).token } context 'with personal access token' do it 'returns 403 without private token when sudo defined' do - get api("/user?private_token=#{personal_access_token.token}&sudo=#{user.id}") + get api("/user?private_token=#{admin_personal_access_token}&sudo=#{user.id}") expect(response).to have_http_status(403) end - it 'returns current user without private token when sudo not defined' do - get api("/user?private_token=#{personal_access_token.token}") + it 'returns initial current user without private token when sudo not defined' do + get api("/user?private_token=#{admin_personal_access_token}") expect(response).to have_http_status(200) expect(response).to match_response_schema('user/public') + expect(json_response['id']).to eq(admin.id) end end context 'with private token' do - it 'returns current user with private token when sudo defined' do - get api("/user?private_token=#{private_token}&sudo=#{user.id}") + it 'returns sudoed user with private token when sudo defined' do + get api("/user?private_token=#{admin.private_token}&sudo=#{user.id}") expect(response).to have_http_status(200) expect(response).to match_response_schema('user/login') + expect(json_response['id']).to eq(user.id) end - it 'returns current user without private token when sudo not defined' do - get api("/user?private_token=#{private_token}") + it 'returns initial current user without private token when sudo not defined' do + get api("/user?private_token=#{admin.private_token}") expect(response).to have_http_status(200) expect(response).to match_response_schema('user/public') + expect(json_response['id']).to eq(admin.id) end end end |