From 9769c2d7fd0728caf951858162ec7df6f93a8a83 Mon Sep 17 00:00:00 2001 From: Robert Schilling Date: Tue, 19 Aug 2014 00:23:02 +0200 Subject: Fix #6417: users with group permission should be able to create groups via API --- doc/api/groups.md | 2 +- lib/api/groups.rb | 4 ++-- spec/factories.rb | 1 + spec/requests/api/groups_spec.rb | 19 ++++++++++--------- 4 files changed, 14 insertions(+), 12 deletions(-) diff --git a/doc/api/groups.md b/doc/api/groups.md index b5a4b05ccaf..c903a850fdd 100644 --- a/doc/api/groups.md +++ b/doc/api/groups.md @@ -35,7 +35,7 @@ Parameters: ## New group -Creates a new project group. Available only for admin. +Creates a new project group. Available only for users who can create groups. ``` POST /groups diff --git a/lib/api/groups.rb b/lib/api/groups.rb index a92abd4b690..218cec40884 100644 --- a/lib/api/groups.rb +++ b/lib/api/groups.rb @@ -20,7 +20,7 @@ module API present @groups, with: Entities::Group end - # Create group. Available only for admin + # Create group. Available only for users who can create groups. # # Parameters: # name (required) - The name of the group @@ -28,7 +28,7 @@ module API # Example Request: # POST /groups post do - authenticated_as_admin! + authorize! :create_group, current_user required_attributes! [:name, :path] attrs = attributes_for_keys [:name, :path, :description] diff --git a/spec/factories.rb b/spec/factories.rb index fc103e5b133..d2b0eeea083 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -22,6 +22,7 @@ FactoryGirl.define do password "12345678" confirmed_at { Time.now } confirmation_token { nil } + can_create_group true trait :admin do admin true diff --git a/spec/requests/api/groups_spec.rb b/spec/requests/api/groups_spec.rb index d963dbac9f1..62b42d63fc2 100644 --- a/spec/requests/api/groups_spec.rb +++ b/spec/requests/api/groups_spec.rb @@ -3,8 +3,9 @@ require 'spec_helper' describe API::API, api: true do include ApiHelpers - let(:user1) { create(:user) } + let(:user1) { create(:user, can_create_group: false) } let(:user2) { create(:user) } + let(:user3) { create(:user) } let(:admin) { create(:admin) } let!(:group1) { create(:group) } let!(:group2) { create(:group) } @@ -94,32 +95,32 @@ describe API::API, api: true do end describe "POST /groups" do - context "when authenticated as user" do + context "when authenticated as user without group permissions" do it "should not create group" do post api("/groups", user1), attributes_for(:group) expect(response.status).to eq(403) end end - context "when authenticated as admin" do + context "when authenticated as user with group permissions" do it "should create group" do - post api("/groups", admin), attributes_for(:group) + post api("/groups", user3), attributes_for(:group) expect(response.status).to eq(201) end it "should not create group, duplicate" do - post api("/groups", admin), {name: "Duplicate Test", path: group2.path} + post api("/groups", user3), {name: 'Duplicate Test', path: group2.path} expect(response.status).to eq(400) expect(response.message).to eq("Bad Request") end it "should return 400 bad request error if name not given" do - post api("/groups", admin), {path: group2.path} + post api("/groups", user3), {path: group2.path} expect(response.status).to eq(400) end it "should return 400 bad request error if path not given" do - post api("/groups", admin), { name: 'test' } + post api("/groups", user3), {name: 'test'} expect(response.status).to eq(400) end end @@ -133,8 +134,8 @@ describe API::API, api: true do end it "should not remove a group if not an owner" do - user3 = create(:user) - group1.add_user(user3, Gitlab::Access::MASTER) + user4 = create(:user) + group1.add_user(user4, Gitlab::Access::MASTER) delete api("/groups/#{group1.id}", user3) expect(response.status).to eq(403) end -- cgit v1.2.1 From d386bb780864f4fc36490e19ea654f31bf193d0f Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Thu, 30 Apr 2015 16:17:03 +0200 Subject: Allow primary email to be set to an email that you've already added. --- CHANGELOG | 1 + app/controllers/admin/users_controller.rb | 3 +-- app/controllers/profiles/emails_controller.rb | 11 ++++++----- app/models/email.rb | 5 ----- app/models/user.rb | 28 +++++++++++++++++++++++++-- app/views/profiles/emails/index.html.haml | 16 +++++++++++---- 6 files changed, 46 insertions(+), 18 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index ecffcb5262c..72b0d697e7d 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,6 +1,7 @@ Please view this file on the master branch, on stable branches it's out of date. v 7.11.0 (unreleased) + - Allow primary email to be set to an email that you've already added. - Get Gitorious importer to work again. - Fix clone URL field and X11 Primary selection (Dmitry Medvinsky) - Ignore invalid lines in .gitmodules diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index adb83996f8b..d36e359934c 100644 --- a/app/controllers/admin/users_controller.rb +++ b/app/controllers/admin/users_controller.rb @@ -102,8 +102,7 @@ class Admin::UsersController < Admin::ApplicationController email = user.emails.find(params[:email_id]) email.destroy - user.set_notification_email - user.save if user.notification_email_changed? + user.update_secondary_emails! respond_to do |format| format.html { redirect_to :back, notice: "Successfully removed email." } diff --git a/app/controllers/profiles/emails_controller.rb b/app/controllers/profiles/emails_controller.rb index 954c98c0d9f..dddca05ce24 100644 --- a/app/controllers/profiles/emails_controller.rb +++ b/app/controllers/profiles/emails_controller.rb @@ -3,14 +3,17 @@ class Profiles::EmailsController < ApplicationController def index @primary = current_user.email - @public_email = current_user.public_email @emails = current_user.emails end def create @email = current_user.emails.new(email_params) - flash[:alert] = @email.errors.full_messages.first unless @email.save + if @email.save + NotificationService.new.new_email(@email) + else + flash[:alert] = @email.errors.full_messages.first + end redirect_to profile_emails_url end @@ -19,9 +22,7 @@ class Profiles::EmailsController < ApplicationController @email = current_user.emails.find(params[:id]) @email.destroy - current_user.set_notification_email - current_user.set_public_email - current_user.save if current_user.notification_email_changed? or current_user.public_email_changed? + current_user.update_secondary_emails! respond_to do |format| format.html { redirect_to profile_emails_url } diff --git a/app/models/email.rb b/app/models/email.rb index 556b0e9586e..935705e2ed4 100644 --- a/app/models/email.rb +++ b/app/models/email.rb @@ -18,7 +18,6 @@ class Email < ActiveRecord::Base validates :email, presence: true, email: { strict_mode: true }, uniqueness: true validate :unique_email, if: ->(email) { email.email_changed? } - after_create :notify before_validation :cleanup_email def cleanup_email @@ -28,8 +27,4 @@ class Email < ActiveRecord::Base def unique_email self.errors.add(:email, 'has already been taken') if User.exists?(email: self.email) end - - def notify - NotificationService.new.new_email(self) - end end diff --git a/app/models/user.rb b/app/models/user.rb index d6b93afe739..847100de282 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -139,6 +139,7 @@ class User < ActiveRecord::Base validate :avatar_type, if: ->(user) { user.avatar_changed? } validate :unique_email, if: ->(user) { user.email_changed? } validate :owns_notification_email, if: ->(user) { user.notification_email_changed? } + validate :owns_public_email, if: ->(user) { user.public_email_changed? } validates :avatar, file_size: { maximum: 200.kilobytes.to_i } before_validation :generate_password, on: :create @@ -146,6 +147,7 @@ class User < ActiveRecord::Base before_validation :set_notification_email, if: ->(user) { user.email_changed? } before_validation :set_public_email, if: ->(user) { user.public_email_changed? } + after_update :update_emails_with_primary_email, if: ->(user) { user.email_changed? } before_save :ensure_authentication_token after_save :ensure_namespace_correct after_initialize :set_projects_limit @@ -276,13 +278,29 @@ class User < ActiveRecord::Base end def unique_email - self.errors.add(:email, 'has already been taken') if Email.exists?(email: self.email) + if !self.emails.exists?(email: self.email) && Email.exists?(email: self.email) + self.errors.add(:email, 'has already been taken') + end end def owns_notification_email self.errors.add(:notification_email, "is not an email you own") unless self.all_emails.include?(self.notification_email) end + def owns_public_email + self.errors.add(:public_email, "is not an email you own") unless self.all_emails.include?(self.public_email) + end + + def update_emails_with_primary_email + primary_email_record = self.emails.find_by(email: self.email) + if primary_email_record + primary_email_record.destroy + self.emails.create(email: self.email_was) + + self.update_secondary_emails! + end + end + # Groups user has access to def authorized_groups @authorized_groups ||= begin @@ -448,10 +466,16 @@ class User < ActiveRecord::Base def set_public_email if self.public_email.blank? || !self.all_emails.include?(self.public_email) - self.public_email = '' + self.public_email = nil end end + def update_secondary_emails! + self.set_notification_email + self.set_public_email + self.save if self.notification_email_changed? || self.public_email_changed? + end + def set_projects_limit connection_default_value_defined = new_record? && !projects_limit_changed? return unless self.projects_limit.nil? || connection_default_value_defined diff --git a/app/views/profiles/emails/index.html.haml b/app/views/profiles/emails/index.html.haml index 09f290429ea..9ef49ef9ec9 100644 --- a/app/views/profiles/emails/index.html.haml +++ b/app/views/profiles/emails/index.html.haml @@ -4,11 +4,15 @@ Your %b Primary Email will be used for avatar detection and web based operations, such as edits and merges. - %br +%p.light Your %b Notification Email will be used for account notifications. - %br +%p.light + Your + %b Public Email + will be displayed on your public profile. +%p.light All email addresses will be used to identify your commits. %hr @@ -20,13 +24,17 @@ %li %strong= @primary %span.label.label-success Primary Email - - if @primary === @public_email + - if @primary === current_user.public_email %span.label.label-info Public Email + - if @primary === current_user.notification_email + %span.label.label-info Notification Email - @emails.each do |email| %li %strong= email.email - - if email.email === @public_email + - if email.email === current_user.public_email %span.label.label-info Public Email + - if email.email === current_user.notification_email + %span.label.label-info Notification Email %span.cgray added #{time_ago_with_tooltip(email.created_at)} = link_to 'Remove', profile_email_path(email), data: { confirm: 'Are you sure?'}, method: :delete, class: 'btn btn-sm btn-remove pull-right' -- cgit v1.2.1 From 8d17e79d1dfc8fb415b3fe4dbabd11659fa6c42e Mon Sep 17 00:00:00 2001 From: Dominik Sander Date: Thu, 30 Apr 2015 23:36:42 +0200 Subject: Removed unused ApplicationController#set_filter_values method The instance variables assigned in `set_filter_values` are not used by anything anymore. --- app/controllers/application_controller.rb | 25 ------------------------- 1 file changed, 25 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 69fd7901832..f56f88b58fa 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -286,40 +286,15 @@ class ApplicationController < ActionController::Base @filter_params end - def set_filter_values(collection) - assignee_id = @filter_params[:assignee_id] - author_id = @filter_params[:author_id] - milestone_id = @filter_params[:milestone_id] - - @sort = @filter_params[:sort] - @assignees = User.where(id: collection.pluck(:assignee_id)) - @authors = User.where(id: collection.pluck(:author_id)) - @milestones = Milestone.where(id: collection.pluck(:milestone_id)) - - if assignee_id.present? && !assignee_id.to_i.zero? - @assignee = @assignees.find_by(id: assignee_id) - end - - if author_id.present? && !author_id.to_i.zero? - @author = @authors.find_by(id: author_id) - end - - if milestone_id.present? && !milestone_id.to_i.zero? - @milestone = @milestones.find_by(id: milestone_id) - end - end - def get_issues_collection set_filters_params issues = IssuesFinder.new.execute(current_user, @filter_params) - set_filter_values(issues) issues end def get_merge_requests_collection set_filters_params merge_requests = MergeRequestsFinder.new.execute(current_user, @filter_params) - set_filter_values(merge_requests) merge_requests end -- cgit v1.2.1 From e6ee8d0ebebc217e247558507eac619931a47121 Mon Sep 17 00:00:00 2001 From: Dominik Sander Date: Thu, 30 Apr 2015 23:29:00 +0200 Subject: Group milestones by title in the dashboard and all other issue views This groups milestones by title for issue views like it has been done for the milestone dashboard/project overview. Before milestones with the same title would show up multiple times in the filter dropdown and one could only filter per project and milestone. Now the milestone filter is based on the title of the milestone, i.e. all issues marked with the same milestone title are shown. --- CHANGELOG | 1 + app/finders/issuable_finder.rb | 5 +++-- app/helpers/milestones_helper.rb | 3 ++- app/views/shared/_issuable_filter.html.haml | 4 ++-- spec/features/issues_spec.rb | 4 ++-- spec/finders/issues_finder_spec.rb | 2 +- 6 files changed, 11 insertions(+), 8 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 3af83ddc256..902ca7a5072 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -20,6 +20,7 @@ v 7.11.0 (unreleased) - Don't crash when an MR from a fork has a cross-reference comment from the target project on of its commits. - Include commit comments in MR from a forked project. - Fix adding new group members from admin area + - Group milestones by title in the dashboard and all other issue views. - Add default project and snippet visibility settings to the admin web UI. - - Fix bug where commit data would not appear in some subdirectories (Stan Hu) diff --git a/app/finders/issuable_finder.rb b/app/finders/issuable_finder.rb index 2c0702073d4..b8f367c6339 100644 --- a/app/finders/issuable_finder.rb +++ b/app/finders/issuable_finder.rb @@ -113,8 +113,9 @@ class IssuableFinder end def by_milestone(items) - if params[:milestone_id].present? - items = items.where(milestone_id: (params[:milestone_id] == NONE ? nil : params[:milestone_id])) + if params[:milestone_title].present? + milestone_ids = (params[:milestone_title] == NONE ? nil : Milestone.where(title: params[:milestone_title]).pluck(:id)) + items = items.where(milestone_id: milestone_ids) end items diff --git a/app/helpers/milestones_helper.rb b/app/helpers/milestones_helper.rb index 282bdf744d2..93e33ebefd8 100644 --- a/app/helpers/milestones_helper.rb +++ b/app/helpers/milestones_helper.rb @@ -28,6 +28,7 @@ module MilestonesHelper Milestone.where(project_id: @projects) end.active - options_from_collection_for_select(milestones, 'id', 'title', params[:milestone_id]) + grouped_milestones = Milestones::GroupService.new(milestones).execute + options_from_collection_for_select(grouped_milestones, 'title', 'title', params[:milestone_title]) end end diff --git a/app/views/shared/_issuable_filter.html.haml b/app/views/shared/_issuable_filter.html.haml index f9eb2dcfa28..fa8b4eae314 100644 --- a/app/views/shared/_issuable_filter.html.haml +++ b/app/views/shared/_issuable_filter.html.haml @@ -15,7 +15,7 @@ #{state_filters_text_for(:all, @project)} .issues-details-filters - = form_tag page_filter_path(without: [:assignee_id, :author_id, :milestone_id, :label_name]), method: :get, class: 'filter-form' do + = form_tag page_filter_path(without: [:assignee_id, :author_id, :milestone_title, :label_name]), method: :get, class: 'filter-form' do - if controller.controller_name == 'issues' .check-all-holder = check_box_tag "check_all_issues", nil, false, @@ -31,7 +31,7 @@ placeholder: 'Author', class: 'trigger-submit', any_user: true, first_user: true) .filter-item.inline.milestone-filter - = select_tag('milestone_id', projects_milestones_options, class: "select2 trigger-submit", prompt: 'Milestone') + = select_tag('milestone_title', projects_milestones_options, class: "select2 trigger-submit", prompt: 'Milestone') - if @project .filter-item.inline.labels-filter diff --git a/spec/features/issues_spec.rb b/spec/features/issues_spec.rb index e217f0739d2..66d73b2505c 100644 --- a/spec/features/issues_spec.rb +++ b/spec/features/issues_spec.rb @@ -95,7 +95,7 @@ describe 'Issues', feature: true do let(:issue) { @issue } it 'should allow filtering by issues with no specified milestone' do - visit namespace_project_issues_path(project.namespace, project, milestone_id: IssuableFinder::NONE) + visit namespace_project_issues_path(project.namespace, project, milestone_title: IssuableFinder::NONE) expect(page).not_to have_content 'foobar' expect(page).to have_content 'barbaz' @@ -103,7 +103,7 @@ describe 'Issues', feature: true do end it 'should allow filtering by a specified milestone' do - visit namespace_project_issues_path(project.namespace, project, milestone_id: issue.milestone.id) + visit namespace_project_issues_path(project.namespace, project, milestone_title: issue.milestone.title) expect(page).to have_content 'foobar' expect(page).not_to have_content 'barbaz' diff --git a/spec/finders/issues_finder_spec.rb b/spec/finders/issues_finder_spec.rb index 479fa950387..69bac387d20 100644 --- a/spec/finders/issues_finder_spec.rb +++ b/spec/finders/issues_finder_spec.rb @@ -43,7 +43,7 @@ describe IssuesFinder do end it 'should filter by milestone id' do - params = { scope: "all", milestone_id: milestone.id, state: 'opened' } + params = { scope: "all", milestone_title: milestone.title, state: 'opened' } issues = IssuesFinder.new.execute(user, params) expect(issues).to eq([issue1]) end -- cgit v1.2.1 From 2c544d43c832e816614a9bef35e1899f34b4a53d Mon Sep 17 00:00:00 2001 From: jubianchi Date: Sat, 17 Jan 2015 23:45:39 +0100 Subject: Query issues, merge requests and milestones with their IID through API --- CHANGELOG | 1 + doc/api/issues.md | 2 ++ doc/api/merge_requests.md | 4 +++- doc/api/milestones.md | 2 ++ lib/api/helpers.rb | 4 ++++ lib/api/issues.rb | 3 +++ lib/api/merge_requests.rb | 16 +++++++++++----- spec/requests/api/issues_spec.rb | 8 ++++++++ spec/requests/api/merge_requests_spec.rb | 8 ++++++++ spec/requests/api/milestones_spec.rb | 7 +++++++ 10 files changed, 49 insertions(+), 6 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 3af83ddc256..b5bacedc48f 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -20,6 +20,7 @@ v 7.11.0 (unreleased) - Don't crash when an MR from a fork has a cross-reference comment from the target project on of its commits. - Include commit comments in MR from a forked project. - Fix adding new group members from admin area + - Query issues, merge requests and milestones with their IID through API (Julien Bianchi) - Add default project and snippet visibility settings to the admin web UI. - - Fix bug where commit data would not appear in some subdirectories (Stan Hu) diff --git a/doc/api/issues.md b/doc/api/issues.md index a7dd8b74c35..d407bc35d79 100644 --- a/doc/api/issues.md +++ b/doc/api/issues.md @@ -99,11 +99,13 @@ GET /projects/:id/issues?labels=foo,bar GET /projects/:id/issues?labels=foo,bar&state=opened GET /projects/:id/issues?milestone=1.0.0 GET /projects/:id/issues?milestone=1.0.0&state=opened +GET /projects/:id/issues?iid=42 ``` Parameters: - `id` (required) - The ID of a project +- `iid` (optional) - Return the issue having the given `iid` - `state` (optional) - Return `all` issues or just those that are `opened` or `closed` - `labels` (optional) - Comma-separated list of label names - `milestone` (optional) - Milestone title diff --git a/doc/api/merge_requests.md b/doc/api/merge_requests.md index 6a272539e45..c1d82ad9576 100644 --- a/doc/api/merge_requests.md +++ b/doc/api/merge_requests.md @@ -10,11 +10,13 @@ The pagination parameters `page` and `per_page` can be used to restrict the list GET /projects/:id/merge_requests GET /projects/:id/merge_requests?state=opened GET /projects/:id/merge_requests?state=all +GET /projects/:id/merge_requests?iid=42 ``` Parameters: - `id` (required) - The ID of a project +- `iid` (optional) - Return the request having the given `iid` - `state` (optional) - Return `all` requests or just those that are `merged`, `opened` or `closed` - `order_by` (optional) - Return requests ordered by `created_at` or `updated_at` fields. Default is `created_at` - `sort` (optional) - Return requests sorted in `asc` or `desc` order. Default is `desc` @@ -388,6 +390,6 @@ Parameters: ] ``` -## Comments on issues +## Comments on merge requets Comments are done via the notes resource. diff --git a/doc/api/milestones.md b/doc/api/milestones.md index d48b3bcce8a..a6828728264 100644 --- a/doc/api/milestones.md +++ b/doc/api/milestones.md @@ -6,6 +6,7 @@ Returns a list of project milestones. ``` GET /projects/:id/milestones +GET /projects/:id/milestones?iid=42 ``` ```json @@ -27,6 +28,7 @@ GET /projects/:id/milestones Parameters: - `id` (required) - The ID of a project +- `iid` (optional) - Return the milestone having the given `iid` ## Get single milestone diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index be133a2920b..85e9081680d 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -173,6 +173,10 @@ module API end end + def filter_by_iid(items, iid) + items.where(iid: iid) + end + # error helpers def forbidden!(reason = nil) diff --git a/lib/api/issues.rb b/lib/api/issues.rb index ff062be6040..c8db93eb778 100644 --- a/lib/api/issues.rb +++ b/lib/api/issues.rb @@ -51,6 +51,7 @@ module API # # Parameters: # id (required) - The ID of a project + # iid (optional) - Return the project issue having the given `iid` # state (optional) - Return "opened" or "closed" issues # labels (optional) - Comma-separated list of label names # milestone (optional) - Milestone title @@ -66,10 +67,12 @@ module API # GET /projects/:id/issues?labels=foo,bar&state=opened # GET /projects/:id/issues?milestone=1.0.0 # GET /projects/:id/issues?milestone=1.0.0&state=closed + # GET /issues?iid=42 get ":id/issues" do issues = user_project.issues issues = filter_issues_state(issues, params[:state]) unless params[:state].nil? issues = filter_issues_labels(issues, params[:labels]) unless params[:labels].nil? + issues = filter_by_iid(issues, params[:iid]) unless params[:iid].nil? unless params[:milestone].nil? issues = filter_issues_milestone(issues, params[:milestone]) diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index b252c57faed..2216a12a87a 100644 --- a/lib/api/merge_requests.rb +++ b/lib/api/merge_requests.rb @@ -24,6 +24,7 @@ module API # # Parameters: # id (required) - The ID of a project + # iid (optional) - Return the project MR having the given `iid` # state (optional) - Return requests "merged", "opened" or "closed" # order_by (optional) - Return requests ordered by `created_at` or `updated_at` fields. Default is `created_at` # sort (optional) - Return requests sorted in `asc` or `desc` order. Default is `desc` @@ -36,11 +37,16 @@ module API # GET /projects/:id/merge_requests?order_by=updated_at # GET /projects/:id/merge_requests?sort=desc # GET /projects/:id/merge_requests?sort=asc + # GET /projects/:id/merge_requests?iid=42 # get ":id/merge_requests" do authorize! :read_merge_request, user_project merge_requests = user_project.merge_requests + unless params[:iid].nil? + merge_requests = filter_by_iid(merge_requests, params[:iid]) + end + merge_requests = case params["state"] when "opened" then merge_requests.opened @@ -169,8 +175,8 @@ module API # Merge MR # # Parameters: - # id (required) - The ID of a project - # merge_request_id (required) - ID of MR + # id (required) - The ID of a project + # merge_request_id (required) - ID of MR # merge_commit_message (optional) - Custom merge commit message # Example: # PUT /projects/:id/merge_request/:merge_request_id/merge @@ -209,7 +215,7 @@ module API # Get a merge request's comments # # Parameters: - # id (required) - The ID of a project + # id (required) - The ID of a project # merge_request_id (required) - ID of MR # Examples: # GET /projects/:id/merge_request/:merge_request_id/comments @@ -225,9 +231,9 @@ module API # Post comment to merge request # # Parameters: - # id (required) - The ID of a project + # id (required) - The ID of a project # merge_request_id (required) - ID of MR - # note (required) - Text of comment + # note (required) - Text of comment # Examples: # POST /projects/:id/merge_request/:merge_request_id/comments # diff --git a/spec/requests/api/issues_spec.rb b/spec/requests/api/issues_spec.rb index b6b0427debf..8770786f49a 100644 --- a/spec/requests/api/issues_spec.rb +++ b/spec/requests/api/issues_spec.rb @@ -194,6 +194,14 @@ describe API::API, api: true do expect(json_response['iid']).to eq(issue.iid) end + it 'should return a project issue by iid' do + get api("/projects/#{project.id}/issues?iid=#{issue.iid}", user) + response.status.should == 200 + json_response.first['title'].should == issue.title + json_response.first['id'].should == issue.id + json_response.first['iid'].should == issue.iid + end + it "should return 404 if issue id not found" do get api("/projects/#{project.id}/issues/54321", user) expect(response.status).to eq(404) diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb index 5fca831f9be..dcd50f73326 100644 --- a/spec/requests/api/merge_requests_spec.rb +++ b/spec/requests/api/merge_requests_spec.rb @@ -115,6 +115,14 @@ describe API::API, api: true do expect(json_response['iid']).to eq(merge_request.iid) end + it 'should return merge_request by iid' do + url = "/projects/#{project.id}/merge_requests?iid=#{merge_request.iid}" + get api(url, user) + response.status.should == 200 + json_response.first['title'].should == merge_request.title + json_response.first['id'].should == merge_request.id + end + it "should return a 404 error if merge_request_id not found" do get api("/projects/#{project.id}/merge_request/999", user) expect(response.status).to eq(404) diff --git a/spec/requests/api/milestones_spec.rb b/spec/requests/api/milestones_spec.rb index effb0723476..6890dd1f3a7 100644 --- a/spec/requests/api/milestones_spec.rb +++ b/spec/requests/api/milestones_spec.rb @@ -30,6 +30,13 @@ describe API::API, api: true do expect(json_response['iid']).to eq(milestone.iid) end + it 'should return a project milestone by iid' do + get api("/projects/#{project.id}/milestones?iid=#{milestone.iid}", user) + response.status.should == 200 + json_response.first['title'].should == milestone.title + json_response.first['id'].should == milestone.id + end + it 'should return 401 error if user not authenticated' do get api("/projects/#{project.id}/milestones/#{milestone.id}") expect(response.status).to eq(401) -- cgit v1.2.1 From eb4f1eb5f55fe3630c9191db1b9da2dc92437391 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Sat, 2 May 2015 06:53:32 -0700 Subject: Add application setting to restrict user signups to e-mail domains This feature was requested long ago: http://feedback.gitlab.com/forums/176466-general/suggestions/4118466-ability-to-register-only-from-ceratain-domains This MR is based off !253 but changed to use application settings and use wildcard strings to give more flexibility in pattern matching. Regexps seemed overkill and easy to get wrong. Only restrict e-mail addresses upon creation --- CHANGELOG | 1 + .../admin/application_settings_controller.rb | 3 +- app/models/application_setting.rb | 24 ++++++++++- app/models/user.rb | 24 +++++++++++ .../admin/application_settings/_form.html.haml | 5 +++ config/initializers/1_settings.rb | 1 + ...icted_signup_domains_to_application_settings.rb | 5 +++ db/schema.rb | 3 +- spec/models/application_setting_spec.rb | 24 +++++++++++ spec/models/user_spec.rb | 47 ++++++++++++++++++++++ 10 files changed, 133 insertions(+), 4 deletions(-) create mode 100644 db/migrate/20150502064022_add_restricted_signup_domains_to_application_settings.rb diff --git a/CHANGELOG b/CHANGELOG index 2c5c9c76ccf..dc3a6709222 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,6 +1,7 @@ Please view this file on the master branch, on stable branches it's out of date. v 7.11.0 (unreleased) + - Add application setting to restrict user signups to e-mail domains (Stan Hu) - Don't allow a merge request to be merged when its title starts with "WIP". - Add a page title to every page. - Get Gitorious importer to work again. diff --git a/app/controllers/admin/application_settings_controller.rb b/app/controllers/admin/application_settings_controller.rb index 8f6a766635a..3975e30835e 100644 --- a/app/controllers/admin/application_settings_controller.rb +++ b/app/controllers/admin/application_settings_controller.rb @@ -41,7 +41,8 @@ class Admin::ApplicationSettingsController < Admin::ApplicationController :max_attachment_size, :default_project_visibility, :default_snippet_visibility, - restricted_visibility_levels: [] + :restricted_signup_domains_raw, + restricted_visibility_levels: [], ) end end diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index 9406fb91939..f2cebde9705 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -18,11 +18,13 @@ # restricted_visibility_levels :text # max_attachment_size :integer default(10) # default_project_visibility :integer -# default_snippet_visibility :integer +# restricted_signup_domains :text # class ApplicationSetting < ActiveRecord::Base serialize :restricted_visibility_levels + serialize :restricted_signup_domains, Array + attr_accessor :restricted_signup_domains_raw validates :home_page_url, allow_blank: true, @@ -55,11 +57,29 @@ class ApplicationSetting < ActiveRecord::Base restricted_visibility_levels: Settings.gitlab['restricted_visibility_levels'], max_attachment_size: Settings.gitlab['max_attachment_size'], default_project_visibility: Settings.gitlab.default_projects_features['visibility_level'], - default_snippet_visibility: Settings.gitlab.default_projects_features['visibility_level'] + default_snippet_visibility: Settings.gitlab.default_projects_features['visibility_level'], + restricted_signup_domains: Settings.gitlab['restricted_signup_domains'] ) end def home_page_url_column_exist ActiveRecord::Base.connection.column_exists?(:application_settings, :home_page_url) end + + def restricted_signup_domains_raw + self.restricted_signup_domains.join("\n") unless self.restricted_signup_domains.nil? + end + + def restricted_signup_domains_raw=(values) + self.restricted_signup_domains = [] + self.restricted_signup_domains = values.split( + /\s*[,;]\s* # comma or semicolon, optionally surrounded by whitespace + | # or + \s # any whitespace character + | # or + [\r\n] # any number of newline characters + /x) + self.restricted_signup_domains.reject! { |d| d.empty? } + end + end diff --git a/app/models/user.rb b/app/models/user.rb index d6b93afe739..f22fdc28435 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -142,6 +142,7 @@ class User < ActiveRecord::Base validates :avatar, file_size: { maximum: 200.kilobytes.to_i } before_validation :generate_password, on: :create + before_validation :restricted_signup_domains, on: :create before_validation :sanitize_attrs before_validation :set_notification_email, if: ->(user) { user.email_changed? } before_validation :set_public_email, if: ->(user) { user.public_email_changed? } @@ -611,4 +612,27 @@ class User < ActiveRecord::Base select(:project_id). uniq.map(&:project_id) end + + def restricted_signup_domains + email_domains = current_application_settings.restricted_signup_domains + + unless email_domains.blank? + match_found = email_domains.any? do |domain| + escaped = Regexp.escape(domain).gsub('\*','.*?') + regexp = Regexp.new "^#{escaped}$", Regexp::IGNORECASE + email_domain = Mail::Address.new(self.email).domain + email_domain =~ regexp + end + + unless match_found + self.errors.add :email, + 'is not whitelisted. ' + + 'Email domains valid for registration are: ' + + email_domains.join(', ') + return false + end + end + + true + end end diff --git a/app/views/admin/application_settings/_form.html.haml b/app/views/admin/application_settings/_form.html.haml index 87e7c9634e9..f6eb00ea0bd 100644 --- a/app/views/admin/application_settings/_form.html.haml +++ b/app/views/admin/application_settings/_form.html.haml @@ -72,6 +72,11 @@ = f.label :max_attachment_size, 'Maximum attachment size (MB)', class: 'control-label col-sm-2' .col-sm-10 = f.number_field :max_attachment_size, class: 'form-control' + .form-group + = f.label :restricted_signup_domains, 'Restricted domains for sign-ups', class: 'control-label col-sm-2' + .col-sm-10 + = f.text_area :restricted_signup_domains_raw, placeholder: 'domain.com', class: 'form-control' + .help-block Ex: domain.com, *.domain.com. Wildcards allowed. Use separate lines for multiple entries. .form-actions = f.submit 'Save', class: 'btn btn-primary' diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb index 0abd34fc3e0..e5ac66a2323 100644 --- a/config/initializers/1_settings.rb +++ b/config/initializers/1_settings.rb @@ -132,6 +132,7 @@ Settings.gitlab.default_projects_features['wiki'] = true if Settings.g Settings.gitlab.default_projects_features['snippets'] = false if Settings.gitlab.default_projects_features['snippets'].nil? Settings.gitlab.default_projects_features['visibility_level'] = Settings.send(:verify_constant, Gitlab::VisibilityLevel, Settings.gitlab.default_projects_features['visibility_level'], Gitlab::VisibilityLevel::PRIVATE) Settings.gitlab['repository_downloads_path'] = File.absolute_path(Settings.gitlab['repository_downloads_path'] || 'tmp/repositories', Rails.root) +Settings.gitlab['restricted_signup_domains'] ||= [] # # Gravatar diff --git a/db/migrate/20150502064022_add_restricted_signup_domains_to_application_settings.rb b/db/migrate/20150502064022_add_restricted_signup_domains_to_application_settings.rb new file mode 100644 index 00000000000..184e2653610 --- /dev/null +++ b/db/migrate/20150502064022_add_restricted_signup_domains_to_application_settings.rb @@ -0,0 +1,5 @@ +class AddRestrictedSignupDomainsToApplicationSettings < ActiveRecord::Migration + def change + add_column :application_settings, :restricted_signup_domains, :text + end +end diff --git a/db/schema.rb b/db/schema.rb index 02882186fc8..f0cccb8b749 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: 20150429002313) do +ActiveRecord::Schema.define(version: 20150502064022) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -31,6 +31,7 @@ ActiveRecord::Schema.define(version: 20150429002313) do t.integer "max_attachment_size", default: 10, null: false t.integer "default_project_visibility" t.integer "default_snippet_visibility" + t.text "restricted_signup_domains" end create_table "broadcast_messages", force: true do |t| diff --git a/spec/models/application_setting_spec.rb b/spec/models/application_setting_spec.rb index b4f0b2c201a..876502d03c2 100644 --- a/spec/models/application_setting_spec.rb +++ b/spec/models/application_setting_spec.rb @@ -21,4 +21,28 @@ require 'spec_helper' describe ApplicationSetting, models: true do it { expect(ApplicationSetting.create_from_defaults).to be_valid } + + context 'restricted signup domains' do + let(:setting) { ApplicationSetting.create_from_defaults } + + it 'set single domain' do + setting.restricted_signup_domains_raw = 'example.com' + expect(setting.restricted_signup_domains).to eq(['example.com']) + end + + it 'set multiple domains with spaces' do + setting.restricted_signup_domains_raw = 'example.com *.example.com' + expect(setting.restricted_signup_domains).to eq(['example.com', '*.example.com']) + end + + it 'set multiple domains with newlines and a space' do + setting.restricted_signup_domains_raw = "example.com\n *.example.com" + expect(setting.restricted_signup_domains).to eq(['example.com', '*.example.com']) + end + + it 'set multiple domains with commas' do + setting.restricted_signup_domains_raw = "example.com, *.example.com" + expect(setting.restricted_signup_domains).to eq(['example.com', '*.example.com']) + end + end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 24384e8bf22..441aa793133 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -54,6 +54,8 @@ require 'spec_helper' describe User do + include Gitlab::CurrentSettings + describe "Associations" do it { is_expected.to have_one(:namespace) } it { is_expected.to have_many(:snippets).class_name('Snippet').dependent(:destroy) } @@ -112,6 +114,51 @@ describe User do user = build(:user, email: "lol!'+=?><#$%^&*()@gmail.com") expect(user).to be_invalid end + + context 'when no signup domains listed' do + before { allow(current_application_settings).to receive(:restricted_signup_domains).and_return([]) } + it 'accepts any email' do + user = build(:user, email: "info@example.com") + expect(user).to be_valid + end + end + + context 'when a signup domain is listed and subdomains are allowed' do + before { allow(current_application_settings).to receive(:restricted_signup_domains).and_return(['example.com', '*.example.com']) } + it 'accepts info@example.com' do + user = build(:user, email: "info@example.com") + expect(user).to be_valid + end + + it 'accepts info@test.example.com' do + user = build(:user, email: "info@test.example.com") + expect(user).to be_valid + end + + it 'rejects example@test.com' do + user = build(:user, email: "example@test.com") + expect(user).to be_invalid + end + end + + context 'when a signup domain is listed and subdomains are not allowed' do + before { allow(current_application_settings).to receive(:restricted_signup_domains).and_return(['example.com']) } + + it 'accepts info@example.com' do + user = build(:user, email: "info@example.com") + expect(user).to be_valid + end + + it 'rejects info@test.example.com' do + user = build(:user, email: "info@test.example.com") + expect(user).to be_invalid + end + + it 'rejects example@test.com' do + user = build(:user, email: "example@test.com") + expect(user).to be_invalid + end + end end end -- cgit v1.2.1 From 0ae574007d2118fc6e291591121ceca2da6fc22e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Onur=20K=C3=BC=C3=A7=C3=BCk?= Date: Sun, 3 May 2015 00:43:46 +0300 Subject: add common method to force utf8 and force oauth properties to be utf8 --- CHANGELOG | 1 + lib/gitlab/o_auth/auth_hash.rb | 19 ++++-- lib/gitlab/utils.rb | 4 ++ spec/lib/gitlab/o_auth/auth_hash_spec.rb | 111 +++++++++++++++++++++++-------- 4 files changed, 100 insertions(+), 35 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 3af83ddc256..a58a5cca969 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -27,6 +27,7 @@ v 7.11.0 (unreleased) - Move snippets UI to fluid layout - Improve UI for sidebar. Increase separation between navigation and content - Improve new project command options (Ben Bodenmiller) + - Add common method to force UTF-8 and use it to properly handle non-ascii OAuth user properties (Onur Küçük) - Prevent sending empty messages to HipChat (Chulki Lee) - Improve UI for mobile phones on dashboard and project pages - Add room notification and message color option for HipChat diff --git a/lib/gitlab/o_auth/auth_hash.rb b/lib/gitlab/o_auth/auth_hash.rb index ce52beec78e..0f16c925900 100644 --- a/lib/gitlab/o_auth/auth_hash.rb +++ b/lib/gitlab/o_auth/auth_hash.rb @@ -9,11 +9,11 @@ module Gitlab end def uid - auth_hash.uid.to_s + Gitlab::Utils.force_utf8(auth_hash.uid.to_s) end def provider - auth_hash.provider + Gitlab::Utils.force_utf8(auth_hash.provider.to_s) end def info @@ -21,23 +21,28 @@ module Gitlab end def name - (info.try(:name) || full_name).to_s.force_encoding('utf-8') + Gitlab::Utils.force_utf8((info.try(:name) || full_name).to_s) end def full_name - "#{info.first_name} #{info.last_name}" + Gitlab::Utils.force_utf8("#{info.first_name} #{info.last_name}") end def username - (info.try(:nickname) || generate_username).to_s.force_encoding('utf-8') + Gitlab::Utils.force_utf8( + (info.try(:nickname) || generate_username).to_s + ) end def email - (info.try(:email) || generate_temporarily_email).downcase + Gitlab::Utils.force_utf8( + (info.try(:email) || generate_temporarily_email).downcase + ) end def password - @password ||= Devise.friendly_token[0, 8].downcase + devise_friendly_token = Devise.friendly_token[0, 8].downcase + @password ||= Gitlab::Utils.force_utf8(devise_friendly_token) end # Get the first part of the email address (before @) diff --git a/lib/gitlab/utils.rb b/lib/gitlab/utils.rb index bd184c27187..d13fe0ef8a9 100644 --- a/lib/gitlab/utils.rb +++ b/lib/gitlab/utils.rb @@ -9,5 +9,9 @@ module Gitlab def system_silent(cmd) Popen::popen(cmd).last.zero? end + + def force_utf8(str) + str.force_encoding(Encoding::UTF_8) + end end end diff --git a/spec/lib/gitlab/o_auth/auth_hash_spec.rb b/spec/lib/gitlab/o_auth/auth_hash_spec.rb index 5eb77b492b2..678086ffa14 100644 --- a/spec/lib/gitlab/o_auth/auth_hash_spec.rb +++ b/spec/lib/gitlab/o_auth/auth_hash_spec.rb @@ -2,54 +2,109 @@ require 'spec_helper' describe Gitlab::OAuth::AuthHash do let(:auth_hash) do - Gitlab::OAuth::AuthHash.new(double({ - provider: 'twitter', - uid: uid, - info: double(info_hash) - })) + Gitlab::OAuth::AuthHash.new( + double({ + provider: provider_ascii, + uid: uid_ascii, + info: double(info_hash) + }) + ) end - let(:uid) { 'my-uid' } - let(:email) { 'my-email@example.com' } - let(:nickname) { 'my-nickname' } + + let(:uid_raw) { + "CN=Onur K\xC3\xBC\xC3\xA7\xC3\xBCk,OU=Test,DC=example,DC=net" + } + let(:email_raw) { "onur.k\xC3\xBC\xC3\xA7\xC3\xBCk@example.net" } + let(:nickname_raw) { "ok\xC3\xBC\xC3\xA7\xC3\xBCk" } + let(:first_name_raw) { 'Onur' } + let(:last_name_raw) { "K\xC3\xBC\xC3\xA7\xC3\xBCk" } + let(:name_raw) { "Onur K\xC3\xBC\xC3\xA7\xC3\xBCk" } + + let(:provider_ascii) { 'ldap'.force_encoding(Encoding::ASCII_8BIT) } + let(:uid_ascii) { uid_raw.force_encoding(Encoding::ASCII_8BIT) } + let(:email_ascii) { email_raw.force_encoding(Encoding::ASCII_8BIT) } + let(:nickname_ascii) { nickname_raw.force_encoding(Encoding::ASCII_8BIT) } + let(:first_name_ascii) { first_name_raw.force_encoding(Encoding::ASCII_8BIT) } + let(:last_name_ascii) { last_name_raw.force_encoding(Encoding::ASCII_8BIT) } + let(:name_ascii) { name_raw.force_encoding(Encoding::ASCII_8BIT) } + + let(:provider_utf8) { provider_ascii.force_encoding(Encoding::UTF_8) } + let(:uid_utf8) { uid_ascii.force_encoding(Encoding::UTF_8) } + let(:email_utf8) { email_ascii.force_encoding(Encoding::UTF_8) } + let(:nickname_utf8) { nickname_ascii.force_encoding(Encoding::UTF_8) } + let(:name_utf8) { name_ascii.force_encoding(Encoding::UTF_8) } + let(:info_hash) { { - email: email, - nickname: nickname, - name: 'John', - first_name: "John", - last_name: "Who" + email: email_ascii, + first_name: first_name_ascii, + last_name: last_name_ascii, + name: name_ascii, + nickname: nickname_ascii, + uid: uid_ascii } } - context "defaults" do - it { expect(auth_hash.provider).to eql 'twitter' } - it { expect(auth_hash.uid).to eql uid } - it { expect(auth_hash.email).to eql email } - it { expect(auth_hash.username).to eql nickname } - it { expect(auth_hash.name).to eql "John" } + context 'defaults' do + it { expect(auth_hash.provider).to eql provider_utf8 } + it { expect(auth_hash.uid).to eql uid_utf8 } + it { expect(auth_hash.email).to eql email_utf8 } + it { expect(auth_hash.username).to eql nickname_utf8 } + it { expect(auth_hash.name).to eql name_utf8 } it { expect(auth_hash.password).to_not be_empty } end - context "email not provided" do + context 'email not provided' do before { info_hash.delete(:email) } - it "generates a temp email" do + + it 'generates a temp email' do expect( auth_hash.email).to start_with('temp-email-for-oauth') end end - context "username not provided" do + context 'username not provided' do before { info_hash.delete(:nickname) } - it "takes the first part of the email as username" do - expect( auth_hash.username ).to eql "my-email" + it 'takes the first part of the email as username' do + expect(auth_hash.username).to eql 'onur-kucuk' end end - context "name not provided" do + context 'name not provided' do before { info_hash.delete(:name) } - it "concats first and lastname as the name" do - expect( auth_hash.name ).to eql "John Who" + it 'concats first and lastname as the name' do + expect(auth_hash.name).to eql name_utf8 + end + end + + context 'auth_hash constructed with ASCII-8BIT encoding' do + it 'forces utf8 encoding on uid' do + auth_hash.uid.encoding.should eql Encoding::UTF_8 + end + + it 'forces utf8 encoding on provider' do + auth_hash.provider.encoding.should eql Encoding::UTF_8 + end + + it 'forces utf8 encoding on name' do + auth_hash.name.encoding.should eql Encoding::UTF_8 + end + + it 'forces utf8 encoding on full_name' do + auth_hash.full_name.encoding.should eql Encoding::UTF_8 + end + + it 'forces utf8 encoding on username' do + auth_hash.username.encoding.should eql Encoding::UTF_8 + end + + it 'forces utf8 encoding on email' do + auth_hash.email.encoding.should eql Encoding::UTF_8 + end + + it 'forces utf8 encoding on password' do + auth_hash.password.encoding.should eql Encoding::UTF_8 end end -end \ No newline at end of file +end -- cgit v1.2.1