DEV: Move UserApiKey scopes to dedicated table (#10704)

This has no functional impact yet, but it is the first step in adding more granular scopes to UserApiKeys

app/controllers/user_api_keys_controller.rb

@@ -71,7 +71,7 @@ class UserApiKeysController < ApplicationController
       client_id: params[:client_id],
       user_id: current_user.id,
       push_url: params[:push_url],
-      scopes: scopes
+      scopes: scopes.map { |name| UserApiKeyScope.new(name: name) }
     )
 
     # we keep the payload short so it encrypts easily with public key
@@ -147,9 +147,6 @@ class UserApiKeysController < ApplicationController
     if current_key = request.env['HTTP_USER_API_KEY']
       request_key = UserApiKey.with_key(current_key).first
       revoke_key ||= request_key
-      if request_key && request_key.id != revoke_key.id && !request_key.scopes.include?("write")
-        raise Discourse::InvalidAccess
-      end
     end
 
     raise Discourse::NotFound unless revoke_key

app/models/user_api_key.rb

@@ -1,6 +1,9 @@
 # frozen_string_literal: true
 
 class UserApiKey < ActiveRecord::Base
+  self.ignored_columns = [
+    "scopes" # TODO(2020-12-18): remove
+  ]
 
   SCOPES = {
     read: [:get],
@@ -19,6 +22,7 @@ class UserApiKey < ActiveRecord::Base
   }
 
   belongs_to :user
+  has_many :scopes, class_name: "UserApiKeyScope", dependent: :destroy
 
   scope :active, -> { where(revoked_at: nil) }
   scope :with_key, ->(key) { where(key_hash: ApiKey.hash_key(key)) }
@@ -41,6 +45,7 @@ class UserApiKey < ActiveRecord::Base
     @key.present?
   end
 
+  # Scopes allowed to be requested by external services
   def self.allowed_scopes
     Set.new(SiteSetting.allow_user_api_key_scopes.split("|"))
   end
@@ -78,13 +83,15 @@ class UserApiKey < ActiveRecord::Base
   end
 
   def has_push?
-    (scopes.include?("push") || scopes.include?("notifications")) && push_url.present? && SiteSetting.allowed_user_api_push_urls.include?(push_url)
+    scopes.any? { |s| s.name == "push" || s.name == "notifications" } &&
+      push_url.present? &&
+      SiteSetting.allowed_user_api_push_urls.include?(push_url)
   end
 
   def allow?(env)
-    scopes.any? do |name|
+    scopes.any? do |s|
-      UserApiKey.allow_scope?(name, env)
+      UserApiKey.allow_scope?(s.name, env)
-    end
+    end || is_revoke_self_request?(env)
   end
 
   def self.invalid_auth_redirect?(auth_redirect)
@@ -92,6 +99,12 @@ class UserApiKey < ActiveRecord::Base
       .split('|')
       .none? { |u| WildcardUrlChecker.check_url(u, auth_redirect) }
   end
+
+  private
+
+  def is_revoke_self_request?(env)
+    UserApiKey.allow_permission?([:post, 'user_api_keys#revoke'], env) && (env[:id].nil? || env[:id].to_i == id)
+  end
 end
 
 # == Schema Information

app/models/user_api_key_scope.rb

@@ -0,0 +1,19 @@
+# frozen_string_literal: true
+
+class UserApiKeyScope < ActiveRecord::Base
+end
+
+# == Schema Information
+#
+# Table name: user_api_key_scopes
+#
+#  id              :bigint           not null, primary key
+#  user_api_key_id :integer          not null
+#  name            :string           not null
+#  created_at      :datetime         not null
+#  updated_at      :datetime         not null
+#
+# Indexes
+#
+#  index_user_api_key_scopes_on_user_api_key_id  (user_api_key_id)
+#

app/services/post_alerter.rb

@@ -470,7 +470,8 @@ class PostAlerter
 
     if SiteSetting.allow_user_api_key_scopes.split("|").include?("push") && SiteSetting.allowed_user_api_push_urls.present?
       clients = user.user_api_keys
-        .where("('push' = ANY(scopes) OR 'notifications' = ANY(scopes))")
+        .joins(:scopes)
+        .where("user_api_key_scopes.name IN ('push', 'notifications')")
         .where("push_url IS NOT NULL AND push_url <> ''")
         .where("position(push_url IN ?) > 0", SiteSetting.allowed_user_api_push_urls)
         .where("revoked_at IS NULL")

db/migrate/20200918095554_add_user_api_key_scopes.rb

@@ -0,0 +1,46 @@
+# frozen_string_literal: true
+
+class AddUserApiKeyScopes < ActiveRecord::Migration[6.0]
+  def change
+    create_table :user_api_key_scopes do |t|
+      t.integer :user_api_key_id, null: false
+      t.string :name, null: false
+      t.timestamps
+    end
+
+    add_index :user_api_key_scopes, :user_api_key_id
+
+    reversible do |dir|
+      dir.up do
+        execute <<~SQL
+          INSERT INTO user_api_key_scopes
+          (
+            user_api_key_id,
+            name,
+            created_at,
+            updated_at
+          )
+          SELECT
+            user_api_keys.id,
+            unnest(user_api_keys.scopes),
+            created_at,
+            updated_at
+          FROM user_api_keys
+        SQL
+
+        Migration::SafeMigrate.disable!
+        change_column_null :user_api_keys, :scopes, true
+        change_column_default :user_api_keys, :scopes, nil
+        Migration::SafeMigrate.enable!
+
+        Migration::ColumnDropper.mark_readonly(:user_api_keys, :scopes)
+      end
+
+      dir.down do
+        change_column_null :user_api_keys, :scopes, false
+        change_column_default :user_api_keys, :scopes, []
+        Migration::ColumnDropper.drop_readonly(:user_api_keys, :scopes)
+      end
+    end
+  end
+end

lib/auth/default_current_user_provider.rb

@@ -307,7 +307,7 @@ class Auth::DefaultCurrentUserProvider
   protected
 
   def lookup_user_api_user_and_update_key(user_api_key, client_id)
-    if api_key = UserApiKey.active.with_key(user_api_key).includes(:user).first
+    if api_key = UserApiKey.active.with_key(user_api_key).includes(:user, :scopes).first
       unless api_key.allow?(@env)
         raise Discourse::InvalidAccess
       end

spec/components/auth/default_current_user_provider_spec.rb

@@ -547,7 +547,7 @@ describe Auth::DefaultCurrentUserProvider do
       UserApiKey.create!(
         application_name: 'my app',
         client_id: '1234',
-        scopes: ['read'],
+        scopes: ['read'].map { |name| UserApiKeyScope.new(name: name) },
         user_id: user.id
       )
     end
@@ -556,7 +556,7 @@ describe Auth::DefaultCurrentUserProvider do
       dupe = UserApiKey.create!(
         application_name: 'my app',
         client_id: '12345',
-        scopes: ['read'],
+        scopes: ['read'].map { |name| UserApiKeyScope.new(name: name) },
         user_id: user.id
       )
 

spec/fabricators/user_api_key_fabricator.rb

@@ -1,8 +1,10 @@
 # frozen_string_literal: true
 
+Fabricator(:user_api_key_scope)
+
 Fabricator(:readonly_user_api_key, from: :user_api_key) do
   user
-  scopes ['read']
+  scopes { [Fabricate.build(:user_api_key_scope, name: 'read')] }
   client_id { SecureRandom.hex }
   application_name 'some app'
 end

spec/models/user_api_key_spec.rb

@@ -5,7 +5,7 @@ require 'rails_helper'
 describe UserApiKey do
   context "#allow?" do
     it "can look up permissions correctly" do
-      key = UserApiKey.new(scopes: ['message_bus', 'notifications'])
+      key = UserApiKey.new(scopes: ['message_bus', 'notifications'].map { |name| UserApiKeyScope.new(name: name) })
 
       expect(key.allow?("PATH_INFO" => "/random", "REQUEST_METHOD" => "GET")).to eq(false)
       expect(key.allow?("PATH_INFO" => "/message-bus/1234/poll", "REQUEST_METHOD" => "POST")).to eq(true)
@@ -20,7 +20,7 @@ describe UserApiKey do
 
     it "can allow all correct scopes to write" do
 
-      key = UserApiKey.new(scopes: ["write"])
+      key = UserApiKey.new(scopes: ["write"].map { |name| UserApiKeyScope.new(name: name) })
 
       expect(key.allow?("PATH_INFO" => "/random", "REQUEST_METHOD" => "GET")).to eq(true)
       expect(key.allow?("PATH_INFO" => "/random", "REQUEST_METHOD" => "PUT")).to eq(true)
@@ -31,7 +31,7 @@ describe UserApiKey do
 
     it "can allow blanket read" do
 
-      key = UserApiKey.new(scopes: ["read"])
+      key = UserApiKey.new(scopes: ["read"].map { |name| UserApiKeyScope.new(name: name) })
 
       expect(key.allow?("PATH_INFO" => "/random", "REQUEST_METHOD" => "GET")).to eq(true)
       expect(key.allow?("PATH_INFO" => "/random", "REQUEST_METHOD" => "PUT")).to eq(false)

spec/requests/user_api_keys_controller_spec.rb

@@ -174,7 +174,7 @@ describe UserApiKeysController do
       expect(parsed["api"]).to eq(4)
 
       key = user.user_api_keys.first
-      expect(key.scopes).to include("push")
+      expect(key.scopes.map(&:name)).to include("push")
       expect(key.push_url).to eq("https://push.it/here")
     end
 
@@ -208,7 +208,7 @@ describe UserApiKeysController do
       api_key = UserApiKey.with_key(parsed["key"]).first
 
       expect(api_key.user_id).to eq(user.id)
-      expect(api_key.scopes.sort).to eq(["push", "message_bus", "notifications", "session_info", "one_time_password"].sort)
+      expect(api_key.scopes.map(&:name).sort).to eq(["push", "message_bus", "notifications", "session_info", "one_time_password"].sort)
       expect(api_key.push_url).to eq("https://push.it/here")
 
       uri.query = ""

spec/services/post_alerter_spec.rb

@@ -724,7 +724,7 @@ describe PostAlerter do
         UserApiKey.create!(user_id: evil_trout.id,
                            client_id: "xxx#{i}",
                            application_name: "iPhone#{i}",
-                           scopes: ['notifications'],
+                           scopes: ['notifications'].map { |name| UserApiKeyScope.new(name: name) },
                            push_url: "https://site2.com/push")
       end
 
@@ -739,7 +739,7 @@ describe PostAlerter do
         UserApiKey.create!(user_id: evil_trout.id,
                            client_id: "xxx#{i}",
                            application_name: "iPhone#{i}",
-                           scopes: ['notifications'],
+                           scopes: ['notifications'].map { |name| UserApiKeyScope.new(name: name) },
                            push_url: "https://site2.com/push")
       end