Merge branch 'refactor/oauth' of /home/git/repositories/gitlab/gitlabhq

app/controllers/omniauth_callbacks_controller.rb

@@ -16,35 +16,41 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController
   end
 
   def ldap
-    # We only find ourselves here if the authentication to LDAP was successful.
-    @user = User.find_for_ldap_auth(request.env["omniauth.auth"], current_user)
-    if @user.persisted?
-      @user.remember_me = true
-    end
-    sign_in_and_redirect @user
+    # We only find ourselves here
+    # if the authentication to LDAP was successful.
+    @user = Gitlab::LDAP::User.find_or_create(oauth)
+    @user.remember_me = true if @user.persisted?
+    sign_in_and_redirect(@user)
   end
 
   private
 
   def handle_omniauth
-    oauth = request.env['omniauth.auth']
-    provider, uid = oauth['provider'], oauth['uid']
-
     if current_user
       # Change a logged-in user's authentication method:
-      current_user.extern_uid = uid
-      current_user.provider = provider
+      current_user.extern_uid = oauth['uid']
+      current_user.provider = oauth['provider']
       current_user.save
       redirect_to profile_path
     else
-      @user = User.find_or_new_for_omniauth(oauth)
+      @user = Gitlab::OAuth::User.find(oauth)
+
+      # Create user if does not exist
+      # and allow_single_sign_on is true
+      if Gitlab.config.omniauth['allow_single_sign_on']
+        @user ||= Gitlab::OAuth::User.create(oauth)
+      end
 
       if @user
-        sign_in_and_redirect @user
+        sign_in_and_redirect(@user)
       else
         flash[:notice] = "There's no such user!"
         redirect_to new_user_session_path
       end
     end
   end
+
+  def oauth
+    @oauth ||= request.env['omniauth.auth']
+  end
 end

app/controllers/registrations_controller.rb

@@ -2,9 +2,6 @@ class RegistrationsController < Devise::RegistrationsController
   before_filter :signup_enabled?
 
   def destroy
-    if current_user.owned_projects.count > 0
-      redirect_to account_profile_path, alert: "Remove projects and groups before removing account." and return
-    end
     current_user.destroy
 
     respond_to do |format|

app/helpers/profile_helper.rb

@@ -4,4 +4,16 @@ module ProfileHelper
       'active'
     end
   end
+
+  def show_profile_username_tab?
+    current_user.can_change_username?
+  end
+
+  def show_profile_social_tab?
+    Gitlab.config.omniauth.enabled && !current_user.ldap_user?
+  end
+
+  def show_profile_remove_tab?
+    Gitlab.config.gitlab.signup_enabled && !current_user.ldap_user?
+  end
 end

app/models/user.rb

@@ -159,6 +159,7 @@ class User < ActiveRecord::Base
   scope :not_in_team, ->(team){ where('users.id NOT IN (:ids)', ids: team.member_ids) }
   scope :not_in_project, ->(project) { project.users.present? ? where("id not in (:ids)", ids: project.users.map(&:id) ) : scoped }
   scope :without_projects, -> { where('id NOT IN (SELECT DISTINCT(user_id) FROM users_projects)') }
+  scope :ldap, -> { where(provider:  'ldap') }
 
   scope :potential_team_members, ->(team) { team.members.any? ? active.not_in_team(team) : active  }
 
@@ -186,22 +187,6 @@ class User < ActiveRecord::Base
       end
     end
 
-    def create_from_omniauth(auth, ldap = false)
-      gitlab_auth.create_from_omniauth(auth, ldap)
-    end
-
-    def find_or_new_for_omniauth(auth)
-      gitlab_auth.find_or_new_for_omniauth(auth)
-    end
-
-    def find_for_ldap_auth(auth, signed_in_resource = nil)
-      gitlab_auth.find_for_ldap_auth(auth, signed_in_resource)
-    end
-
-    def gitlab_auth
-      Gitlab::Auth.new
-    end
-
     def search query
       where("name LIKE :query OR email LIKE :query OR username LIKE :query", query: "%#{query}%")
     end

app/views/profiles/account.html.haml

@@ -5,47 +5,41 @@
   - if current_user.ldap_user?
     Some options are unavailable for LDAP accounts
 %hr
-- unless current_user.ldap_user?
-  - if Gitlab.config.omniauth.enabled
-    %fieldset
-      %legend Social Accounts
-      .oauth_select_holder
-        %p.hint Tip: Click on icon to activate signin with one of the following services
-        - enabled_social_providers.each do |provider|
-          %span{class: oauth_active_class(provider) }
-            = link_to authbutton(provider, 32), omniauth_authorize_path(User, provider)
 
 
-  %fieldset.update-password
-    %legend Password
-    = form_for @user, url: update_password_profile_path, method: :put do |f|
-      .padded
-        %p.slead After a successful password update you will be redirected to login page where you should login with your new password
-        -if @user.errors.any?
-          .alert.alert-error
-            %ul
-              - @user.errors.full_messages.each do |msg|
-                %li= msg
-
-        .control-group
-          = f.label :password
-          .controls= f.password_field :password, required: true
-        .control-group
-          = f.label :password_confirmation
-          .controls
-            = f.password_field :password_confirmation, required: true
-        .control-group
-          .controls
-            = f.submit 'Save password', class: "btn btn-save"
+.row
+  .span2
+    %ul.nav.nav-pills.nav-stacked.nav-stacked-menu
+      %li.active
+        = link_to '#tab-token', 'data-toggle' => 'tab' do
+          Private Token
+      %li
+        = link_to '#tab-password', 'data-toggle' => 'tab' do
+          Password
 
+      - if show_profile_social_tab?
+        %li
+          = link_to '#tab-social', 'data-toggle' => 'tab' do
+            Social Accounts
 
+      - if show_profile_username_tab?
+        %li
+          = link_to '#tab-username', 'data-toggle' => 'tab' do
+            Change Username
 
-%fieldset.update-token
+      - if show_profile_remove_tab?
+        %li
+          = link_to '#tab-remove', 'data-toggle' => 'tab' do
+            Remove Account
+  .span10
+    .tab-content
+      .tab-pane.active#tab-token
+        %fieldset.update-token
           %legend
             Private token
             %span.cred.pull-right
               keep it secret!
-  .padded
+          %div
             = form_for @user, url: reset_private_token_profile_path, method: :put do |f|
               .data
                 %p.slead
@@ -60,15 +54,48 @@
                     %span You don`t have one yet. Click generate to fix it.
                     = f.submit 'Generate', class: "btn success btn-build-token"
 
+      .tab-pane#tab-password
+        %fieldset.update-password
+          %legend Password
+          = form_for @user, url: update_password_profile_path, method: :put do |f|
+            %div
+              %p.slead After a successful password update you will be redirected to login page where you should login with your new password
+              -if @user.errors.any?
+                .alert.alert-error
+                  %ul
+                    - @user.errors.full_messages.each do |msg|
+                      %li= msg
+              .control-group
+                = f.label :password
+                .controls= f.password_field :password, required: true
+              .control-group
+                = f.label :password_confirmation
+                .controls
+                  = f.password_field :password_confirmation, required: true
+              .control-group
+                .controls
+                  = f.submit 'Save password', class: "btn btn-save"
+
+      - if show_profile_social_tab?
+        .tab-pane#tab-social
+          %fieldset
+            %legend Social Accounts
+            .oauth_select_holder
+              %p.hint Tip: Click on icon to activate signin with one of the following services
+              - enabled_social_providers.each do |provider|
+                %span{class: oauth_active_class(provider) }
+                  = link_to authbutton(provider, 32), omniauth_authorize_path(User, provider)
 
-- if current_user.can_change_username?
+      - if show_profile_username_tab?
+        .tab-pane#tab-username
           %fieldset.update-username
             %legend
               Username
               %small.cred.pull-right
                 Changing your username can have unintended side effects!
             = form_for @user, url: update_username_profile_path,  method: :put, remote: true do |f|
-      .padded
+              %div
+              .control-group
                 = f.label :username
                 .controls
                   = f.text_field :username, required: true
@@ -86,10 +113,20 @@
                 .controls
                   = f.submit 'Save username', class: "btn btn-save"
 
-- if gitlab_config.signup_enabled
+      - if show_profile_remove_tab?
+        .tab-pane#tab-remove
           %fieldset.remove-account
             %legend
               Remove account
-      %small.cred.pull-right
-        Before removing the account you must remove all projects!
-    = link_to 'Delete account', user_registration_path, confirm: "REMOVE #{current_user.name}? Are you sure?", method: :delete, class: "btn btn-remove delete-key btn-small pull-right"
+            %div
+              %p Deleting an account has the following effects:
+              %ul
+                %li All user content like authored issues, snippets, comments will be removed
+                - rp = current_user.personal_projects.count
+                - unless rp.zero?
+                  %li #{pluralize rp, 'personal project'} will be removed and cannot be restored
+                - if current_user.solo_owned_groups.present?
+                  %li
+                    Next groups will be abandoned. You should transfer or remove them:
+                    %strong #{current_user.solo_owned_groups.map(&:name).join(', ')}
+              = link_to 'Delete account', user_registration_path, confirm: "REMOVE #{current_user.name}? Are you sure?", method: :delete, class: "btn btn-remove"

lib/gitlab/auth.rb

@@ -7,97 +7,16 @@ module Gitlab
         # Second chance - try LDAP authentication
         return nil unless ldap_conf.enabled
 
-        ldap_auth(login, password)
+        Gitlab::LDAP::User.authenticate(login, password)
       else
         user if user.valid_password?(password)
       end
     end
 
-    def find_for_ldap_auth(auth, signed_in_resource = nil)
-      uid = auth.info.uid
-      provider = auth.provider
-      email = auth.info.email.downcase unless auth.info.email.nil?
-      raise OmniAuth::Error, "LDAP accounts must provide an uid and email address" if uid.nil? or email.nil?
-
-      if @user = User.find_by_extern_uid_and_provider(uid, provider)
-        @user
-      elsif @user = User.find_by_email(email)
-        log.info "Updating legacy LDAP user #{email} with extern_uid => #{uid}"
-        @user.update_attributes(extern_uid: uid, provider: provider)
-        @user
-      else
-        create_from_omniauth(auth, true)
-      end
-    end
-
-    def create_from_omniauth(auth, ldap = false)
-      provider = auth.provider
-      uid = auth.info.uid || auth.uid
-      uid = uid.to_s.force_encoding("utf-8")
-      name = auth.info.name.to_s.force_encoding("utf-8")
-      email = auth.info.email.to_s.downcase unless auth.info.email.nil?
-
-      ldap_prefix = ldap ? '(LDAP) ' : ''
-      raise OmniAuth::Error, "#{ldap_prefix}#{provider} does not provide an email"\
-        " address" if auth.info.email.blank?
-
-      log.info "#{ldap_prefix}Creating user from #{provider} login"\
-        " {uid => #{uid}, name => #{name}, email => #{email}}"
-      password = Devise.friendly_token[0, 8].downcase
-      @user = User.new({
-        extern_uid: uid,
-        provider: provider,
-        name: name,
-        username: email.match(/^[^@]*/)[0],
-        email: email,
-        password: password,
-        password_confirmation: password,
-      }, as: :admin).with_defaults
-      @user.save!
-
-      if Gitlab.config.omniauth['block_auto_created_users'] && !ldap
-        @user.block
-      end
-
-      @user
-    end
-
-    def find_or_new_for_omniauth(auth)
-      provider, uid = auth.provider, auth.uid
-      email = auth.info.email.downcase unless auth.info.email.nil?
-
-      if @user = User.find_by_provider_and_extern_uid(provider, uid)
-        @user
-      elsif @user = User.find_by_email(email)
-        @user.update_attributes(extern_uid: uid, provider: provider)
-        @user
-      else
-        if Gitlab.config.omniauth['allow_single_sign_on']
-          @user = create_from_omniauth(auth)
-          @user
-        end
-      end
-    end
-
     def log
       Gitlab::AppLogger
     end
 
-    def ldap_auth(login, password)
-      # Check user against LDAP backend if user is not authenticated
-      # Only check with valid login and password to prevent anonymous bind results
-      return nil unless ldap_conf.enabled && !login.blank? && !password.blank?
-
-      ldap = OmniAuth::LDAP::Adaptor.new(ldap_conf)
-      ldap_user = ldap.bind_as(
-        filter: Net::LDAP::Filter.eq(ldap.uid, login),
-        size: 1,
-        password: password
-      )
-
-      User.find_by_extern_uid_and_provider(ldap_user.dn, 'ldap') if ldap_user
-    end
-
     def ldap_conf
       @ldap_conf ||= Gitlab.config.ldap
     end

lib/gitlab/backend/grack_auth.rb

 require_relative 'shell_env'
-require_relative 'grack_ldap'
 require_relative 'grack_helpers'
 
 module Grack
   class Auth < Rack::Auth::Basic
-    include LDAP
     include Helpers
 
     attr_accessor :user, :project, :ref, :env

lib/gitlab/backend/grack_ldap.rb

-require 'omniauth-ldap'
-
-module Grack
-  module LDAP
-    def ldap_auth(login, password)
-      # Check user against LDAP backend if user is not authenticated
-      # Only check with valid login and password to prevent anonymous bind results
-      return nil unless ldap_conf.enabled && !login.blank? && !password.blank?
-
-      ldap = OmniAuth::LDAP::Adaptor.new(ldap_conf)
-      ldap_user = ldap.bind_as(
-        filter: Net::LDAP::Filter.eq(ldap.uid, login),
-        size: 1,
-        password: password
-      )
-
-      User.find_by_extern_uid_and_provider(ldap_user.dn, 'ldap') if ldap_user
-    end
-
-    def ldap_conf
-      @ldap_conf ||= Gitlab.config.ldap
-    end
-  end
-end

lib/gitlab/ldap/user.rb

+require 'gitlab/oauth/user'
+
+# LDAP extension for User model
+#
+# * Find or create user from omniauth.auth data
+# * Links LDAP account with existing user
+# * Auth LDAP user with login and password
+#
+module Gitlab
+  module LDAP
+    class User < Gitlab::OAuth::User
+      class << self
+        def find_or_create(auth)
+          @auth = auth
+
+          if uid.blank? || email.blank?
+            raise_error("Account must provide an uid and email address")
+          end
+
+          user = find(auth)
+
+          unless user
+            # Look for user with same emails
+            #
+            # Possible cases:
+            # * When user already has account and need to link his LDAP account.
+            # * LDAP uid changed for user with same email and we need to update his uid
+            #
+            user = model.find_by_email(email)
+
+            if user
+              user.update_attributes(extern_uid: uid, provider: provider)
+              log.info("(LDAP) Updating legacy LDAP user #{email} with extern_uid => #{uid}")
+            else
+              # Create a new user inside GitLab database
+              # based on LDAP credentials
+              #
+              #
+              user = create(auth)
+            end
+          end
+
+          user
+        end
+
+        def authenticate(login, password)
+          # Check user against LDAP backend if user is not authenticated
+          # Only check with valid login and password to prevent anonymous bind results
+          return nil unless ldap_conf.enabled && login.present? && password.present?
+
+          ldap = OmniAuth::LDAP::Adaptor.new(ldap_conf)
+          ldap_user = ldap.bind_as(
+            filter: Net::LDAP::Filter.eq(ldap.uid, login),
+            size: 1,
+            password: password
+          )
+
+          find_by_uid(ldap_user.dn) if ldap_user
+        end
+
+        private
+
+        def find_by_uid(uid)
+          model.where(provider: provider, extern_uid: uid).last
+        end
+
+        def provider
+          'ldap'
+        end
+
+        def raise_error(message)
+          raise OmniAuth::Error, "(LDAP) " + message
+        end
+
+        def ldap_conf
+          Gitlab.config.ldap
+        end
+      end
+    end
+  end
+end

lib/gitlab/oauth/user.rb

+# OAuth extension for User model
+#
+# * Find GitLab user based on omniauth uid and provider
+# * Create new user from omniauth data
+#
+module Gitlab
+  module OAuth
+    class User
+      class << self
+        attr_reader :auth
+
+        def find(auth)
+          @auth = auth
+          find_by_uid_and_provider
+        end
+
+        def create(auth)
+          @auth = auth
+          password = Devise.friendly_token[0, 8].downcase
+          opts = {
+            extern_uid: uid,
+            provider: provider,
+            name: name,
+            username: username,
+            email: email,
+            password: password,
+            password_confirmation: password,
+          }
+
+          user = model.new(opts, as: :admin).with_defaults
+          user.save!
+          log.info "(OAuth) Creating user #{email} from login with extern_uid => #{uid}"
+
+          if Gitlab.config.omniauth['block_auto_created_users'] && !ldap?
+            user.block
+          end
+
+          user
+        end
+
+        private
+
+        def find_by_uid_and_provider
+          model.where(provider: provider, extern_uid: uid).last
+        end
+
+        def uid
+          auth.info.uid || auth.uid
+        end
+
+        def email
+          auth.info.email.downcase unless auth.info.email.nil?
+        end
+
+        def name
+          auth.info.name.to_s.force_encoding("utf-8")
+        end
+
+        def username
+          email.match(/^[^@]*/)[0]
+        end
+
+        def provider
+          auth.provider
+        end
+
+        def log
+          Gitlab::AppLogger
+        end
+
+        def model
+          ::User
+        end
+
+        def raise_error(message)
+          raise OmniAuth::Error, "(OAuth) " + message
+        end
+
+        def ldap?
+          provider == 'ldap'
+        end
+      end
+    end
+  end
+end

spec/features/profile_spec.rb

@@ -17,26 +17,12 @@ describe "Profile account page" do
 
     it { page.should have_content("Remove account") }
 
-    it "should delete the account", js: true do
+    it "should delete the account" do
       expect { click_link "Delete account" }.to change {User.count}.by(-1)
       current_path.should == new_user_session_path
     end
   end
 
-  describe "when signup is enabled and user has a project" do
-    before do
-      Gitlab.config.gitlab.stub(:signup_enabled).and_return(true)
-      @project = create(:project, namespace: @user.namespace)
-      @project.team << [@user, :master]
-      visit account_profile_path
-    end
-    it { page.should have_content("Remove account") }
-
-    it "should not allow user to delete the account" do
-      expect { click_link "Delete account" }.not_to change {User.count}.by(-1)
-    end
-  end
-
   describe "when signup is disabled" do
     before do
       Gitlab.config.gitlab.stub(:signup_enabled).and_return(false)

spec/lib/auth_spec.rb

@@ -3,102 +3,26 @@ require 'spec_helper'
 describe Gitlab::Auth do
   let(:gl_auth) { Gitlab::Auth.new }
 
+  describe :find do
     before do
-    Gitlab.config.stub(omniauth: {})
-
-    @info = mock(
-      uid: '12djsak321',
-      name: 'John',
-      email: 'john@mail.com'
+      @user = create(
+        :user,
+        username: 'john',
+        password: '888777',
+        password_confirmation: '888777'
       )
     end
 
-  describe :find_for_ldap_auth do
-    before do
-      @auth = mock(
-        uid: '12djsak321',
-        info: @info,
-        provider: 'ldap'
-      )
-    end
-
-    it "should find by uid & provider" do
-      User.should_receive :find_by_extern_uid_and_provider
-      gl_auth.find_for_ldap_auth(@auth)
-    end
-
-    it "should update credentials by email if missing uid" do
-      user = double('User')
-      User.stub find_by_extern_uid_and_provider: nil
-      User.stub find_by_email: user
-      user.should_receive :update_attributes
-      gl_auth.find_for_ldap_auth(@auth)
+    it "should find user by valid login/password" do
+      gl_auth.find('john', '888777').should == @user
     end
 
-
-    it "should create from auth if user does not exist"do
-      User.stub find_by_extern_uid_and_provider: nil
-      User.stub find_by_email: nil
-      gl_auth.should_receive :create_from_omniauth
-      gl_auth.find_for_ldap_auth(@auth)
-    end
+    it "should not find user with invalid password" do
+      gl_auth.find('john', 'invalid').should_not == @user
     end
 
-  describe :find_or_new_for_omniauth do
-    before do
-      @auth = mock(
-        info: @info,
-        provider: 'twitter',
-        uid: '12djsak321',
-      )
-    end
-
-    it "should find user"do
-      User.should_receive :find_by_provider_and_extern_uid
-      gl_auth.should_not_receive :create_from_omniauth
-      gl_auth.find_or_new_for_omniauth(@auth)
-    end
-
-    it "should not create user"do
-      User.stub find_by_provider_and_extern_uid: nil
-      gl_auth.should_not_receive :create_from_omniauth
-      gl_auth.find_or_new_for_omniauth(@auth)
-    end
-
-    it "should create user if single_sing_on"do
-      Gitlab.config.omniauth['allow_single_sign_on'] = true
-      User.stub find_by_provider_and_extern_uid: nil
-      gl_auth.should_receive :create_from_omniauth
-      gl_auth.find_or_new_for_omniauth(@auth)
-    end
-  end
-
-  describe :create_from_omniauth do
-    it "should create user from LDAP" do
-      @auth = mock(info: @info, provider: 'ldap')
-      user = gl_auth.create_from_omniauth(@auth, true)
-
-      user.should be_valid
-      user.extern_uid.should == @info.uid
-      user.provider.should == 'ldap'
-    end
-
-    it "should create user from Omniauth" do
-      @auth = mock(info: @info, provider: 'twitter')
-      user = gl_auth.create_from_omniauth(@auth, false)
-
-      user.should be_valid
-      user.extern_uid.should == @info.uid
-      user.provider.should == 'twitter'
-    end
-
-    it "should apply defaults to user" do
-      @auth = mock(info: @info, provider: 'ldap')
-      user = gl_auth.create_from_omniauth(@auth, true)
-
-      user.should be_valid
-      user.projects_limit.should == Gitlab.config.gitlab.default_projects_limit
-      user.can_create_group.should == Gitlab.config.gitlab.default_can_create_group
+    it "should not find user with invalid login and password" do
+      gl_auth.find('jon', 'invalid').should_not == @user
     end
   end
 end

spec/lib/oauth_spec.rb

+require 'spec_helper'
+
+describe Gitlab::OAuth::User do
+  let(:gl_auth) { Gitlab::OAuth::User }
+
+  before do
+    Gitlab.config.stub(omniauth: {})
+
+    @info = mock(
+      uid: '12djsak321',
+      name: 'John',
+      email: 'john@mail.com'
+    )
+  end
+
+  describe :create do
+    it "should create user from LDAP" do
+      @auth = mock(info: @info, provider: 'ldap')
+      user = gl_auth.create(@auth)
+
+      user.should be_valid
+      user.extern_uid.should == @info.uid
+      user.provider.should == 'ldap'
+    end
+
+    it "should create user from Omniauth" do
+      @auth = mock(info: @info, provider: 'twitter')
+      user = gl_auth.create(@auth)
+
+      user.should be_valid
+      user.extern_uid.should == @info.uid
+      user.provider.should == 'twitter'
+    end
+
+    it "should apply defaults to user" do
+      @auth = mock(info: @info, provider: 'ldap')
+      user = gl_auth.create(@auth)
+
+      user.should be_valid
+      user.projects_limit.should == Gitlab.config.gitlab.default_projects_limit
+      user.can_create_group.should == Gitlab.config.gitlab.default_can_create_group
+    end
+  end
+end