From c4982890489d254da2fe998aab30bf257767ed5e Mon Sep 17 00:00:00 2001
From: Markus Koller <markus-koller@gmx.ch>
Date: Fri, 9 Dec 2016 18:36:50 +0100
Subject: Implement OpenID Connect identity provider

---
 Gemfile                                            |   1 +
 Gemfile.lock                                       |  13 ++
 app/models/oauth_access_grant.rb                   |   4 +
 app/models/oauth_access_token.rb                   |   2 +-
 app/views/doorkeeper/authorizations/new.html.haml  |   2 +
 changelogs/unreleased/feature-openid-connect.yml   |   4 +
 config/initializers/doorkeeper.rb                  |   7 +-
 config/initializers/doorkeeper_openid_connect.rb   |  36 ++++++
 config/initializers/secret_token.rb                |   7 +-
 config/locales/doorkeeper.en.yml                   |   1 +
 config/routes.rb                                   |   2 +
 ...5216_create_doorkeeper_openid_connect_tables.rb |  37 ++++++
 ...lidate_foreign_keys_on_oauth_openid_requests.rb |  20 +++
 db/schema.rb                                       |   6 +
 doc/integration/README.md                          |   1 +
 doc/integration/openid_connect_provider.md         |  47 ++++++++
 lib/gitlab/auth.rb                                 |   2 +-
 spec/factories/oauth_access_grants.rb              |  11 ++
 spec/factories/oauth_access_tokens.rb              |   3 +-
 spec/factories/oauth_applications.rb               |   2 +-
 spec/initializers/secret_token_spec.rb             |  25 +++-
 spec/requests/openid_connect_spec.rb               | 134 +++++++++++++++++++++
 spec/routing/openid_connect_spec.rb                |  30 +++++
 23 files changed, 388 insertions(+), 9 deletions(-)
 create mode 100644 app/models/oauth_access_grant.rb
 create mode 100644 changelogs/unreleased/feature-openid-connect.yml
 create mode 100644 config/initializers/doorkeeper_openid_connect.rb
 create mode 100644 db/migrate/20161209165216_create_doorkeeper_openid_connect_tables.rb
 create mode 100644 db/post_migrate/20170209140523_validate_foreign_keys_on_oauth_openid_requests.rb
 create mode 100644 doc/integration/openid_connect_provider.md
 create mode 100644 spec/factories/oauth_access_grants.rb
 create mode 100644 spec/requests/openid_connect_spec.rb
 create mode 100644 spec/routing/openid_connect_spec.rb

diff --git a/Gemfile b/Gemfile
index 4ac5a0ccfc1..b66cd38b4dc 100644
--- a/Gemfile
+++ b/Gemfile
@@ -20,6 +20,7 @@ gem 'rugged', '~> 0.24.0'
 # Authentication libraries
 gem 'devise',                 '~> 4.2'
 gem 'doorkeeper',             '~> 4.2.0'
+gem 'doorkeeper-openid_connect', '~> 1.1.0'
 gem 'omniauth',               '~> 1.4.2'
 gem 'omniauth-auth0',         '~> 1.4.1'
 gem 'omniauth-azure-oauth2',  '~> 0.0.6'
diff --git a/Gemfile.lock b/Gemfile.lock
index d4131a3dede..62388628eaa 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -78,6 +78,7 @@ GEM
     better_errors (1.0.1)
       coderay (>= 1.0.0)
       erubis (>= 2.6.6)
+    bindata (2.3.5)
     binding_of_caller (0.7.2)
       debug_inspector (>= 0.0.1)
     bootstrap-sass (3.3.6)
@@ -167,6 +168,9 @@ GEM
       unf (>= 0.0.5, < 1.0.0)
     doorkeeper (4.2.0)
       railties (>= 4.2)
+    doorkeeper-openid_connect (1.1.2)
+      doorkeeper (~> 4.0)
+      json-jwt (~> 1.6)
     dropzonejs-rails (0.7.2)
       rails (> 3.1)
     email_reply_trimmer (0.1.6)
@@ -376,6 +380,12 @@ GEM
       railties (>= 4.2.0)
       thor (>= 0.14, < 2.0)
     json (1.8.6)
+    json-jwt (1.7.1)
+      activesupport
+      bindata
+      multi_json (>= 1.3)
+      securecompare
+      url_safe_base64
     json-schema (2.6.2)
       addressable (~> 2.3.8)
     jwt (1.5.6)
@@ -684,6 +694,7 @@ GEM
     scss_lint (0.47.1)
       rake (>= 0.9, < 11)
       sass (~> 3.4.15)
+    securecompare (1.0.0)
     seed-fu (2.3.6)
       activerecord (>= 3.1)
       activesupport (>= 3.1)
@@ -789,6 +800,7 @@ GEM
       get_process_mem (~> 0)
       unicorn (>= 4, < 6)
     uniform_notifier (1.10.0)
+    url_safe_base64 (0.2.2)
     validates_hostname (1.0.6)
       activerecord (>= 3.0)
       activesupport (>= 3.0)
@@ -866,6 +878,7 @@ DEPENDENCIES
   devise-two-factor (~> 3.0.0)
   diffy (~> 3.1.0)
   doorkeeper (~> 4.2.0)
+  doorkeeper-openid_connect (~> 1.1.0)
   dropzonejs-rails (~> 0.7.1)
   email_reply_trimmer (~> 0.1)
   email_spec (~> 1.6.0)
diff --git a/app/models/oauth_access_grant.rb b/app/models/oauth_access_grant.rb
new file mode 100644
index 00000000000..3a997406565
--- /dev/null
+++ b/app/models/oauth_access_grant.rb
@@ -0,0 +1,4 @@
+class OauthAccessGrant < Doorkeeper::AccessGrant
+  belongs_to :resource_owner, class_name: 'User'
+  belongs_to :application, class_name: 'Doorkeeper::Application'
+end
diff --git a/app/models/oauth_access_token.rb b/app/models/oauth_access_token.rb
index 116fb71ac08..b85f5dbaf2e 100644
--- a/app/models/oauth_access_token.rb
+++ b/app/models/oauth_access_token.rb
@@ -1,4 +1,4 @@
-class OauthAccessToken < ActiveRecord::Base
+class OauthAccessToken < Doorkeeper::AccessToken
   belongs_to :resource_owner, class_name: 'User'
   belongs_to :application, class_name: 'Doorkeeper::Application'
 end
diff --git a/app/views/doorkeeper/authorizations/new.html.haml b/app/views/doorkeeper/authorizations/new.html.haml
index a196561f381..82aa51f9778 100644
--- a/app/views/doorkeeper/authorizations/new.html.haml
+++ b/app/views/doorkeeper/authorizations/new.html.haml
@@ -27,6 +27,7 @@
       = hidden_field_tag :state, @pre_auth.state
       = hidden_field_tag :response_type, @pre_auth.response_type
       = hidden_field_tag :scope, @pre_auth.scope
+      = hidden_field_tag :nonce, @pre_auth.nonce
       = submit_tag "Authorize", class: "btn btn-success wide pull-left"
     = form_tag oauth_authorization_path, method: :delete do
       = hidden_field_tag :client_id, @pre_auth.client.uid
@@ -34,4 +35,5 @@
       = hidden_field_tag :state, @pre_auth.state
       = hidden_field_tag :response_type, @pre_auth.response_type
       = hidden_field_tag :scope, @pre_auth.scope
+      = hidden_field_tag :nonce, @pre_auth.nonce
       = submit_tag "Deny", class: "btn btn-danger prepend-left-10"
diff --git a/changelogs/unreleased/feature-openid-connect.yml b/changelogs/unreleased/feature-openid-connect.yml
new file mode 100644
index 00000000000..e84eb7aff86
--- /dev/null
+++ b/changelogs/unreleased/feature-openid-connect.yml
@@ -0,0 +1,4 @@
+---
+title: Implement OpenID Connect identity provider
+merge_request: 8018
+author: Markus Koller
diff --git a/config/initializers/doorkeeper.rb b/config/initializers/doorkeeper.rb
index 88cd0f5f652..52551a5d5eb 100644
--- a/config/initializers/doorkeeper.rb
+++ b/config/initializers/doorkeeper.rb
@@ -8,7 +8,12 @@ Doorkeeper.configure do
     # Put your resource owner authentication logic here.
     # Ensure user is redirected to redirect_uri after login
     session[:user_return_to] = request.fullpath
-    current_user || redirect_to(new_user_session_url)
+    if current_user
+      current_user
+    else
+      redirect_to(new_user_session_url)
+      nil
+    end
   end
 
   resource_owner_from_credentials do |routes|
diff --git a/config/initializers/doorkeeper_openid_connect.rb b/config/initializers/doorkeeper_openid_connect.rb
new file mode 100644
index 00000000000..700ca25b884
--- /dev/null
+++ b/config/initializers/doorkeeper_openid_connect.rb
@@ -0,0 +1,36 @@
+Doorkeeper::OpenidConnect.configure do
+  issuer Gitlab.config.gitlab.url
+
+  jws_private_key Rails.application.secrets.jws_private_key
+
+  resource_owner_from_access_token do |access_token|
+    User.active.find_by(id: access_token.resource_owner_id)
+  end
+
+  auth_time_from_resource_owner do |user|
+    user.current_sign_in_at
+  end
+
+  reauthenticate_resource_owner do |user, return_to|
+    store_location_for user, return_to
+    sign_out user
+    redirect_to new_user_session_url
+  end
+
+  subject do |user|
+    # hash the user's ID with the Rails secret_key_base to avoid revealing it
+    Digest::SHA256.hexdigest "#{user.id}-#{Rails.application.secrets.secret_key_base}"
+  end
+
+  claims do
+    with_options scope: :openid do |o|
+      o.claim(:name)           { |user| user.name }
+      o.claim(:nickname)       { |user| user.username }
+      o.claim(:email)          { |user| user.public_email  }
+      o.claim(:email_verified) { |user| true if user.public_email? }
+      o.claim(:website)        { |user| user.full_website_url if user.website_url? }
+      o.claim(:profile)        { |user| Rails.application.routes.url_helpers.user_url user }
+      o.claim(:picture)        { |user| user.avatar_url }
+    end
+  end
+end
diff --git a/config/initializers/secret_token.rb b/config/initializers/secret_token.rb
index 291fa6c0abc..f9c1d2165d3 100644
--- a/config/initializers/secret_token.rb
+++ b/config/initializers/secret_token.rb
@@ -24,7 +24,8 @@ def create_tokens
   defaults = {
     secret_key_base: file_secret_key || generate_new_secure_token,
     otp_key_base: env_secret_key || file_secret_key || generate_new_secure_token,
-    db_key_base: generate_new_secure_token
+    db_key_base: generate_new_secure_token,
+    jws_private_key: generate_new_rsa_private_key
   }
 
   missing_secrets = set_missing_keys(defaults)
@@ -41,6 +42,10 @@ def generate_new_secure_token
   SecureRandom.hex(64)
 end
 
+def generate_new_rsa_private_key
+  OpenSSL::PKey::RSA.new(2048).to_pem
+end
+
 def warn_missing_secret(secret)
   warn "Missing Rails.application.secrets.#{secret} for #{Rails.env} environment. The secret will be generated and stored in config/secrets.yml."
 end
diff --git a/config/locales/doorkeeper.en.yml b/config/locales/doorkeeper.en.yml
index 1d728282d90..14d49885fb3 100644
--- a/config/locales/doorkeeper.en.yml
+++ b/config/locales/doorkeeper.en.yml
@@ -60,6 +60,7 @@ en:
     scopes:
       api: Access your API
       read_user: Read user information
+      openid: Authenticate using OpenID Connect
 
     flash:
       applications:
diff --git a/config/routes.rb b/config/routes.rb
index 06293316937..1a851da6203 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -22,6 +22,8 @@ Rails.application.routes.draw do
                 authorizations: 'oauth/authorizations'
   end
 
+  use_doorkeeper_openid_connect
+
   # Autocomplete
   get '/autocomplete/users' => 'autocomplete#users'
   get '/autocomplete/users/:id' => 'autocomplete#user'
diff --git a/db/migrate/20161209165216_create_doorkeeper_openid_connect_tables.rb b/db/migrate/20161209165216_create_doorkeeper_openid_connect_tables.rb
new file mode 100644
index 00000000000..e63d5927f86
--- /dev/null
+++ b/db/migrate/20161209165216_create_doorkeeper_openid_connect_tables.rb
@@ -0,0 +1,37 @@
+class CreateDoorkeeperOpenidConnectTables < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    create_table :oauth_openid_requests do |t|
+      t.integer :access_grant_id, null: false
+      t.string :nonce, null: false
+    end
+
+    if Gitlab::Database.postgresql?
+      # add foreign key without validation to avoid downtime on PostgreSQL,
+      # also see db/post_migrate/20170209140523_validate_foreign_keys_on_oauth_openid_requests.rb
+      execute %q{
+        ALTER TABLE "oauth_openid_requests"
+          ADD CONSTRAINT "fk_oauth_openid_requests_oauth_access_grants_access_grant_id"
+          FOREIGN KEY ("access_grant_id")
+          REFERENCES "oauth_access_grants" ("id")
+          NOT VALID;
+      }
+    else
+      execute %q{
+        ALTER TABLE oauth_openid_requests
+          ADD CONSTRAINT fk_oauth_openid_requests_oauth_access_grants_access_grant_id
+          FOREIGN KEY (access_grant_id)
+          REFERENCES oauth_access_grants (id);
+      }
+    end
+  end
+
+  def down
+    drop_table :oauth_openid_requests
+  end
+end
diff --git a/db/post_migrate/20170209140523_validate_foreign_keys_on_oauth_openid_requests.rb b/db/post_migrate/20170209140523_validate_foreign_keys_on_oauth_openid_requests.rb
new file mode 100644
index 00000000000..e206f9af636
--- /dev/null
+++ b/db/post_migrate/20170209140523_validate_foreign_keys_on_oauth_openid_requests.rb
@@ -0,0 +1,20 @@
+class ValidateForeignKeysOnOauthOpenidRequests < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    if Gitlab::Database.postgresql?
+      execute %q{
+        ALTER TABLE "oauth_openid_requests"
+          VALIDATE CONSTRAINT "fk_oauth_openid_requests_oauth_access_grants_access_grant_id";
+      }
+    end
+  end
+
+  def down
+    # noop
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 624cf9432d0..8984ab3aac1 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -878,6 +878,11 @@ ActiveRecord::Schema.define(version: 20170306170512) do
   add_index "oauth_applications", ["owner_id", "owner_type"], name: "index_oauth_applications_on_owner_id_and_owner_type", using: :btree
   add_index "oauth_applications", ["uid"], name: "index_oauth_applications_on_uid", unique: true, using: :btree
 
+  create_table "oauth_openid_requests", force: :cascade do |t|
+    t.integer "access_grant_id", null: false
+    t.string "nonce", null: false
+  end
+
   create_table "pages_domains", force: :cascade do |t|
     t.integer "project_id"
     t.text "certificate"
@@ -1374,6 +1379,7 @@ ActiveRecord::Schema.define(version: 20170306170512) do
   add_foreign_key "merge_request_metrics", "merge_requests", on_delete: :cascade
   add_foreign_key "merge_requests_closing_issues", "issues", on_delete: :cascade
   add_foreign_key "merge_requests_closing_issues", "merge_requests", on_delete: :cascade
+  add_foreign_key "oauth_openid_requests", "oauth_access_grants", column: "access_grant_id", name: "fk_oauth_openid_requests_oauth_access_grants_access_grant_id"
   add_foreign_key "personal_access_tokens", "users"
   add_foreign_key "project_authorizations", "projects", on_delete: :cascade
   add_foreign_key "project_authorizations", "users", on_delete: :cascade
diff --git a/doc/integration/README.md b/doc/integration/README.md
index 22bdf33443d..e56e58498a6 100644
--- a/doc/integration/README.md
+++ b/doc/integration/README.md
@@ -12,6 +12,7 @@ See the documentation below for details on how to configure these services.
 - [SAML](saml.md) Configure GitLab as a SAML 2.0 Service Provider
 - [CAS](cas.md) Configure GitLab to sign in using CAS
 - [OAuth2 provider](oauth_provider.md) OAuth2 application creation
+- [OpenID Connect](openid_connect_provider.md) Use GitLab as an identity provider
 - [Gmail actions buttons](gmail_action_buttons_for_gitlab.md) Adds GitLab actions to messages
 - [reCAPTCHA](recaptcha.md) Configure GitLab to use Google reCAPTCHA for new users
 - [Akismet](akismet.md) Configure Akismet to stop spam
diff --git a/doc/integration/openid_connect_provider.md b/doc/integration/openid_connect_provider.md
new file mode 100644
index 00000000000..56f367d841e
--- /dev/null
+++ b/doc/integration/openid_connect_provider.md
@@ -0,0 +1,47 @@
+# GitLab as OpenID Connect identity provider
+
+This document is about using GitLab as an OpenID Connect identity provider
+to sign in to other services.
+
+## Introduction to OpenID Connect
+
+[OpenID Connect] \(OIC) is a simple identity layer on top of the
+OAuth 2.0 protocol. It allows clients to verify the identity of the end-user
+based on the authentication performed by GitLab, as well as to obtain
+basic profile information about the end-user in an interoperable and
+REST-like manner. OIC performs many of the same tasks as OpenID 2.0,
+but does so in a way that is API-friendly, and usable by native and
+mobile applications.
+
+On the client side, you can use [omniauth-openid-connect] for Rails
+applications, or any of the other available [client implementations].
+
+GitLab's implementation uses the [doorkeeper-openid_connect] gem, refer
+to its README for more details about which parts of the specifications
+are supported.
+
+## Enabling OpenID Connect for OAuth applications
+
+Refer to the [OAuth guide] for basic information on how to set up OAuth
+applications in GitLab. To enable OIC for an application, all you have to do
+is select the `openid` scope in the application settings.
+
+Currently the following user information is shared with clients:
+
+| Claim            | Type      | Description |
+|:-----------------|:----------|:------------|
+| `sub`            | `string`  | An opaque token that uniquely identifies the user
+| `auth_time`      | `integer` | The timestamp for the user's last authentication
+| `name`           | `string`  | The user's full name
+| `nickname`       | `string`  | The user's GitLab username
+| `email`          | `string`  | The user's public email address
+| `email_verified` | `boolean` | Whether the user's public email address was verified
+| `website`        | `string`  | URL for the user's website
+| `profile`        | `string`  | URL for the user's GitLab profile
+| `picture`        | `string`  | URL for the user's GitLab avatar
+
+[OpenID Connect]: http://openid.net/connect/ "OpenID Connect website"
+[doorkeeper-openid_connect]: https://github.com/doorkeeper-gem/doorkeeper-openid_connect "Doorkeeper::OpenidConnect website"
+[OAuth guide]: oauth_provider.md "GitLab as OAuth2 authentication service provider"
+[omniauth-openid-connect]: https://github.com/jjbohn/omniauth-openid-connect/ "OmniAuth::OpenIDConnect website"
+[client implementations]: http://openid.net/developers/libraries#connect "List of available client implementations"
diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb
index 0a0bd0e781c..6166f8d0dd9 100644
--- a/lib/gitlab/auth.rb
+++ b/lib/gitlab/auth.rb
@@ -2,7 +2,7 @@ module Gitlab
   module Auth
     MissingPersonalTokenError = Class.new(StandardError)
 
-    SCOPES = [:api, :read_user].freeze
+    SCOPES = [:api, :read_user, :openid, :profile, :email].freeze
     DEFAULT_SCOPES = [:api].freeze
     OPTIONAL_SCOPES = SCOPES - DEFAULT_SCOPES
 
diff --git a/spec/factories/oauth_access_grants.rb b/spec/factories/oauth_access_grants.rb
new file mode 100644
index 00000000000..543b3e99274
--- /dev/null
+++ b/spec/factories/oauth_access_grants.rb
@@ -0,0 +1,11 @@
+FactoryGirl.define do
+  factory :oauth_access_grant do
+    resource_owner_id { create(:user).id }
+    application
+    token { Doorkeeper::OAuth::Helpers::UniqueToken.generate }
+    expires_in 2.hours
+
+    redirect_uri { application.redirect_uri }
+    scopes { application.scopes }
+  end
+end
diff --git a/spec/factories/oauth_access_tokens.rb b/spec/factories/oauth_access_tokens.rb
index ccf02d0719b..a46bc1d8ce8 100644
--- a/spec/factories/oauth_access_tokens.rb
+++ b/spec/factories/oauth_access_tokens.rb
@@ -2,6 +2,7 @@ FactoryGirl.define do
   factory :oauth_access_token do
     resource_owner
     application
-    token '123456'
+    token { Doorkeeper::OAuth::Helpers::UniqueToken.generate }
+    scopes { application.scopes }
   end
 end
diff --git a/spec/factories/oauth_applications.rb b/spec/factories/oauth_applications.rb
index d116a573830..86cdc208268 100644
--- a/spec/factories/oauth_applications.rb
+++ b/spec/factories/oauth_applications.rb
@@ -1,7 +1,7 @@
 FactoryGirl.define do
   factory :oauth_application, class: 'Doorkeeper::Application', aliases: [:application] do
     name { FFaker::Name.name }
-    uid { FFaker::Name.name }
+    uid { Doorkeeper::OAuth::Helpers::UniqueToken.generate }
     redirect_uri { FFaker::Internet.uri('http') }
     owner
     owner_type 'User'
diff --git a/spec/initializers/secret_token_spec.rb b/spec/initializers/secret_token_spec.rb
index ad7f032d1e5..65c97da2efd 100644
--- a/spec/initializers/secret_token_spec.rb
+++ b/spec/initializers/secret_token_spec.rb
@@ -6,6 +6,9 @@ describe 'create_tokens', lib: true do
 
   let(:secrets) { ActiveSupport::OrderedOptions.new }
 
+  HEX_KEY = /\h{128}/
+  RSA_KEY = /\A-----BEGIN RSA PRIVATE KEY-----\n.+\n-----END RSA PRIVATE KEY-----\n\Z/m
+
   before do
     allow(File).to receive(:write)
     allow(File).to receive(:delete)
@@ -15,7 +18,7 @@ describe 'create_tokens', lib: true do
     allow(self).to receive(:exit)
   end
 
-  context 'setting secret_key_base and otp_key_base' do
+  context 'setting secret keys' do
     context 'when none of the secrets exist' do
       before do
         stub_env('SECRET_KEY_BASE', nil)
@@ -24,19 +27,29 @@ describe 'create_tokens', lib: true do
         allow(self).to receive(:warn_missing_secret)
       end
 
-      it 'generates different secrets for secret_key_base, otp_key_base, and db_key_base' do
+      it 'generates different hashes for secret_key_base, otp_key_base, and db_key_base' do
         create_tokens
 
         keys = secrets.values_at(:secret_key_base, :otp_key_base, :db_key_base)
 
         expect(keys.uniq).to eq(keys)
-        expect(keys.map(&:length)).to all(eq(128))
+        expect(keys).to all(match(HEX_KEY))
+      end
+
+      it 'generates an RSA key for jws_private_key' do
+        create_tokens
+
+        keys = secrets.values_at(:jws_private_key)
+
+        expect(keys.uniq).to eq(keys)
+        expect(keys).to all(match(RSA_KEY))
       end
 
       it 'warns about the secrets to add to secrets.yml' do
         expect(self).to receive(:warn_missing_secret).with('secret_key_base')
         expect(self).to receive(:warn_missing_secret).with('otp_key_base')
         expect(self).to receive(:warn_missing_secret).with('db_key_base')
+        expect(self).to receive(:warn_missing_secret).with('jws_private_key')
 
         create_tokens
       end
@@ -48,6 +61,7 @@ describe 'create_tokens', lib: true do
           expect(new_secrets['secret_key_base']).to eq(secrets.secret_key_base)
           expect(new_secrets['otp_key_base']).to eq(secrets.otp_key_base)
           expect(new_secrets['db_key_base']).to eq(secrets.db_key_base)
+          expect(new_secrets['jws_private_key']).to eq(secrets.jws_private_key)
         end
 
         create_tokens
@@ -63,6 +77,7 @@ describe 'create_tokens', lib: true do
     context 'when the other secrets all exist' do
       before do
         secrets.db_key_base = 'db_key_base'
+        secrets.jws_private_key = 'jws_private_key'
 
         allow(File).to receive(:exist?).with('.secret').and_return(true)
         allow(File).to receive(:read).with('.secret').and_return('file_key')
@@ -73,6 +88,7 @@ describe 'create_tokens', lib: true do
           stub_env('SECRET_KEY_BASE', 'env_key')
           secrets.secret_key_base = 'secret_key_base'
           secrets.otp_key_base = 'otp_key_base'
+          secrets.jws_private_key = 'jws_private_key'
         end
 
         it 'does not issue a warning' do
@@ -98,6 +114,7 @@ describe 'create_tokens', lib: true do
         before do
           secrets.secret_key_base = 'secret_key_base'
           secrets.otp_key_base = 'otp_key_base'
+          secrets.jws_private_key = 'jws_private_key'
         end
 
         it 'does not write any files' do
@@ -112,6 +129,7 @@ describe 'create_tokens', lib: true do
           expect(secrets.secret_key_base).to eq('secret_key_base')
           expect(secrets.otp_key_base).to eq('otp_key_base')
           expect(secrets.db_key_base).to eq('db_key_base')
+          expect(secrets.jws_private_key).to eq('jws_private_key')
         end
 
         it 'deletes the .secret file' do
@@ -135,6 +153,7 @@ describe 'create_tokens', lib: true do
             expect(new_secrets['secret_key_base']).to eq('file_key')
             expect(new_secrets['otp_key_base']).to eq('file_key')
             expect(new_secrets['db_key_base']).to eq('db_key_base')
+            expect(new_secrets['jws_private_key']).to eq('jws_private_key')
           end
 
           create_tokens
diff --git a/spec/requests/openid_connect_spec.rb b/spec/requests/openid_connect_spec.rb
new file mode 100644
index 00000000000..5206634bca5
--- /dev/null
+++ b/spec/requests/openid_connect_spec.rb
@@ -0,0 +1,134 @@
+require 'spec_helper'
+
+describe 'OpenID Connect requests' do
+  include ApiHelpers
+
+  let(:user) { create :user }
+  let(:access_grant) { create :oauth_access_grant, application: application, resource_owner_id: user.id }
+  let(:access_token) { create :oauth_access_token, application: application, resource_owner_id: user.id }
+
+  def request_access_token
+    login_as user
+
+    post '/oauth/token',
+      grant_type: 'authorization_code',
+      code: access_grant.token,
+      redirect_uri: application.redirect_uri,
+      client_id: application.uid,
+      client_secret: application.secret
+  end
+
+  def request_user_info
+    get '/oauth/userinfo', nil, 'Authorization' => "Bearer #{access_token.token}"
+  end
+
+  def hashed_subject
+    Digest::SHA256.hexdigest("#{user.id}-#{Rails.application.secrets.secret_key_base}")
+  end
+
+  context 'Application without OpenID scope' do
+    let(:application) { create :oauth_application, scopes: 'api' }
+
+    it 'token response does not include an ID token' do
+      request_access_token
+
+      expect(json_response).to include 'access_token'
+      expect(json_response).not_to include 'id_token'
+    end
+
+    it 'userinfo response is unauthorized' do
+      request_user_info
+
+      expect(response).to have_http_status 403
+      expect(response.body).to be_blank
+    end
+  end
+
+  context 'Application with OpenID scope' do
+    let(:application) { create :oauth_application, scopes: 'openid' }
+
+    it 'token response includes an ID token' do
+      request_access_token
+
+      expect(json_response).to include 'id_token'
+    end
+
+    context 'UserInfo payload' do
+      let(:user) do
+        create(
+          :user,
+          name: 'Alice',
+          username: 'alice',
+          emails: [private_email, public_email],
+          email: private_email.email,
+          public_email: public_email.email,
+          website_url: 'https://example.com',
+          avatar: fixture_file_upload(Rails.root + "spec/fixtures/dk.png"),
+        )
+      end
+
+      let(:public_email) { build :email, email: 'public@example.com' }
+      let(:private_email) { build :email, email: 'private@example.com' }
+
+      it 'includes all user information' do
+        request_user_info
+
+        expect(json_response).to eq({
+          'sub'            => hashed_subject,
+          'name'           => 'Alice',
+          'nickname'       => 'alice',
+          'email'          => 'public@example.com',
+          'email_verified' => true,
+          'website'        => 'https://example.com',
+          'profile'        => 'http://localhost/alice',
+          'picture'        => "http://localhost/uploads/user/avatar/#{user.id}/dk.png",
+        })
+      end
+    end
+
+    context 'ID token payload' do
+      before do
+        request_access_token
+        @payload = JSON::JWT.decode(json_response['id_token'], :skip_verification)
+      end
+
+      it 'includes the Gitlab root URL' do
+        expect(@payload['iss']).to eq Gitlab.config.gitlab.url
+      end
+
+      it 'includes the hashed user ID' do
+        expect(@payload['sub']).to eq hashed_subject
+      end
+
+      it 'includes the time of the last authentication' do
+        expect(@payload['auth_time']).to eq user.current_sign_in_at.to_i
+      end
+
+      it 'does not include any unknown properties' do
+        expect(@payload.keys).to eq %w[iss sub aud exp iat auth_time]
+      end
+    end
+
+    context 'when user is blocked' do
+      it 'returns authentication error' do
+        access_grant
+        user.block
+
+        expect do
+          request_access_token
+        end.to throw_symbol :warden
+      end
+    end
+
+    context 'when user is ldap_blocked' do
+      it 'returns authentication error' do
+        access_grant
+        user.ldap_block
+
+        expect do
+          request_access_token
+        end.to throw_symbol :warden
+      end
+    end
+  end
+end
diff --git a/spec/routing/openid_connect_spec.rb b/spec/routing/openid_connect_spec.rb
new file mode 100644
index 00000000000..2c3bc08f1a1
--- /dev/null
+++ b/spec/routing/openid_connect_spec.rb
@@ -0,0 +1,30 @@
+require 'spec_helper'
+
+# oauth_discovery_keys      GET /oauth/discovery/keys(.:format)             doorkeeper/openid_connect/discovery#keys
+# oauth_discovery_provider  GET /.well-known/openid-configuration(.:format) doorkeeper/openid_connect/discovery#provider
+# oauth_discovery_webfinger GET /.well-known/webfinger(.:format)            doorkeeper/openid_connect/discovery#webfinger
+describe Doorkeeper::OpenidConnect::DiscoveryController, 'routing' do
+  it "to #provider" do
+    expect(get('/.well-known/openid-configuration')).to route_to('doorkeeper/openid_connect/discovery#provider')
+  end
+
+  it "to #webfinger" do
+    expect(get('/.well-known/webfinger')).to route_to('doorkeeper/openid_connect/discovery#webfinger')
+  end
+
+  it "to #keys" do
+    expect(get('/oauth/discovery/keys')).to route_to('doorkeeper/openid_connect/discovery#keys')
+  end
+end
+
+# oauth_userinfo GET  /oauth/userinfo(.:format) doorkeeper/openid_connect/userinfo#show
+#                POST /oauth/userinfo(.:format) doorkeeper/openid_connect/userinfo#show
+describe Doorkeeper::OpenidConnect::UserinfoController, 'routing' do
+  it "to #show" do
+    expect(get('/oauth/userinfo')).to route_to('doorkeeper/openid_connect/userinfo#show')
+  end
+
+  it "to #show" do
+    expect(post('/oauth/userinfo')).to route_to('doorkeeper/openid_connect/userinfo#show')
+  end
+end
-- 
cgit v1.2.1


From 6bf7037ecdf33de9b1d3962bda547b1097cdd59c Mon Sep 17 00:00:00 2001
From: Markus Koller <markus-koller@gmx.ch>
Date: Fri, 9 Dec 2016 18:37:18 +0100
Subject: Remove duplicated code in Oauth::AuthorizationsController

---
 app/controllers/oauth/authorizations_controller.rb | 44 +---------------------
 1 file changed, 2 insertions(+), 42 deletions(-)

diff --git a/app/controllers/oauth/authorizations_controller.rb b/app/controllers/oauth/authorizations_controller.rb
index c721dca58d9..05190103767 100644
--- a/app/controllers/oauth/authorizations_controller.rb
+++ b/app/controllers/oauth/authorizations_controller.rb
@@ -1,8 +1,8 @@
 class Oauth::AuthorizationsController < Doorkeeper::AuthorizationsController
-  before_action :authenticate_resource_owner!
-
   layout 'profile'
 
+  # Overriden from Doorkeeper::AuthorizationsController to
+  # include the call to session.delete
   def new
     if pre_auth.authorizable?
       if skip_authorization? || matching_token?
@@ -16,44 +16,4 @@ class Oauth::AuthorizationsController < Doorkeeper::AuthorizationsController
       render "doorkeeper/authorizations/error"
     end
   end
-
-  # TODO: Handle raise invalid authorization
-  def create
-    redirect_or_render authorization.authorize
-  end
-
-  def destroy
-    redirect_or_render authorization.deny
-  end
-
-  private
-
-  def matching_token?
-    Doorkeeper::AccessToken.matching_token_for(pre_auth.client,
-                                               current_resource_owner.id,
-                                               pre_auth.scopes)
-  end
-
-  def redirect_or_render(auth)
-    if auth.redirectable?
-      redirect_to auth.redirect_uri
-    else
-      render json: auth.body, status: auth.status
-    end
-  end
-
-  def pre_auth
-    @pre_auth ||=
-      Doorkeeper::OAuth::PreAuthorization.new(Doorkeeper.configuration,
-                                              server.client_via_uid,
-                                              params)
-  end
-
-  def authorization
-    @authorization ||= strategy.request
-  end
-
-  def strategy
-    @strategy ||= server.authorization_request(pre_auth.response_type)
-  end
 end
-- 
cgit v1.2.1


From 789db2cc19b20a4df8ff9f02dd1a771e2736d2fd Mon Sep 17 00:00:00 2001
From: Markus Koller <markus-koller@gmx.ch>
Date: Tue, 20 Dec 2016 15:15:46 +0100
Subject: Make sure scopes are loaded in admin OAuth application form

---
 app/controllers/admin/applications_controller.rb   |  2 +-
 .../admin/applications_controller_spec.rb          | 65 ++++++++++++++++++++++
 2 files changed, 66 insertions(+), 1 deletion(-)
 create mode 100644 spec/controllers/admin/applications_controller_spec.rb

diff --git a/app/controllers/admin/applications_controller.rb b/app/controllers/admin/applications_controller.rb
index 62f62e99a97..9c9f420c1e0 100644
--- a/app/controllers/admin/applications_controller.rb
+++ b/app/controllers/admin/applications_controller.rb
@@ -2,7 +2,7 @@ class Admin::ApplicationsController < Admin::ApplicationController
   include OauthApplications
 
   before_action :set_application, only: [:show, :edit, :update, :destroy]
-  before_action :load_scopes, only: [:new, :edit]
+  before_action :load_scopes, only: [:new, :create, :edit, :update]
 
   def index
     @applications = Doorkeeper::Application.where("owner_id IS NULL")
diff --git a/spec/controllers/admin/applications_controller_spec.rb b/spec/controllers/admin/applications_controller_spec.rb
new file mode 100644
index 00000000000..e311b8a63b2
--- /dev/null
+++ b/spec/controllers/admin/applications_controller_spec.rb
@@ -0,0 +1,65 @@
+require 'spec_helper'
+
+describe Admin::ApplicationsController do
+  let(:admin) { create(:admin) }
+  let(:application) { create(:oauth_application, owner_id: nil, owner_type: nil) }
+
+  before do
+    sign_in(admin)
+  end
+
+  describe 'GET #new' do
+    it 'renders the application form' do
+      get :new
+
+      expect(response).to render_template :new
+      expect(assigns[:scopes]).to be_kind_of(Doorkeeper::OAuth::Scopes)
+    end
+  end
+
+  describe 'GET #edit' do
+    it 'renders the application form' do
+      get :edit, id: application.id
+
+      expect(response).to render_template :edit
+      expect(assigns[:scopes]).to be_kind_of(Doorkeeper::OAuth::Scopes)
+    end
+  end
+
+  describe 'POST #create' do
+    it 'creates the application' do
+      expect do
+        post :create, doorkeeper_application: attributes_for(:application)
+      end.to change { Doorkeeper::Application.count }.by(1)
+
+      application = Doorkeeper::Application.last
+
+      expect(response).to redirect_to(admin_application_path(application))
+    end
+
+    it 'renders the application form on errors' do
+      expect do
+        post :create, doorkeeper_application: attributes_for(:application).merge(redirect_uri: nil)
+      end.not_to change { Doorkeeper::Application.count }
+
+      expect(response).to render_template :new
+      expect(assigns[:scopes]).to be_kind_of(Doorkeeper::OAuth::Scopes)
+    end
+  end
+
+  describe 'PATCH #update' do
+    it 'updates the application' do
+      patch :update, id: application.id, doorkeeper_application: { redirect_uri: 'http://example.com/' }
+
+      expect(response).to redirect_to(admin_application_path(application))
+      expect(application.reload.redirect_uri).to eq 'http://example.com/'
+    end
+
+    it 'renders the application form on errors' do
+      patch :update, id: application.id, doorkeeper_application: { redirect_uri: nil }
+
+      expect(response).to render_template :edit
+      expect(assigns[:scopes]).to be_kind_of(Doorkeeper::OAuth::Scopes)
+    end
+  end
+end
-- 
cgit v1.2.1


From 93daeee16428707fc348f8c45215854aed6e117a Mon Sep 17 00:00:00 2001
From: Markus Koller <markus-koller@gmx.ch>
Date: Wed, 18 Jan 2017 11:23:25 +0100
Subject: Don't allow blocked users to authenticate through other means

Gitlab::Auth.find_with_user_password is currently used in these places:

- resource_owner_from_credentials in config/initializers/doorkeeper.rb,
  which is used for the OAuth Resource Owner Password Credentials flow

- the /session API call in lib/api/session.rb, which is used to reveal
  the user's current authentication_token

In both cases users should only be authenticated if they're in the
active state.
---
 lib/gitlab/auth.rb                          |  2 +-
 spec/lib/gitlab/auth_spec.rb                | 12 ++++++++++++
 spec/requests/api/doorkeeper_access_spec.rb | 18 ++++++++++++++++++
 spec/requests/api/oauth_tokens_spec.rb      | 22 ++++++++++++++++++++++
 spec/requests/api/session_spec.rb           | 18 ++++++++++++++++++
 spec/requests/git_http_spec.rb              | 12 ++++++++++--
 6 files changed, 81 insertions(+), 3 deletions(-)

diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb
index 6166f8d0dd9..c6f9d0d7b82 100644
--- a/lib/gitlab/auth.rb
+++ b/lib/gitlab/auth.rb
@@ -40,7 +40,7 @@ module Gitlab
 
             Gitlab::LDAP::Authentication.login(login, password)
           else
-            user if user.valid_password?(password)
+            user if user.active? && user.valid_password?(password)
           end
         end
       end
diff --git a/spec/lib/gitlab/auth_spec.rb b/spec/lib/gitlab/auth_spec.rb
index daf8f5c1d6c..8726ca569ca 100644
--- a/spec/lib/gitlab/auth_spec.rb
+++ b/spec/lib/gitlab/auth_spec.rb
@@ -210,6 +210,18 @@ describe Gitlab::Auth, lib: true do
       end
     end
 
+    it "does not find user in blocked state" do
+      user.block
+
+      expect( gl_auth.find_with_user_password(username, password) ).not_to eql user
+    end
+
+    it "does not find user in ldap_blocked state" do
+      user.ldap_block
+
+      expect( gl_auth.find_with_user_password(username, password) ).not_to eql user
+    end
+
     context "with ldap enabled" do
       before do
         allow(Gitlab::LDAP::Config).to receive(:enabled?).and_return(true)
diff --git a/spec/requests/api/doorkeeper_access_spec.rb b/spec/requests/api/doorkeeper_access_spec.rb
index 2974875510a..f6fd567eca5 100644
--- a/spec/requests/api/doorkeeper_access_spec.rb
+++ b/spec/requests/api/doorkeeper_access_spec.rb
@@ -39,4 +39,22 @@ describe API::API, api: true do
       end
     end
   end
+
+  describe "when user is blocked" do
+    it "returns authentication error" do
+      user.block
+      get api("/user"), access_token: token.token
+
+      expect(response).to have_http_status(401)
+    end
+  end
+
+  describe "when user is ldap_blocked" do
+    it "returns authentication error" do
+      user.ldap_block
+      get api("/user"), access_token: token.token
+
+      expect(response).to have_http_status(401)
+    end
+  end
 end
diff --git a/spec/requests/api/oauth_tokens_spec.rb b/spec/requests/api/oauth_tokens_spec.rb
index 7e2cc50e591..367225df717 100644
--- a/spec/requests/api/oauth_tokens_spec.rb
+++ b/spec/requests/api/oauth_tokens_spec.rb
@@ -29,5 +29,27 @@ describe API::API, api: true  do
         expect(json_response['access_token']).not_to be_nil
       end
     end
+
+    context "when user is blocked" do
+      it "does not create an access token" do
+        user = create(:user)
+        user.block
+
+        request_oauth_token(user)
+
+        expect(response).to have_http_status(401)
+      end
+    end
+
+    context "when user is ldap_blocked" do
+      it "does not create an access token" do
+        user = create(:user)
+        user.ldap_block
+
+        request_oauth_token(user)
+
+        expect(response).to have_http_status(401)
+      end
+    end
   end
 end
diff --git a/spec/requests/api/session_spec.rb b/spec/requests/api/session_spec.rb
index 794e2b5c04d..28fab2011a5 100644
--- a/spec/requests/api/session_spec.rb
+++ b/spec/requests/api/session_spec.rb
@@ -87,5 +87,23 @@ describe API::Session, api: true  do
         expect(response).to have_http_status(400)
       end
     end
+
+    context "when user is blocked" do
+      it "returns authentication error" do
+        user.block
+        post api("/session"), email: user.username, password: user.password
+
+        expect(response).to have_http_status(401)
+      end
+    end
+
+    context "when user is ldap_blocked" do
+      it "returns authentication error" do
+        user.ldap_block
+        post api("/session"), email: user.username, password: user.password
+
+        expect(response).to have_http_status(401)
+      end
+    end
   end
 end
diff --git a/spec/requests/git_http_spec.rb b/spec/requests/git_http_spec.rb
index 87786e85621..006d6a6af1c 100644
--- a/spec/requests/git_http_spec.rb
+++ b/spec/requests/git_http_spec.rb
@@ -221,12 +221,20 @@ describe 'Git HTTP requests', lib: true do
               end
 
               context "when the user is blocked" do
-                it "responds with status 404" do
+                it "responds with status 401" do
                   user.block
                   project.team << [user, :master]
 
                   download(path, env) do |response|
-                    expect(response).to have_http_status(404)
+                    expect(response).to have_http_status(401)
+                  end
+                end
+
+                it "responds with status 401 for unknown projects (no project existence information leak)" do
+                  user.block
+
+                  download('doesnt/exist.git', env) do |response|
+                    expect(response).to have_http_status(401)
                   end
                 end
               end
-- 
cgit v1.2.1


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: Only use API scopes for personal access tokens

---
 .../profiles/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
-- 
cgit v1.2.1


From 8699c8338f21404aa08c9a141768201ed02b2c93 Mon Sep 17 00:00:00 2001
From: Markus Koller <markus-koller@gmx.ch>
Date: Mon, 6 Feb 2017 16:39:35 +0100
Subject: 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.
---
 app/models/personal_access_token.rb                |  7 +++---
 lib/gitlab/auth.rb                                 |  5 +++-
 .../profiles/personal_access_tokens_spec.rb        | 29 ++++++----------------
 spec/models/personal_access_token_spec.rb          | 14 ++++++++---
 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
-- 
cgit v1.2.1


From 972678b6bfe83bf21971b108b8f767ce37529916 Mon Sep 17 00:00:00 2001
From: Markus Koller <markus-koller@gmx.ch>
Date: Wed, 8 Feb 2017 19:41:27 +0100
Subject: Remove whitespace in Gemfile

---
 Gemfile | 102 ++++++++++++++++++++++++++++++++--------------------------------
 1 file changed, 51 insertions(+), 51 deletions(-)

diff --git a/Gemfile b/Gemfile
index b66cd38b4dc..2f813324a35 100644
--- a/Gemfile
+++ b/Gemfile
@@ -18,26 +18,26 @@ gem 'pg', '~> 0.18.2', group: :postgres
 gem 'rugged', '~> 0.24.0'
 
 # Authentication libraries
-gem 'devise',                 '~> 4.2'
-gem 'doorkeeper',             '~> 4.2.0'
+gem 'devise', '~> 4.2'
+gem 'doorkeeper', '~> 4.2.0'
 gem 'doorkeeper-openid_connect', '~> 1.1.0'
-gem 'omniauth',               '~> 1.4.2'
-gem 'omniauth-auth0',         '~> 1.4.1'
-gem 'omniauth-azure-oauth2',  '~> 0.0.6'
-gem 'omniauth-cas3',          '~> 1.1.2'
-gem 'omniauth-facebook',      '~> 4.0.0'
-gem 'omniauth-github',        '~> 1.1.1'
-gem 'omniauth-gitlab',        '~> 1.0.2'
+gem 'omniauth', '~> 1.4.2'
+gem 'omniauth-auth0', '~> 1.4.1'
+gem 'omniauth-azure-oauth2', '~> 0.0.6'
+gem 'omniauth-cas3', '~> 1.1.2'
+gem 'omniauth-facebook', '~> 4.0.0'
+gem 'omniauth-github', '~> 1.1.1'
+gem 'omniauth-gitlab', '~> 1.0.2'
 gem 'omniauth-google-oauth2', '~> 0.4.1'
-gem 'omniauth-kerberos',      '~> 0.3.0', group: :kerberos
+gem 'omniauth-kerberos', '~> 0.3.0', group: :kerberos
 gem 'omniauth-oauth2-generic', '~> 0.2.2'
-gem 'omniauth-saml',          '~> 1.7.0'
-gem 'omniauth-shibboleth',    '~> 1.2.0'
-gem 'omniauth-twitter',       '~> 1.2.0'
-gem 'omniauth_crowd',         '~> 2.2.0'
-gem 'omniauth-authentiq',     '~> 0.3.0'
-gem 'rack-oauth2',            '~> 1.2.1'
-gem 'jwt',                    '~> 1.5.6'
+gem 'omniauth-saml', '~> 1.7.0'
+gem 'omniauth-shibboleth', '~> 1.2.0'
+gem 'omniauth-twitter', '~> 1.2.0'
+gem 'omniauth_crowd', '~> 2.2.0'
+gem 'omniauth-authentiq', '~> 0.3.0'
+gem 'rack-oauth2', '~> 1.2.1'
+gem 'jwt', '~> 1.5.6'
 
 # Spam and anti-bot protection
 gem 'recaptcha', '~> 3.0', require: 'recaptcha/rails'
@@ -69,9 +69,9 @@ gem 'gollum-rugged_adapter', '~> 0.4.2', require: false
 gem 'github-linguist', '~> 4.7.0', require: 'linguist'
 
 # API
-gem 'grape',        '~> 0.19.0'
+gem 'grape', '~> 0.19.0'
 gem 'grape-entity', '~> 0.6.0'
-gem 'rack-cors',    '~> 0.4.0', require: 'rack/cors'
+gem 'rack-cors', '~> 0.4.0', require: 'rack/cors'
 
 # Pagination
 gem 'kaminari', '~> 0.17.0'
@@ -103,19 +103,19 @@ gem 'unf', '~> 0.1.4'
 gem 'seed-fu', '~> 2.3.5'
 
 # Markdown and HTML processing
-gem 'html-pipeline',        '~> 1.11.0'
-gem 'deckar01-task_list',   '1.0.6', require: 'task_list/railtie'
-gem 'gitlab-markup',        '~> 1.5.1'
-gem 'redcarpet',            '~> 3.4'
-gem 'RedCloth',             '~> 4.3.2'
-gem 'rdoc',                 '~> 4.2'
-gem 'org-ruby',             '~> 0.9.12'
-gem 'creole',               '~> 0.5.0'
-gem 'wikicloth',            '0.8.1'
-gem 'asciidoctor',          '~> 1.5.2'
+gem 'html-pipeline', '~> 1.11.0'
+gem 'deckar01-task_list', '1.0.6', require: 'task_list/railtie'
+gem 'gitlab-markup', '~> 1.5.1'
+gem 'redcarpet', '~> 3.4'
+gem 'RedCloth', '~> 4.3.2'
+gem 'rdoc', '~> 4.2'
+gem 'org-ruby', '~> 0.9.12'
+gem 'creole', '~> 0.5.0'
+gem 'wikicloth', '0.8.1'
+gem 'asciidoctor', '~> 1.5.2'
 gem 'asciidoctor-plantuml', '0.0.7'
-gem 'rouge',                '~> 2.0'
-gem 'truncato',             '~> 0.7.8'
+gem 'rouge', '~> 2.0'
+gem 'truncato', '~> 0.7.8'
 
 # See https://groups.google.com/forum/#!topic/ruby-security-ann/aSbgDiwb24s
 # and https://groups.google.com/forum/#!topic/ruby-security-ann/Dy7YiKb_pMM
@@ -230,18 +230,18 @@ gem 'sass-rails', '~> 5.0.6'
 gem 'coffee-rails', '~> 4.1.0'
 gem 'uglifier', '~> 2.7.2'
 
-gem 'addressable',        '~> 2.3.8'
-gem 'bootstrap-sass',     '~> 3.3.0'
+gem 'addressable', '~> 2.3.8'
+gem 'bootstrap-sass', '~> 3.3.0'
 gem 'font-awesome-rails', '~> 4.7'
-gem 'gemojione',          '~> 3.0'
-gem 'gon',                '~> 6.1.0'
+gem 'gemojione', '~> 3.0'
+gem 'gon', '~> 6.1.0'
 gem 'jquery-atwho-rails', '~> 1.3.2'
-gem 'jquery-rails',       '~> 4.1.0'
-gem 'request_store',      '~> 1.3'
-gem 'select2-rails',      '~> 3.5.9'
-gem 'virtus',             '~> 1.0.1'
-gem 'net-ssh',            '~> 3.0.1'
-gem 'base32',             '~> 0.3.0'
+gem 'jquery-rails', '~> 4.1.0'
+gem 'request_store', '~> 1.3'
+gem 'select2-rails', '~> 3.5.9'
+gem 'virtus', '~> 1.0.1'
+gem 'net-ssh', '~> 3.0.1'
+gem 'base32', '~> 0.3.0'
 
 # Sentry integration
 gem 'sentry-raven', '~> 2.0.0'
@@ -279,13 +279,13 @@ group :development, :test do
   gem 'awesome_print', '~> 1.2.0', require: false
   gem 'fuubar', '~> 2.0.0'
 
-  gem 'database_cleaner',   '~> 1.5.0'
+  gem 'database_cleaner', '~> 1.5.0'
   gem 'factory_girl_rails', '~> 4.7.0'
-  gem 'rspec-rails',        '~> 3.5.0'
-  gem 'rspec-retry',        '~> 0.4.5'
-  gem 'spinach-rails',      '~> 0.2.1'
+  gem 'rspec-rails', '~> 3.5.0'
+  gem 'rspec-retry', '~> 0.4.5'
+  gem 'spinach-rails', '~> 0.2.1'
   gem 'spinach-rerun-reporter', '~> 0.0.2'
-  gem 'rspec_profiling',    '~> 0.0.5'
+  gem 'rspec_profiling', '~> 0.0.5'
 
   # Prevent occasions where minitest is not bundled in packaged versions of ruby (see #3826)
   gem 'minitest', '~> 5.7.0'
@@ -293,13 +293,13 @@ group :development, :test do
   # Generate Fake data
   gem 'ffaker', '~> 2.4'
 
-  gem 'capybara',            '~> 2.6.2'
+  gem 'capybara', '~> 2.6.2'
   gem 'capybara-screenshot', '~> 1.0.0'
-  gem 'poltergeist',         '~> 1.9.0'
+  gem 'poltergeist', '~> 1.9.0'
 
-  gem 'spring',                   '~> 1.7.0'
-  gem 'spring-commands-rspec',    '~> 1.0.4'
-  gem 'spring-commands-spinach',  '~> 1.1.0'
+  gem 'spring', '~> 1.7.0'
+  gem 'spring-commands-rspec', '~> 1.0.4'
+  gem 'spring-commands-spinach', '~> 1.1.0'
 
   gem 'rubocop', '~> 0.47.1', require: false
   gem 'rubocop-rspec', '~> 1.12.0', require: false
-- 
cgit v1.2.1


From b2ca28d24bfbb0a574fccdf1ea05d549ccd6bf66 Mon Sep 17 00:00:00 2001
From: Markus Koller <markus-koller@gmx.ch>
Date: Wed, 8 Feb 2017 20:23:43 +0100
Subject: Add specs for Doorkeeper resource_owner_authenticator

---
 config/initializers/doorkeeper.rb    |  4 +--
 spec/initializers/doorkeeper_spec.rb | 67 +++++++++++++++++++++++++++++++++---
 2 files changed, 65 insertions(+), 6 deletions(-)

diff --git a/config/initializers/doorkeeper.rb b/config/initializers/doorkeeper.rb
index 52551a5d5eb..a5636765774 100644
--- a/config/initializers/doorkeeper.rb
+++ b/config/initializers/doorkeeper.rb
@@ -6,11 +6,11 @@ Doorkeeper.configure do
   # This block will be called to check whether the resource owner is authenticated or not.
   resource_owner_authenticator do
     # Put your resource owner authentication logic here.
-    # Ensure user is redirected to redirect_uri after login
-    session[:user_return_to] = request.fullpath
     if current_user
       current_user
     else
+      # Ensure user is redirected to redirect_uri after login
+      session[:user_return_to] = request.fullpath
       redirect_to(new_user_session_url)
       nil
     end
diff --git a/spec/initializers/doorkeeper_spec.rb b/spec/initializers/doorkeeper_spec.rb
index 32133edece7..74bdbb01166 100644
--- a/spec/initializers/doorkeeper_spec.rb
+++ b/spec/initializers/doorkeeper_spec.rb
@@ -2,11 +2,70 @@ 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
+  describe '#default_scopes' do
+    it 'matches Gitlab::Auth::DEFAULT_SCOPES' do
+      expect(subject.default_scopes).to eq Gitlab::Auth::DEFAULT_SCOPES
+    end
   end
 
-  it 'optional_scopes matches Gitlab::Auth::OPTIONAL_SCOPES' do
-    expect(subject.optional_scopes).to eq Gitlab::Auth::OPTIONAL_SCOPES
+  describe '#optional_scopes' do
+    it 'matches Gitlab::Auth::OPTIONAL_SCOPES' do
+      expect(subject.optional_scopes).to eq Gitlab::Auth::OPTIONAL_SCOPES
+    end
+  end
+
+  describe '#resource_owner_authenticator' do
+    subject { controller.instance_exec(&Doorkeeper.configuration.authenticate_resource_owner) }
+
+    let(:controller) { double }
+
+    before do
+      allow(controller).to receive(:current_user).and_return(current_user)
+      allow(controller).to receive(:session).and_return({})
+      allow(controller).to receive(:request).and_return(OpenStruct.new(fullpath: '/return-path'))
+      allow(controller).to receive(:redirect_to)
+      allow(controller).to receive(:new_user_session_url).and_return('/login')
+    end
+
+    context 'with a user present' do
+      let(:current_user) { create(:user) }
+
+      it 'returns the user' do
+        expect(subject).to eq current_user
+      end
+
+      it 'does not redirect' do
+        expect(controller).not_to receive(:redirect_to)
+
+        subject
+      end
+
+      it 'does not store the return path' do
+        subject
+
+        expect(controller.session).not_to include :user_return_to
+      end
+    end
+
+    context 'without a user present' do
+      let(:current_user) { nil }
+
+      # NOTE: this is required for doorkeeper-openid_connect
+      it 'returns nil' do
+        expect(subject).to eq nil
+      end
+
+      it 'redirects to the login form' do
+        expect(controller).to receive(:redirect_to).with('/login')
+
+        subject
+      end
+
+      it 'stores the return path' do
+        subject
+
+        expect(controller.session[:user_return_to]).to eq '/return-path'
+      end
+    end
   end
 end
-- 
cgit v1.2.1