From eefbc837301acc49a33617063faafa97adee307e Mon Sep 17 00:00:00 2001
From: Markus Koller <markus-koller@gmx.ch>
Date: Tue, 31 Jan 2017 11:21:29 +0100
Subject: [PATCH] Only use API scopes for personal access tokens

---
 .../personal_access_tokens_controller.rb       |  2 +-
 app/models/personal_access_token.rb            | 10 ++++++++++
 lib/gitlab/auth.rb                             |  9 +++++++--
 spec/initializers/doorkeeper_spec.rb           | 12 ++++++++++++
 spec/lib/gitlab/auth_spec.rb                   | 18 ++++++++++++++++++
 spec/models/personal_access_token_spec.rb      | 16 ++++++++++++++++
 6 files changed, 64 insertions(+), 3 deletions(-)
 create mode 100644 spec/initializers/doorkeeper_spec.rb

diff --git a/app/controllers/profiles/personal_access_tokens_controller.rb b/app/controllers/profiles/personal_access_tokens_controller.rb
index 6e007f17913..d7cc53a96d2 100644
--- a/app/controllers/profiles/personal_access_tokens_controller.rb
+++ b/app/controllers/profiles/personal_access_tokens_controller.rb
@@ -35,7 +35,7 @@ class Profiles::PersonalAccessTokensController < Profiles::ApplicationController
 
   def set_index_vars
     @personal_access_token ||= current_user.personal_access_tokens.build
-    @scopes = Gitlab::Auth::SCOPES
+    @scopes = Gitlab::Auth::API_SCOPES
     @active_personal_access_tokens = current_user.personal_access_tokens.active.order(:expires_at)
     @inactive_personal_access_tokens = current_user.personal_access_tokens.inactive
   end
diff --git a/app/models/personal_access_token.rb b/app/models/personal_access_token.rb
index 10a34c42fd8..f3e38aba7c9 100644
--- a/app/models/personal_access_token.rb
+++ b/app/models/personal_access_token.rb
@@ -9,6 +9,8 @@ class PersonalAccessToken < ActiveRecord::Base
   scope :active, -> { where(revoked: false).where("expires_at >= NOW() OR expires_at IS NULL") }
   scope :inactive, -> { where("revoked = true OR expires_at < NOW()") }
 
+  validate :validate_scopes
+
   def self.generate(params)
     personal_access_token = self.new(params)
     personal_access_token.ensure_token
@@ -19,4 +21,12 @@ class PersonalAccessToken < ActiveRecord::Base
     self.revoked = true
     self.save
   end
+
+  protected
+
+  def validate_scopes
+    unless Set.new(scopes.map(&:to_sym)).subset?(Set.new(Gitlab::Auth::API_SCOPES))
+      errors.add :scopes, "can only contain API scopes"
+    end
+  end
 end
diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb
index c6f9d0d7b82..92fe770728b 100644
--- a/lib/gitlab/auth.rb
+++ b/lib/gitlab/auth.rb
@@ -2,9 +2,14 @@ module Gitlab
   module Auth
     MissingPersonalTokenError = Class.new(StandardError)
 
-    SCOPES = [:api, :read_user, :openid, :profile, :email].freeze
+    # Scopes used for GitLab API access
+    API_SCOPES = [:api, :read_user].freeze
+
+    # Scopes used by doorkeeper-openid_connect
+    OPENID_SCOPES = [:openid].freeze
+
     DEFAULT_SCOPES = [:api].freeze
-    OPTIONAL_SCOPES = SCOPES - DEFAULT_SCOPES
+    OPTIONAL_SCOPES = (API_SCOPES + OPENID_SCOPES - DEFAULT_SCOPES).freeze
 
     class << self
       def find_for_git_client(login, password, project:, ip:)
diff --git a/spec/initializers/doorkeeper_spec.rb b/spec/initializers/doorkeeper_spec.rb
new file mode 100644
index 00000000000..32133edece7
--- /dev/null
+++ b/spec/initializers/doorkeeper_spec.rb
@@ -0,0 +1,12 @@
+require 'spec_helper'
+require_relative '../../config/initializers/doorkeeper'
+
+describe Doorkeeper.configuration do
+  it 'default_scopes matches Gitlab::Auth::DEFAULT_SCOPES' do
+    expect(subject.default_scopes).to eq Gitlab::Auth::DEFAULT_SCOPES
+  end
+
+  it 'optional_scopes matches Gitlab::Auth::OPTIONAL_SCOPES' do
+    expect(subject.optional_scopes).to eq Gitlab::Auth::OPTIONAL_SCOPES
+  end
+end
diff --git a/spec/lib/gitlab/auth_spec.rb b/spec/lib/gitlab/auth_spec.rb
index 8726ca569ca..0609ca78b3c 100644
--- a/spec/lib/gitlab/auth_spec.rb
+++ b/spec/lib/gitlab/auth_spec.rb
@@ -3,6 +3,24 @@ require 'spec_helper'
 describe Gitlab::Auth, lib: true do
   let(:gl_auth) { described_class }
 
+  describe 'constants' do
+    it 'API_SCOPES contains all scopes for API access' do
+      expect(subject::API_SCOPES).to eq [:api, :read_user]
+    end
+
+    it 'OPENID_SCOPES contains all scopes for OpenID Connect' do
+      expect(subject::OPENID_SCOPES).to eq [:openid]
+    end
+
+    it 'DEFAULT_SCOPES contains all default scopes' do
+      expect(subject::DEFAULT_SCOPES).to eq [:api]
+    end
+
+    it 'OPTIONAL_SCOPES contains all non-default scopes' do
+      expect(subject::OPTIONAL_SCOPES).to eq [:read_user, :openid]
+    end
+  end
+
   describe 'find_for_git_client' do
     context 'build token' do
       subject { gl_auth.find_for_git_client('gitlab-ci-token', build.token, project: project, ip: 'ip') }
diff --git a/spec/models/personal_access_token_spec.rb b/spec/models/personal_access_token_spec.rb
index 46eb71cef14..4cc9cf02e6d 100644
--- a/spec/models/personal_access_token_spec.rb
+++ b/spec/models/personal_access_token_spec.rb
@@ -12,4 +12,20 @@ describe PersonalAccessToken, models: true do
       expect(personal_access_token).not_to be_persisted
     end
   end
+
+  describe 'validate_scopes' do
+    it "allows creating a token with API scopes" do
+      personal_access_token = build(:personal_access_token)
+      personal_access_token.scopes = [:api, :read_user]
+
+      expect(personal_access_token).to be_valid
+    end
+
+    it "rejects creating a token with non-API scopes" do
+      personal_access_token = build(:personal_access_token)
+      personal_access_token.scopes = [:openid, :api]
+
+      expect(personal_access_token).not_to be_valid
+    end
+  end
 end
-- 
GitLab