From 44261a5d9fd5b78f8a44fe330e2386525f4c3437 Mon Sep 17 00:00:00 2001
From: Valery Sizov <vsv2711@gmail.com>
Date: Wed, 9 Sep 2015 17:36:01 +0300
Subject: [PATCH] integration with gitlab auth
---
app/controllers/ci/application_controller.rb | 61 +++++--------------
app/controllers/ci/builds_controller.rb | 2 +-
app/controllers/ci/commits_controller.rb | 2 +-
app/controllers/ci/projects_controller.rb | 11 ++--
.../ci/user_sessions_controller.rb | 10 ---
app/helpers/ci/commits_helper.rb | 2 +-
app/models/ability.rb | 1 +
app/models/ci/user.rb | 32 +---------
app/views/ci/builds/_build.html.haml | 2 +-
app/views/ci/builds/show.html.haml | 12 ++--
app/views/ci/commits/_commit.html.haml | 2 +-
app/views/ci/commits/show.html.haml | 4 +-
app/views/ci/projects/_gl_projects.html.haml | 2 +-
app/views/ci/projects/gitlab.html.haml | 4 --
app/views/ci/projects/show.html.haml | 2 +-
app/views/ci/user_sessions/show.html.haml | 2 +-
app/views/layouts/ci/_info.html.haml | 2 +-
app/views/layouts/ci/_nav.html.haml | 6 +-
app/views/layouts/ci/project.html.haml | 2 +-
lib/ci/api/projects.rb | 10 +--
spec/models/ci/user_spec.rb | 50 ---------------
21 files changed, 51 insertions(+), 170 deletions(-)
diff --git a/app/controllers/ci/application_controller.rb b/app/controllers/ci/application_controller.rb
index 95390d09737..e5c99066a68 100644
--- a/app/controllers/ci/application_controller.rb
+++ b/app/controllers/ci/application_controller.rb
@@ -1,5 +1,5 @@
module Ci
- class ApplicationController < ActionController::Base
+ class ApplicationController < ::ApplicationController
def self.railtie_helpers_paths
"app/helpers/ci"
end
@@ -9,49 +9,19 @@ module Ci
rescue_from Ci::Network::UnauthorizedError, with: :invalid_token
before_filter :default_headers
#before_filter :check_config
+ helper_method :gl_project
protect_from_forgery
- helper_method :current_user
- before_filter :reset_cache
-
private
- def current_user
- @current_user ||= session[:ci_current_user]
- end
-
- def sign_in(user)
- session[:ci_current_user] = user
- end
-
- def sign_out
- reset_session
- end
-
- def authenticate_user!
- unless current_user
- redirect_to new_ci_user_sessions_path
- return
- end
- end
-
- def authenticate_admin!
- unless current_user && current_user.is_admin
- redirect_to new_ci_user_sessions_path
- return
- end
- end
-
def authenticate_public_page!
unless project.public
unless current_user
- redirect_to(new_ci_user_sessions_path(state: generate_oauth_state(request.fullpath))) and return
+ redirect_to(new_user_sessions_path) and return
end
- unless current_user.can_access_project?(project.gitlab_id)
- page_404 and return
- end
+ return access_denied! unless can?(current_user, :read_project, gl_project)
end
end
@@ -62,19 +32,23 @@ module Ci
end
def authorize_access_project!
- unless current_user.can_access_project?(@project.gitlab_id)
+ unless can?(current_user, :read_project, gl_project)
return page_404
end
end
- def authorize_project_developer!
- unless current_user.has_developer_access?(@project.gitlab_id)
+ def authorize_manage_builds!
+ unless can?(current_user, :manage_builds, gl_project)
return page_404
end
end
+ def authenticate_admin!
+ return render_404 unless current_user.is_admin?
+ end
+
def authorize_manage_project!
- unless current_user.can_manage_project?(@project.gitlab_id)
+ unless can?(current_user, :manage_project, gl_project)
return page_404
end
end
@@ -83,13 +57,6 @@ module Ci
render file: "#{Rails.root}/public/404.html", status: 404, layout: false
end
- # Reset user cache every day for security purposes
- def reset_cache
- if current_user && current_user.sync_at < (Time.zone.now - 24.hours)
- current_user.reset_cache
- end
- end
-
def default_headers
headers['X-Frame-Options'] = 'DENY'
headers['X-XSS-Protection'] = '1; mode=block'
@@ -129,5 +96,9 @@ module Ci
reset_session
redirect_to ci_root_path
end
+
+ def gl_project
+ ::Project.find(@project.gitlab_id)
+ end
end
end
diff --git a/app/controllers/ci/builds_controller.rb b/app/controllers/ci/builds_controller.rb
index eeff3f1e0a0..28fad3671f7 100644
--- a/app/controllers/ci/builds_controller.rb
+++ b/app/controllers/ci/builds_controller.rb
@@ -5,7 +5,7 @@ module Ci
before_filter :project
before_filter :authorize_access_project!, except: [:status, :show]
before_filter :authorize_manage_project!, except: [:status, :show, :retry, :cancel]
- before_filter :authorize_project_developer!, only: [:retry, :cancel]
+ before_filter :authorize_manage_builds!, only: [:retry, :cancel]
before_filter :build, except: [:show]
def show
diff --git a/app/controllers/ci/commits_controller.rb b/app/controllers/ci/commits_controller.rb
index 9f74a2fd807..bad9075dde6 100644
--- a/app/controllers/ci/commits_controller.rb
+++ b/app/controllers/ci/commits_controller.rb
@@ -4,7 +4,7 @@ module Ci
before_filter :authenticate_public_page!, only: :show
before_filter :project
before_filter :authorize_access_project!, except: [:status, :show, :cancel]
- before_filter :authorize_project_developer!, only: [:cancel]
+ before_filter :authorize_manage_builds!, only: [:cancel]
before_filter :commit, only: :show
def show
diff --git a/app/controllers/ci/projects_controller.rb b/app/controllers/ci/projects_controller.rb
index 6ff7fc9f77a..80a5e602171 100644
--- a/app/controllers/ci/projects_controller.rb
+++ b/app/controllers/ci/projects_controller.rb
@@ -21,12 +21,15 @@ module Ci
@limit, @offset = (params[:limit] || PROJECTS_BATCH).to_i, (params[:offset] || 0).to_i
@page = @offset == 0 ? 1 : (@offset / @limit + 1)
- current_user.reset_cache if params[:reset_cache]
+ @gl_projects = current_user.authorized_projects
+ @gl_projects = @gl_projects.where("name LIKE %?%", params[:search]) if params[:search]
+ @gl_projects = @gl_projects.page(@page).per(@limit)
- @gl_projects = current_user.gitlab_projects(params[:search], @page, @limit)
@projects = Ci::Project.where(gitlab_id: @gl_projects.map(&:id)).ordered_by_last_commit_date
@total_count = @gl_projects.size
- @gl_projects.reject! { |gl_project| @projects.map(&:gitlab_id).include?(gl_project.id) }
+
+ @gl_projects = @gl_projects.where.not(id: @projects.map(&:gitlab_id))
+
respond_to do |format|
format.json do
pager_json("ci/projects/gitlab", @total_count)
@@ -52,7 +55,7 @@ module Ci
def create
project_data = OpenStruct.new(JSON.parse(params["project"]))
- unless current_user.can_manage_project?(project_data.id)
+ unless can?(current_user, :manage_project, ::Project.find(project_data.id))
return redirect_to ci_root_path, alert: 'You have to have at least master role to enable CI for this project'
end
diff --git a/app/controllers/ci/user_sessions_controller.rb b/app/controllers/ci/user_sessions_controller.rb
index 82134c1f7ba..818e1fcdea1 100644
--- a/app/controllers/ci/user_sessions_controller.rb
+++ b/app/controllers/ci/user_sessions_controller.rb
@@ -10,11 +10,6 @@ module Ci
end
def auth
- unless is_oauth_state_valid?(params[:state])
- redirect_to new_ci_user_sessions_path
- return
- end
-
redirect_to client.auth_code.authorize_url({
redirect_uri: callback_ci_user_sessions_url,
state: params[:state]
@@ -22,11 +17,6 @@ module Ci
end
def callback
- unless is_oauth_state_valid?(params[:state])
- redirect_to new_ci_user_sessions_path
- return
- end
-
token = client.auth_code.get_token(params[:code], redirect_uri: callback_ci_user_sessions_url).token
@user_session = Ci::UserSession.new
diff --git a/app/helpers/ci/commits_helper.rb b/app/helpers/ci/commits_helper.rb
index 0479bc10594..748d12138b1 100644
--- a/app/helpers/ci/commits_helper.rb
+++ b/app/helpers/ci/commits_helper.rb
@@ -16,7 +16,7 @@ module Ci
end
def commit_link(commit)
- link_to(commit.short_sha, ci_project_ref_commit_path(commit.project, commit.ref, commit.sha))
+ link_to(commit.short_sha, ci_project_ref_commits_path(commit.project, commit.ref, commit.sha))
end
def truncate_first_line(message, length = 50)
diff --git a/app/models/ability.rb b/app/models/ability.rb
index f8e5afa9b01..a020b24a550 100644
--- a/app/models/ability.rb
+++ b/app/models/ability.rb
@@ -149,6 +149,7 @@ class Ability
:admin_merge_request,
:create_merge_request,
:create_wiki,
+ :manage_builds,
:push_code
]
end
diff --git a/app/models/ci/user.rb b/app/models/ci/user.rb
index 49ec7126fc1..f7e2eaae1f1 100644
--- a/app/models/ci/user.rb
+++ b/app/models/ci/user.rb
@@ -39,43 +39,13 @@ module Ci
@sync_at = Time.now
end
- def can_access_project?(project_gitlab_id)
- !!project_info(project_gitlab_id)
- end
-
- # Indicate if user has developer access or higher
- def has_developer_access?(project_gitlab_id)
- data = project_info(project_gitlab_id)
-
- return false unless data && data["permissions"]
-
- permissions = data["permissions"]
-
- if permissions["project_access"] && permissions["project_access"]["access_level"] >= DEVELOPER_ACCESS
- return true
- end
-
- if permissions["group_access"] && permissions["group_access"]["access_level"] >= DEVELOPER_ACCESS
- return true
- end
- end
-
- def can_manage_project?(project_gitlab_id)
- Rails.cache.fetch(cache_key('manage', project_gitlab_id, sync_at)) do
- !!Ci::Network.new.project_hooks(authenticate_options, project_gitlab_id)
- end
- end
-
def authorized_runners
Ci::Runner.specific.includes(:runner_projects).
where(Ci::RunnerProject.table_name => { project_id: authorized_projects } )
end
def authorized_projects
- Ci::Project.where(gitlab_id: gitlab_projects.map(&:id)).select do |project|
- # This is slow: it makes request to GitLab for each project to verify manage permission
- can_manage_project?(project.gitlab_id)
- end
+ Ci::Project.where(gitlab_id: current_user.authorized_projects.map(&:id))
end
def authenticate_options
diff --git a/app/views/ci/builds/_build.html.haml b/app/views/ci/builds/_build.html.haml
index 54ca1102b5c..da306c9f020 100644
--- a/app/views/ci/builds/_build.html.haml
+++ b/app/views/ci/builds/_build.html.haml
@@ -35,7 +35,7 @@
#{build.coverage}%
%td
- - if defined?(controls) && current_user && current_user.has_developer_access?(@project.gitlab_id)
+ - if defined?(controls) && current_user && can?(current_user, :manage_builds, gl_project)
.pull-right
- if build.active?
= link_to cancel_ci_project_build_path(build.project, build, return_to: request.original_url), title: 'Cancel build' do
diff --git a/app/views/ci/builds/show.html.haml b/app/views/ci/builds/show.html.haml
index 0bef67d8a20..a698176c3ed 100644
--- a/app/views/ci/builds/show.html.haml
+++ b/app/views/ci/builds/show.html.haml
@@ -1,10 +1,10 @@
%h4.page-title
- = link_to @project.name, @project
+ = link_to ci_project_path(@project)
@
= @commit.short_sha
%p
- = link_to ci_project_ref_commit_path(@project, @commit.ref, @commit.sha) do
+ = link_to ci_project_ref_commits_path(@project, @commit.ref, @commit.sha) do
← Back to project commit
%hr
#up-build-trace
@@ -12,7 +12,7 @@
%ul.nav.nav-tabs.append-bottom-10
- @commit.builds_without_retry_sorted.each do |build|
%li{class: ('active' if build == @build) }
- = link_to ci_build_url(build) do
+ = link_to ci_project_build_url(@project, build) do
%i{class: build_icon_css_class(build)}
%span
Build ##{build.id}
@@ -84,7 +84,7 @@
.build-widget
%h4.title
Build
- - if current_user && current_user.has_developer_access?(@project.gitlab_id)
+ - if current_user && can?(current_user, :manage_builds, gl_project)
.pull-right
- if @build.active?
= link_to "Cancel", cancel_ci_project_build_path(@project, @build), class: 'btn btn-sm btn-danger'
@@ -161,7 +161,7 @@
- @builds.each_with_index do |build, i|
%tr.build.alert{class: build_status_alert_class(build)}
%td
- = link_to ci_build_url(build) do
+ = link_to ci_project_build_url(@project, build) do
%span ##{build.id}
%td
- if build.name
@@ -173,4 +173,4 @@
:javascript
- new CiBuild("#{ci_build_url(@build)}", "#{@build.status}")
+ new CiBuild("#{ci_project_build_url(@project, @build)}", "#{@build.status}")
diff --git a/app/views/ci/commits/_commit.html.haml b/app/views/ci/commits/_commit.html.haml
index a955a5b6479..c1b1988d147 100644
--- a/app/views/ci/commits/_commit.html.haml
+++ b/app/views/ci/commits/_commit.html.haml
@@ -7,7 +7,7 @@
%td.build-link
- = link_to ci_project_ref_commit_path(commit.project, commit.ref, commit.sha) do
+ = link_to ci_project_ref_commits_path(commit.project, commit.ref, commit.sha) do
%strong #{commit.short_sha}
%td.build-message
diff --git a/app/views/ci/commits/show.html.haml b/app/views/ci/commits/show.html.haml
index 832cc6a1bae..9b597b45aa5 100644
--- a/app/views/ci/commits/show.html.haml
+++ b/app/views/ci/commits/show.html.haml
@@ -33,10 +33,10 @@
%span.attr-name Created at:
#{@commit.created_at.to_s(:short)}
-- if current_user && current_user.has_developer_access?(@project.gitlab_id)
+- if current_user && can?(current_user, :manage_builds, gl_project)
.pull-right
- if @commit.builds.running_or_pending.any?
- = link_to "Cancel", cancel_ci_project_ref_commit_path(@project, @commit.ref, @commit.sha), class: 'btn btn-sm btn-danger'
+ = link_to "Cancel", cancel_ci_project_ref_commits_path(@project, @commit.ref, @commit.sha), class: 'btn btn-sm btn-danger'
- if @commit.yaml_errors.present?
diff --git a/app/views/ci/projects/_gl_projects.html.haml b/app/views/ci/projects/_gl_projects.html.haml
index 6ed19e13887..f63d8246e96 100644
--- a/app/views/ci/projects/_gl_projects.html.haml
+++ b/app/views/ci/projects/_gl_projects.html.haml
@@ -11,5 +11,5 @@
Added
- else
= form_tag ci_projects_path do
- = hidden_field_tag :project, project.to_h.to_json
+ = hidden_field_tag :project, project.to_json
= submit_tag 'Add project to CI', class: 'btn btn-default btn-sm'
diff --git a/app/views/ci/projects/gitlab.html.haml b/app/views/ci/projects/gitlab.html.haml
index bd55b1f12e7..0344676680e 100644
--- a/app/views/ci/projects/gitlab.html.haml
+++ b/app/views/ci/projects/gitlab.html.haml
@@ -4,10 +4,6 @@
Fetched from GitLab (#{link_to GitlabCi.config.gitlab_server.url, GitlabCi.config.gitlab_server.url, no_turbolink})
- if params[:search].present?
by keyword: "#{params[:search]}",
- #{time_ago_in_words(current_user.sync_at)} ago.
- = link_to gitlab_ci_projects_path(reset_cache: true, search: params[:search]), class: 'sync-now btn btn-sm btn-default reset-cache' do
- %i.fa.fa-refresh
- Sync now
%br
.pull-right
diff --git a/app/views/ci/projects/show.html.haml b/app/views/ci/projects/show.html.haml
index 27899591391..b79ab957ba8 100644
--- a/app/views/ci/projects/show.html.haml
+++ b/app/views/ci/projects/show.html.haml
@@ -1,6 +1,6 @@
= render 'ci/shared/guide' unless @project.setup_finished?
-- if current_user && current_user.can_manage_project?(@project.gitlab_id) && !@project.any_runners?
+- if current_user && can?(current_user, :manage_project, gl_project) && !@project.any_runners?
.alert.alert-danger
Builds for this project wont be served unless you configure runners on
= link_to "Runners page", ci_project_runners_path(@project)
diff --git a/app/views/ci/user_sessions/show.html.haml b/app/views/ci/user_sessions/show.html.haml
index 43f64a429b2..0710e205618 100644
--- a/app/views/ci/user_sessions/show.html.haml
+++ b/app/views/ci/user_sessions/show.html.haml
@@ -2,7 +2,7 @@
%h3
Hi, #{@user.name}
- - if @user.is_admin
+ - if @user.is_admin?
%span.label.label-success Admin
.profile-block
diff --git a/app/views/layouts/ci/_info.html.haml b/app/views/layouts/ci/_info.html.haml
index bce3ce77031..9bc23cfad05 100644
--- a/app/views/layouts/ci/_info.html.haml
+++ b/app/views/layouts/ci/_info.html.haml
@@ -5,5 +5,5 @@
- if notice
.alert.alert-info= notice
- - if current_user && current_user.is_admin && Ci::Runner.count.zero?
+ - if current_user && current_user.is_admin? && Ci::Runner.count.zero?
= render 'ci/shared/no_runners'
diff --git a/app/views/layouts/ci/_nav.html.haml b/app/views/layouts/ci/_nav.html.haml
index babd14ca2d3..4a0e45a0217 100644
--- a/app/views/layouts/ci/_nav.html.haml
+++ b/app/views/layouts/ci/_nav.html.haml
@@ -9,7 +9,7 @@
.collapse.navbar-collapse
%ul.nav.navbar-nav
- - if current_user && current_user.is_admin
+ - if current_user && current_user.is_admin?
%li
= link_to ci_admin_projects_path do
Admin
@@ -19,12 +19,12 @@
%ul.nav.navbar-nav.pull-right
- if current_user
%li
- = link_to ci_user_sessions_path do
+ = link_to new_user_session_path do
.profile-holder
= image_tag user_avatar_url(current_user, 64), class: 'avatar s32', alt: ''
%span= current_user.name
%li
- = link_to ci_user_sessions_path, class: "logout", method: :delete do
+ = link_to destroy_user_session_path, class: "logout", method: :delete do
%i.fa.fa-signout
Logout
- else
diff --git a/app/views/layouts/ci/project.html.haml b/app/views/layouts/ci/project.html.haml
index 763a7fc0b02..9549485d095 100644
--- a/app/views/layouts/ci/project.html.haml
+++ b/app/views/layouts/ci/project.html.haml
@@ -16,7 +16,7 @@
= link_to 'View on GitLab', @project.gitlab_url, no_turbolink.merge( class: 'btn btn-sm' )
%hr
.container
- - if current_user && current_user.can_manage_project?(@project.gitlab_id)
+ - if current_user && can?(current_user, :manage_project, gl_project)
.row
.col-md-2.append-bottom-20
= render 'layouts/ci/nav_project'
diff --git a/lib/ci/api/projects.rb b/lib/ci/api/projects.rb
index f9b4937c033..bdcacecf6ab 100644
--- a/lib/ci/api/projects.rb
+++ b/lib/ci/api/projects.rb
@@ -66,7 +66,7 @@ module Ci
get ":id" do
project = Ci::Project.find(params[:id])
- unauthorized! unless current_user.can_access_project?(project.gitlab_id)
+ unauthorized! unless can?(current_user, :read_project, gl_project)
present project, with: Entities::Project
end
@@ -118,7 +118,7 @@ module Ci
put ":id" do
project = Ci::Project.find(params[:id])
- unauthorized! unless current_user.can_manage_project?(project.gitlab_id)
+ unauthorized! unless can?(current_user, :manage_project, gl_project)
attrs = attributes_for_keys [:name, :gitlab_id, :path, :gitlab_url, :default_ref, :ssh_url_to_repo]
@@ -144,7 +144,7 @@ module Ci
delete ":id" do
project = Ci::Project.find(params[:id])
- unauthorized! unless current_user.can_manage_project?(project.gitlab_id)
+ unauthorized! unless can?(current_user, :manage_project, gl_project)
project.destroy
end
@@ -160,7 +160,7 @@ module Ci
project = Ci::Project.find(params[:id])
runner = Ci::Runner.find(params[:runner_id])
- unauthorized! unless current_user.can_manage_project?(project.gitlab_id)
+ unauthorized! unless can?(current_user, :manage_project, gl_project)
options = {
project_id: project.id,
@@ -188,7 +188,7 @@ module Ci
project = Ci::Project.find(params[:id])
runner = Ci::Runner.find(params[:runner_id])
- unauthorized! unless current_user.can_manage_project?(project.gitlab_id)
+ unauthorized! unless can?(current_user, :manage_project, gl_project)
options = {
project_id: project.id,
diff --git a/spec/models/ci/user_spec.rb b/spec/models/ci/user_spec.rb
index d1b87988b74..c4d7b3ccae5 100644
--- a/spec/models/ci/user_spec.rb
+++ b/spec/models/ci/user_spec.rb
@@ -2,56 +2,6 @@ require 'spec_helper'
describe Ci::User do
- describe "has_developer_access?" do
- before do
- @user = User.new({})
- end
-
- let(:project_with_owner_access) do
- {
- "name" => "gitlab-shell",
- "permissions" => {
- "project_access" => {
- "access_level"=> 10,
- "notification_level" => 3
- },
- "group_access" => {
- "access_level" => 50,
- "notification_level" => 3
- }
- }
- }
- end
-
- let(:project_with_reporter_access) do
- {
- "name" => "gitlab-shell",
- "permissions" => {
- "project_access" => {
- "access_level" => 20,
- "notification_level" => 3
- },
- "group_access" => {
- "access_level" => 10,
- "notification_level" => 3
- }
- }
- }
- end
-
- it "returns false for reporter" do
- @user.stub(:project_info).and_return(project_with_reporter_access)
-
- @user.has_developer_access?(1).should be_false
- end
-
- it "returns true for owner" do
- @user.stub(:project_info).and_return(project_with_owner_access)
-
- @user.has_developer_access?(1).should be_true
- end
- end
-
describe "authorized_projects" do
let (:user) { User.new({}) }
--
GitLab