Merge pull request #1031 from f3ndot/support-app-confidentiality

Allow public clients to authenticate without client_secret

.rubocop.yml

@@ -2,6 +2,10 @@ AllCops:
   Exclude:
     - "spec/dummy/db/*"
 
+Metrics/BlockLength:
+  Exclude:
+    - spec/**/*
+
 LineLength:
   Exclude:
     - spec/**/*

app/controllers/doorkeeper/applications_controller.rb

@@ -57,7 +57,8 @@ def set_application
     end
 
     def application_params
-      params.require(:doorkeeper_application).permit(:name, :redirect_uri, :scopes)
+      params.require(:doorkeeper_application).
+        permit(:name, :redirect_uri, :scopes, :confidential)
     end
   end
 end

app/views/doorkeeper/applications/_form.html.erb

@@ -27,6 +27,17 @@
     </div>
   <% end %>
 
+  <%= content_tag :div, class: "form-group#{' has-error' if application.errors[:confidential].present?}" do %>
+    <%= f.label :confidential, class: 'col-sm-2 control-label' %>
+    <div class="col-sm-10">
+      <%= f.check_box :confidential, class: 'form-control' %>
+      <%= doorkeeper_errors_for application, :confidential %>
+      <span class="help-block">
+        <%= t('doorkeeper.applications.help.confidential') %>
+      </span>
+    </div>
+  <% end %>
+
   <%= content_tag :div, class: "form-group#{' has-error' if application.errors[:scopes].present?}" do %>
     <%= f.label :scopes, class: 'col-sm-2 control-label' %>
     <div class="col-sm-10">

app/views/doorkeeper/applications/index.html.erb

@@ -9,6 +9,7 @@
   <tr>
     <th><%= t('.name') %></th>
     <th><%= t('.callback_url') %></th>
+    <th><%= t('.confidential') %></th>
     <th></th>
     <th></th>
   </tr>
@@ -18,6 +19,7 @@
     <tr id="application_<%= application.id %>">
       <td><%= link_to application.name, oauth_application_path(application) %></td>
       <td><%= application.redirect_uri %></td>
+      <td><%= application.confidential? ? t('doorkeeper.applications.index.confidentiality.yes') : t('doorkeeper.applications.index.confidentiality.no') %></td>
       <td><%= link_to t('doorkeeper.applications.buttons.edit'), edit_oauth_application_path(application), class: 'btn btn-link' %></td>
       <td><%= render 'delete_form', application: application %></td>
     </tr>

app/views/doorkeeper/applications/show.html.erb

@@ -13,6 +13,9 @@
     <h4><%= t('.scopes') %>:</h4>
     <p><code id="scopes"><%= @application.scopes %></code></p>
 
+    <h4><%= t('.confidential') %>:</h4>
+    <p><code id="confidential"><%= @application.confidential? %></code></p>
+
     <h4><%= t('.callback_urls') %>:</h4>
 
     <table>

config/locales/en.yml

@@ -28,6 +28,7 @@ en:
       form:
         error: 'Whoops! Check your form for possible errors'
       help:
+        confidential: 'Application will be used where the client secret can be kept confidential. Native mobile apps and Single Page Apps are considered non-confidential.'
         redirect_uri: 'Use one line per URI'
         native_redirect_uri: 'Use %{native_redirect_uri} if you want to add localhost URIs for development purposes'
         scopes: 'Separate scopes with spaces. Leave blank to use the default scopes.'
@@ -38,13 +39,18 @@ en:
         new: 'New Application'
         name: 'Name'
         callback_url: 'Callback URL'
+        confidential: 'Confidential?'
+        confidentiality:
+          yes: 'Yes'
+          no: 'No'
       new:
         title: 'New Application'
       show:
         title: 'Application: %{name}'
         application_id: 'Application Id'
         secret: 'Secret'
         scopes: 'Scopes'
+        confidential: 'Confidential'
         callback_urls: 'Callback urls'
         actions: 'Actions'
 

lib/doorkeeper/models/application_mixin.rb

@@ -10,14 +10,21 @@ module ClassMethods
       # Returns an instance of the Doorkeeper::Application with
       # specific UID and secret.
       #
+      # Public/Non-confidential applications will only find by uid if secret is
+      # blank.
+      #
       # @param uid [#to_s] UID (any object that responds to `#to_s`)
       # @param secret [#to_s] secret (any object that responds to `#to_s`)
       #
       # @return [Doorkeeper::Application, nil] Application instance or nil
       #   if there is no record with such credentials
       #
       def by_uid_and_secret(uid, secret)
-        find_by(uid: uid.to_s, secret: secret.to_s)
+        app = by_uid(uid)
+        return unless app
+        return app if secret.blank? && !app.confidential?
+        return unless app.secret == secret
+        app
       end
 
       # Returns an instance of the Doorkeeper::Application with specific UID.

lib/doorkeeper/oauth/client/credentials.rb

@@ -23,8 +23,10 @@ def from_basic(request)
           end
         end
 
+        # Public clients may have their secret blank, but "credentials" are
+        # still present
         def blank?
-          uid.blank? || secret.blank?
+          uid.blank?
         end
       end
     end

lib/doorkeeper/orm/active_record/application.rb

@@ -11,6 +11,7 @@ class Application < ActiveRecord::Base
     validates :name, :secret, :uid, presence: true
     validates :uid, uniqueness: true
     validates :redirect_uri, redirect_uri: true
+    validates :confidential, inclusion: { in: [true, false] }
 
     before_validation :generate_uid, :generate_secret, on: :create
 

lib/doorkeeper/request/password.rb

@@ -3,7 +3,7 @@
 module Doorkeeper
   module Request
     class Password < Strategy
-      delegate :credentials, :resource_owner, :parameters, to: :server
+      delegate :credentials, :resource_owner, :parameters, :client, to: :server
 
       def request
         @request ||= OAuth::PasswordAccessTokenRequest.new(
@@ -13,16 +13,6 @@ def request
           parameters
         )
       end
-
-      private
-
-      def client
-        if credentials
-          server.client
-        elsif parameters[:client_id]
-          server.client_via_uid
-        end
-      end
     end
   end
 end

lib/generators/doorkeeper/templates/migration.rb.erb

@@ -6,6 +6,7 @@ class CreateDoorkeeperTables < ActiveRecord::Migration<%= migration_version %>
       t.string  :secret,       null: false
       t.text    :redirect_uri, null: false
       t.string  :scopes,       null: false, default: ''
+      t.boolean :confidential, null: false, default: true
       t.timestamps             null: false
     end
 

spec/dummy/db/migrate/20111122132257_create_users.rb

@@ -1,4 +1,6 @@
-class CreateUsers < ActiveRecord::Migration
+# frozen_string_literal: true
+
+class CreateUsers < ActiveRecord::Migration[4.2]
   def change
     create_table :users do |t|
       t.string :name

spec/dummy/db/migrate/20120312140401_add_password_to_users.rb

@@ -1,4 +1,6 @@
-class AddPasswordToUsers < ActiveRecord::Migration
+# frozen_string_literal: true
+
+class AddPasswordToUsers < ActiveRecord::Migration[4.2]
   def change
     add_column :users, :password, :string
   end

spec/dummy/db/migrate/20151223192035_create_doorkeeper_tables.rb

@@ -1,4 +1,6 @@
-class CreateDoorkeeperTables < ActiveRecord::Migration
+# frozen_string_literal: true
+
+class CreateDoorkeeperTables < ActiveRecord::Migration[4.2]
   def change
     create_table :oauth_applications do |t|
       t.string  :name,         null: false

spec/dummy/db/migrate/20151223200000_add_owner_to_application.rb

@@ -1,4 +1,6 @@
-class AddOwnerToApplication < ActiveRecord::Migration
+# frozen_string_literal: true
+
+class AddOwnerToApplication < ActiveRecord::Migration[4.2]
   def change
     add_column :oauth_applications, :owner_id, :integer, null: true
     add_column :oauth_applications, :owner_type, :string, null: true

spec/dummy/db/migrate/20160320211015_add_previous_refresh_token_to_access_tokens.rb

@@ -1,4 +1,6 @@
-class AddPreviousRefreshTokenToAccessTokens < ActiveRecord::Migration
+# frozen_string_literal: true
+
+class AddPreviousRefreshTokenToAccessTokens < ActiveRecord::Migration[4.2]
   def change
     add_column(
       :oauth_access_tokens,

spec/dummy/db/migrate/20180210183654_add_confidential_to_application.rb

@@ -0,0 +1,13 @@
+# frozen_string_literal: true
+
+class AddConfidentialToApplication < ActiveRecord::Migration[5.1]
+  def change
+    add_column(
+      :oauth_applications,
+      :confidential,
+      :boolean,
+      null: false,
+      default: true # maintaining backwards compatibility: require secrets
+    )
+  end
+end

spec/dummy/db/schema.rb

@@ -1,4 +1,3 @@
-# encoding: UTF-8
 # This file is auto-generated from the current state of the database. Instead
 # of editing this file, please use the migrations feature of Active Record to
 # incrementally modify your database, and then regenerate this schema definition.
@@ -11,61 +10,59 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20170822064514) do
+ActiveRecord::Schema.define(version: 20180210183654) do
 
   create_table "oauth_access_grants", force: :cascade do |t|
-    t.integer  "resource_owner_id",     null: false
-    t.integer  "application_id",        null: false
-    t.string   "token",                 null: false
-    t.integer  "expires_in",            null: false
-    t.text     "redirect_uri",          null: false
-    t.datetime "created_at",            null: false
+    t.integer "resource_owner_id", null: false
+    t.integer "application_id", null: false
+    t.string "token", null: false
+    t.integer "expires_in", null: false
+    t.text "redirect_uri", null: false
+    t.datetime "created_at", null: false
     t.datetime "revoked_at"
-    t.string   "scopes"
+    t.string "scopes"
     unless ENV['WITHOUT_PKCE']
       t.string   "code_challenge"
       t.string   "code_challenge_method"
     end
+    t.index ["token"], name: "index_oauth_access_grants_on_token", unique: true
   end
 
-  add_index "oauth_access_grants", ["token"], name: "index_oauth_access_grants_on_token", unique: true
-
   create_table "oauth_access_tokens", force: :cascade do |t|
-    t.integer  "resource_owner_id"
-    t.integer  "application_id"
-    t.string   "token",                               null: false
-    t.string   "refresh_token"
-    t.integer  "expires_in"
+    t.integer "resource_owner_id"
+    t.integer "application_id"
+    t.string "token", null: false
+    t.string "refresh_token"
+    t.integer "expires_in"
     t.datetime "revoked_at"
-    t.datetime "created_at",                          null: false
-    t.string   "scopes"
-    t.string   "previous_refresh_token", default: "", null: false
+    t.datetime "created_at", null: false
+    t.string "scopes"
+    t.string "previous_refresh_token", default: "", null: false
+    t.index ["refresh_token"], name: "index_oauth_access_tokens_on_refresh_token", unique: true
+    t.index ["resource_owner_id"], name: "index_oauth_access_tokens_on_resource_owner_id"
+    t.index ["token"], name: "index_oauth_access_tokens_on_token", unique: true
   end
 
-  add_index "oauth_access_tokens", ["refresh_token"], name: "index_oauth_access_tokens_on_refresh_token", unique: true
-  add_index "oauth_access_tokens", ["resource_owner_id"], name: "index_oauth_access_tokens_on_resource_owner_id"
-  add_index "oauth_access_tokens", ["token"], name: "index_oauth_access_tokens_on_token", unique: true
-
   create_table "oauth_applications", force: :cascade do |t|
-    t.string   "name",                      null: false
-    t.string   "uid",                       null: false
-    t.string   "secret",                    null: false
-    t.text     "redirect_uri",              null: false
-    t.string   "scopes",       default: "", null: false
-    t.datetime "created_at",                null: false
-    t.datetime "updated_at",                null: false
-    t.integer  "owner_id"
-    t.string   "owner_type"
+    t.string "name", null: false
+    t.string "uid", null: false
+    t.string "secret", null: false
+    t.text "redirect_uri", null: false
+    t.string "scopes", default: "", null: false
+    t.datetime "created_at", null: false
+    t.datetime "updated_at", null: false
+    t.integer "owner_id"
+    t.string "owner_type"
+    t.boolean "confidential", default: true, null: false
+    t.index ["owner_id", "owner_type"], name: "index_oauth_applications_on_owner_id_and_owner_type"
+    t.index ["uid"], name: "index_oauth_applications_on_uid", unique: true
   end
 
-  add_index "oauth_applications", ["owner_id", "owner_type"], name: "index_oauth_applications_on_owner_id_and_owner_type"
-  add_index "oauth_applications", ["uid"], name: "index_oauth_applications_on_uid", unique: true
-
   create_table "users", force: :cascade do |t|
-    t.string   "name"
+    t.string "name"
     t.datetime "created_at"
     t.datetime "updated_at"
-    t.string   "password"
+    t.string "password"
   end
 
 end

spec/lib/oauth/client/credentials_spec.rb

@@ -7,9 +7,11 @@ class Doorkeeper::OAuth::Client
     let(:client_id) { 'some-uid' }
     let(:client_secret) { 'some-secret' }
 
-    it 'is blank when any of the credentials is blank' do
+    it 'is blank when the uid in credentials is blank' do
+      expect(Credentials.new(nil, nil)).to be_blank
       expect(Credentials.new(nil, 'something')).to be_blank
-      expect(Credentials.new('something', nil)).to be_blank
+      expect(Credentials.new('something', nil)).to be_present
+      expect(Credentials.new('something', 'something')).to be_present
     end
 
     describe :from_request do