Require explicit scopes on personal access tokens

Gitlab::Auth and API::APIGuard already check for at least one valid
scope on personal access tokens, so if the scopes are empty the token
will always fail validation.

4 files changed, 26 insertions, 29 deletions

diff --git a/app/models/personal_access_token.rb b/app/models/personal_access_token.rb
index f3e38aba7c9..df8a0612b18 100644
--- a/app/models/personal_access_token.rb
+++ b/app/models/personal_access_token.rb
@@ -9,7 +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
+  validates :scopes, presence: true
+  validate :validate_api_scopes
 
   def self.generate(params)
     personal_access_token = self.new(params)
@@ -24,8 +25,8 @@ class PersonalAccessToken < ActiveRecord::Base
 
   protected
 
-  def validate_scopes
-    unless Set.new(scopes.map(&:to_sym)).subset?(Set.new(Gitlab::Auth::API_SCOPES))
+  def validate_api_scopes
+    unless scopes.all? { |scope| Gitlab::Auth::API_SCOPES.include?(scope.to_sym) }
       errors.add :scopes, "can only contain API scopes"
     end
   end
diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb
index 92fe770728b..53edc0a9e2c 100644
--- a/lib/gitlab/auth.rb
+++ b/lib/gitlab/auth.rb
@@ -5,10 +5,13 @@ module Gitlab
     # Scopes used for GitLab API access
     API_SCOPES = [:api, :read_user].freeze
 
-    # Scopes used by doorkeeper-openid_connect
+    # Scopes used for OpenID Connect
     OPENID_SCOPES = [:openid].freeze
 
+    # Default scopes for OAuth applications that don't define their own
     DEFAULT_SCOPES = [:api].freeze
+
+    # Other available scopes
     OPTIONAL_SCOPES = (API_SCOPES + OPENID_SCOPES - DEFAULT_SCOPES).freeze
 
     class << self
diff --git a/spec/controllers/profiles/personal_access_tokens_spec.rb b/spec/controllers/profiles/personal_access_tokens_spec.rb
index 9d5f4c99f6d..19572ce53b7 100644
--- a/spec/controllers/profiles/personal_access_tokens_spec.rb
+++ b/spec/controllers/profiles/personal_access_tokens_spec.rb
@@ -2,6 +2,7 @@ require 'spec_helper'
 
 describe Profiles::PersonalAccessTokensController do
   let(:user) { create(:user) }
+  let(:token_attributes) { attributes_for(:personal_access_token) }
 
   describe '#create' do
     def created_token
@@ -10,40 +11,24 @@ describe Profiles::PersonalAccessTokensController do
 
     before { sign_in(user) }
 
-    it "allows creation of a token" do
-      name = FFaker::Product.brand
+    it "allows creation of a token with scopes" do
+      scopes = %w[api read_user]
 
-      post :create, personal_access_token: { name: name }
+      post :create, personal_access_token: token_attributes.merge(scopes: scopes)
 
       expect(created_token).not_to be_nil
-      expect(created_token.name).to eq(name)
-      expect(created_token.expires_at).to be_nil
+      expect(created_token.name).to eq(token_attributes[:name])
+      expect(created_token.scopes).to eq(scopes)
       expect(PersonalAccessToken.active).to include(created_token)
     end
 
     it "allows creation of a token with an expiry date" do
       expires_at = 5.days.from_now
 
-      post :create, personal_access_token: { name: FFaker::Product.brand, expires_at: expires_at }
+      post :create, personal_access_token: token_attributes.merge(expires_at: expires_at)
 
       expect(created_token).not_to be_nil
       expect(created_token.expires_at.to_i).to eq(expires_at.to_i)
     end
-
-    context "scopes" do
-      it "allows creation of a token with scopes" do
-        post :create, personal_access_token: { name: FFaker::Product.brand, scopes: %w(api read_user) }
-
-        expect(created_token).not_to be_nil
-        expect(created_token.scopes).to eq(%w(api read_user))
-      end
-
-      it "allows creation of a token with no scopes" do
-        post :create, personal_access_token: { name: FFaker::Product.brand, scopes: [] }
-
-        expect(created_token).not_to be_nil
-        expect(created_token.scopes).to eq([])
-      end
-    end
   end
 end
diff --git a/spec/models/personal_access_token_spec.rb b/spec/models/personal_access_token_spec.rb
index 4cc9cf02e6d..50f61ec18fd 100644
--- a/spec/models/personal_access_token_spec.rb
+++ b/spec/models/personal_access_token_spec.rb
@@ -13,19 +13,27 @@ describe PersonalAccessToken, models: true do
     end
   end
 
-  describe 'validate_scopes' do
+  context "validations" do
+    let(:personal_access_token) { build(:personal_access_token) }
+
+    it "requires at least one scope" do
+      personal_access_token.scopes = []
+
+      expect(personal_access_token).not_to be_valid
+      expect(personal_access_token.errors[:scopes].first).to eq "can't be blank"
+    end
+
     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
+      expect(personal_access_token.errors[:scopes].first).to eq "can only contain API scopes"
     end
   end
 end