FEATURE: user API now contains scopes so permission is granular

previously we supported blanket read and write for user API, this change amends it so we can define more limited scopes. A scope only covers a few routes. You can not grant access to part of the site and leave a large amount of the information hidden to API consumer.
2016-10-14 16:05:27 +11:00 · 2016-10-14 16:05:27 +11:00 · f4f5524190
parent becff2de4d
commit f4f5524190
16 changed files with 164 additions and 75 deletions
--- a/app/assets/javascripts/discourse/templates/user/preferences.hbs
+++ b/app/assets/javascripts/discourse/templates/user/preferences.hbs
@ -336,7 +336,13 @@
          {{else}}
            {{d-button action="revokeApiKey" actionParam=key class="btn" label="user.revoke_access"}}
          {{/if}}
-          <p><span>{{i18n "user.api_permissions"}}</span> {{#if key.write}}{{i18n "user.api_read_write"}}{{else}}{{i18n "user.api_read"}}{{/if}}</p>
+          <p>
+            <ul>
+              {{#each key.scopes as |scope|}}
+              <li>{{scope}}</li>
+              {{/each}}
+            </ul>
+          </p>
          <p><span>{{i18n "user.api_approved"}}</span> {{bound-date key.created_at}}</p>
        </div>
        {{/each}}
--- a/app/controllers/user_api_keys_controller.rb
+++ b/app/controllers/user_api_keys_controller.rb
@ -6,7 +6,7 @@ class UserApiKeysController < ApplicationController
  skip_before_filter :check_xhr, :preload_json
  before_filter :ensure_logged_in, only: [:create, :revoke, :undo_revoke]

-  AUTH_API_VERSION ||= 1
+  AUTH_API_VERSION ||= 2

  def new

@ -34,14 +34,14 @@ class UserApiKeysController < ApplicationController
      return
    end

-    @access_description = params[:access].include?("w") ? t("user_api_key.read_write") : t("user_api_key.read")
    @application_name = params[:application_name]
    @public_key = params[:public_key]
    @nonce = params[:nonce]
-    @access = params[:access]
    @client_id = params[:client_id]
    @auth_redirect = params[:auth_redirect]
    @push_url = params[:push_url]
+    @localized_scopes = params[:scopes].split(",").map{|s| I18n.t("user_api_key.scopes.#{s}")}
+    @scopes = params[:scopes]

  rescue Discourse::InvalidAccess
    @generic_error = true
@ -60,10 +60,6 @@ class UserApiKeysController < ApplicationController

    raise Discourse::InvalidAccess unless meets_tl?

-    request_read = params[:access].include? 'r'
-    request_read ||= params[:access].include? 'p'
-    request_write = params[:access].include? 'w'
-
    validate_params

    # destroy any old keys we had
@ -72,12 +68,10 @@ class UserApiKeysController < ApplicationController
    key = UserApiKey.create!(
      application_name: params[:application_name],
      client_id: params[:client_id],
-      read: request_read,
-      push: params[:push_url].present?,
      user_id: current_user.id,
-      write: request_write,
+      push_url: params[:push_url],
      key: SecureRandom.hex,
-      push_url: params[:push_url]
+      scopes: params[:scopes].split(",")
    )

    # we keep the payload short so it encrypts easily with public key
@ -85,7 +79,8 @@ class UserApiKeysController < ApplicationController
    payload = {
      key: key.key,
      nonce: params[:nonce],
-      access: key.access
+      push: key.has_push?,
+      api: AUTH_API_VERSION
    }.to_json

    public_key = OpenSSL::PKey::RSA.new(params[:public_key])
@ -100,7 +95,7 @@ class UserApiKeysController < ApplicationController
    if current_key = request.env['HTTP_USER_API_KEY']
      request_key = UserApiKey.find_by(key: current_key)
      revoke_key ||= request_key
-      if request_key && request_key.id != revoke_key.id && !request_key.write
+      if request_key && request_key.id != revoke_key.id && !request_key.scopes.include?("write")
        raise Discourse::InvalidAccess
      end
    end
@ -127,7 +122,7 @@ class UserApiKeysController < ApplicationController
    [
     :public_key,
     :nonce,
-     :access,
+     :scopes,
     :client_id,
     :auth_redirect,
     :application_name
@ -135,13 +130,9 @@ class UserApiKeysController < ApplicationController
  end

  def validate_params
-    request_read = params[:access].include? 'r'
-    request_read ||= params[:access].include? 'p'
-    request_write = params[:access].include? 'w'
+    requested_scopes = Set.new(params[:scopes].split(","))

-    raise Discourse::InvalidAccess unless request_read || request_push
-    raise Discourse::InvalidAccess if request_read && !SiteSetting.allow_read_user_api_keys
-    raise Discourse::InvalidAccess if request_write && !SiteSetting.allow_write_user_api_keys
+    raise Discourse::InvalidAccess unless UserApiKey.allowed_scopes.superset?(requested_scopes)

    # our pk has got to parse
    OpenSSL::PKey::RSA.new(params[:public_key])
--- a/app/models/user_api_key.rb
+++ b/app/models/user_api_key.rb
@ -1,10 +1,63 @@
 class UserApiKey < ActiveRecord::Base
+
+  SCOPES = {
+    read: [:get],
+    write: [:get, :post, :patch],
+    message_bus: [[:post, 'message_bus']],
+    push: nil,
+    notifications: [[:post, 'message_bus'], [:get, 'notifications#index'], [:put, 'notifications#mark_read']],
+    session_info: [[:get, 'session#current'], [:get, 'users#topic_tracking_state']]
+  }
+
  belongs_to :user

-  def access
-    has_push = push && push_url.present? && SiteSetting.allowed_user_api_push_urls.include?(push_url)
-    "#{read ? "r" : ""}#{write ? "w" : ""}#{has_push ? "p" : ""}"
+  def self.allowed_scopes
+    Set.new(SiteSetting.allow_user_api_key_scopes.split("|"))
  end
+
+  def self.available_scopes
+    @available_scopes ||= Set.new(SCOPES.keys.map(&:to_s))
+  end
+
+  def self.allow_permission?(permission, env)
+    verb, action = permission
+    actual_verb = env["REQUEST_METHOD"] || ""
+
+    # safe in Ruby 2.3 which is only one supported
+    return false unless actual_verb.downcase == verb.to_s
+    return true unless action
+
+    # not a rails route, special handling
+    return true if action == "message_bus" && env["PATH_INFO"] =~ /^\/message-bus\/.*\/poll/
+
+    params  = env['action_dispatch.request.path_parameters']
+
+    return false unless params
+
+    actual_action = "#{params[:controller]}##{params[:action]}"
+    actual_action == action
+  end
+
+  def self.allow_scope?(name, env)
+    if allowed = SCOPES[name.to_sym]
+      good = allowed.any? do |permission|
+        allow_permission?(permission, env)
+      end
+
+      good || allow_permission?([:post, 'user_api_keys#revoke'], env)
+    end
+  end
+
+  def has_push?
+    (scopes.include?("push") || scopes.include?("notifications")) && push_url.present? && SiteSetting.allowed_user_api_push_urls.include?(push_url)
+  end
+
+  def allow?(env)
+    scopes.any? do |name|
+      UserApiKey.allow_scope?(name, env)
+    end
+  end
+
 end

 # == Schema Information
--- a/app/serializers/user_serializer.rb
+++ b/app/serializers/user_serializer.rb
@ -148,8 +148,7 @@ class UserSerializer < BasicUserSerializer
      {
        id: k.id,
        application_name: k.application_name,
-        read: k.read,
-        write: k.write,
+        scopes: k.scopes.map{|s| I18n.t("user_api_key.scopes.#{s}")},
        created_at: k.created_at
      }
    end
--- a/app/services/post_alerter.rb
+++ b/app/services/post_alerter.rb
@ -396,9 +396,9 @@ class PostAlerter
  end

  def push_notification(user, payload)
-    if SiteSetting.allow_push_user_api_keys && SiteSetting.allowed_user_api_push_urls.present?
+    if SiteSetting.allow_user_api_key_scopes.split("|").include?("push") && SiteSetting.allowed_user_api_push_urls.present?
      clients = user.user_api_keys
-          .where('push AND push_url IS NOT NULL AND position(push_url in ?) > 0 AND revoked_at IS NULL',
+          .where("'push' = ANY(scopes) AND push_url IS NOT NULL AND position(push_url in ?) > 0 AND revoked_at IS NULL",
                  SiteSetting.allowed_user_api_push_urls)
          .pluck(:client_id, :push_url)

--- a/app/views/user_api_keys/new.html.erb
+++ b/app/views/user_api_keys/new.html.erb
@ -1,5 +1,5 @@
 <h1><%= t "user_api_key.title" %></h1>
-<div>
+<div class='authorize-api-key'>
 <% if @no_trust_level %>
  <h3>
  <%= t("user_api_key.no_trust_level") %>
@ -10,7 +10,14 @@
  </h3>
 <% else %>
 <p>
-  <%= t("user_api_key.description", application_name: @application_name, access: @access_description) %>
+  <%= t("user_api_key.description", application_name: @application_name) %>
+</p>
+<p>
+  <ul class='scopes'>
+    <%- @localized_scopes.each do |scope| %>
+      <li><%= scope %></li>
+    <%- end %>
+  </ul>
 </p>
 <%= form_tag(user_api_key_path) do %>
  <%= hidden_field_tag 'application_name', @application_name %>
@ -20,6 +27,7 @@
  <%= hidden_field_tag 'auth_redirect', @auth_redirect %>
  <%= hidden_field_tag 'push_url', @push_url %>
  <%= hidden_field_tag 'public_key', @public_key%>
+  <%= hidden_field_tag 'scopes', @scopes%>
  <%= submit_tag t('user_api_key.authorize'), class: 'btn btn-danger', id: 'submit' %>
 <% end %>
 <script>
--- a/config/locales/client.en.yml
+++ b/config/locales/client.en.yml
@ -570,10 +570,7 @@ en:
      apps: "Apps"
      revoke_access: "Revoke Access"
      undo_revoke_access: "Undo Revoke Access"
-      api_permissions: "Permissions:"
      api_approved: "Approved:"
-      api_read: "read"
-      api_read_write: "read and write"

      staff_counters:
        flags_given: "helpful flags"
--- a/config/locales/server.en.yml
+++ b/config/locales/server.en.yml
@ -650,9 +650,16 @@ en:
    authorize: "Authorize"
    read: "read"
    read_write: "read/write"
-    description: "Would you like to grant \"%{application_name}\" %{access} access to your account?"
+    description: "\"%{application_name}\" is requesting the following access to your account:"
    no_trust_level: "Sorry, you do not have the required trust level to access the user API"
    generic_error: "Sorry, we are unable to issue user API keys, this feature may be disabled by the site admin"
+    scopes:
+      message_bus: "Live updates"
+      notifications: "Read and clear notifications"
+      push: "Push notifications to external services"
+      session_info: "Read user session info"
+      read: "Read all"
+      write: "Write all"

  reports:
    visits:
@ -1387,9 +1394,8 @@ en:

    max_user_api_reqs_per_day: "Maximum number of user API requests per key per day"
    max_user_api_reqs_per_minute: "Maximum number of user API requests per key per minute"
-    allow_read_user_api_keys: "Allow generation of readonly user API keys"
-    allow_write_user_api_keys: "Allow generation of write user API keys"
-    allow_push_user_api_keys: "Allow generation of push user API keys"
+    allow_user_api_keys: "Allow generation of user API keys"
+    allow_user_api_key_scopes: "List of scopes allowed for user API keys"
    max_api_keys_per_user: "Maximum number of user API keys per user"
    min_trust_level_for_user_api_key: "Trust level required for generation of user API keys"
    allowed_user_api_auth_redirects: "Allowed URL for authentication redirect for user API keys"
--- a/config/site_settings.yml
+++ b/config/site_settings.yml
@ -1287,12 +1287,11 @@ user_api:
    default: 2880
  max_user_api_reqs_per_minute:
    default: 20
-  allow_read_user_api_keys:
-    default: true
-  allow_write_user_api_keys:
-    default: true
-  allow_push_user_api_keys:
+  allow_user_api_keys:
    default: true
+  allow_user_api_key_scopes:
+    default: 'read|write|message_bus|push|notifications|session_info'
+    type: list
  max_api_keys_per_user:
    default: 10
  push_api_secret_key:
--- a/db/migrate/20161013012136_add_scopes_to_user_api_keys.rb
+++ b/db/migrate/20161013012136_add_scopes_to_user_api_keys.rb
@ -0,0 +1,13 @@
+class AddScopesToUserApiKeys < ActiveRecord::Migration
+  def change
+    add_column :user_api_keys, :scopes, :text, array: true, null: false, default: []
+
+    execute "UPDATE user_api_keys SET scopes = scopes || ARRAY['write'] WHERE write"
+    execute "UPDATE user_api_keys SET scopes = scopes || ARRAY['read'] WHERE read"
+    execute "UPDATE user_api_keys SET scopes = scopes || ARRAY['push'] WHERE push"
+
+    remove_column :user_api_keys, :read
+    remove_column :user_api_keys, :write
+    remove_column :user_api_keys, :push
+  end
+end
--- a/lib/auth/default_current_user_provider.rb
+++ b/lib/auth/default_current_user_provider.rb
@ -6,6 +6,7 @@ class Auth::DefaultCurrentUserProvider
  CURRENT_USER_KEY ||= "_DISCOURSE_CURRENT_USER".freeze
  API_KEY ||= "api_key".freeze
  USER_API_KEY ||= "HTTP_USER_API_KEY".freeze
+  USER_API_CLIENT_ID ||= "HTTP_USER_API_CLIENT_ID".freeze
  API_KEY_ENV ||= "_DISCOURSE_API".freeze
  TOKEN_COOKIE ||= "_t".freeze
  PATH_INFO ||= "PATH_INFO".freeze
@ -90,7 +91,7 @@ class Auth::DefaultCurrentUserProvider
        limiter_min.performed!
      end

-      current_user = lookup_user_api_user(api_key)
+      current_user = lookup_user_api_user_and_update_key(api_key, @env[USER_API_CLIENT_ID])
      raise Discourse::InvalidAccess unless current_user

      limiter_min.performed!
@ -176,16 +177,14 @@ class Auth::DefaultCurrentUserProvider

  protected

-  WHITELISTED_WRITE_PATHS ||= [/^\/message-bus\/.*\/poll/, /^\/user-api-key\/revoke$/]
-  def lookup_user_api_user(user_api_key)
+  def lookup_user_api_user_and_update_key(user_api_key, client_id)
    if api_key = UserApiKey.where(key: user_api_key, revoked_at: nil).includes(:user).first
-      unless api_key.write
-        if @env["REQUEST_METHOD"] != "GET"
-          path = @env["PATH_INFO"]
-          unless WHITELISTED_WRITE_PATHS.any?{|whitelisted| path =~ whitelisted}
-            raise Discourse::InvalidAccess
-          end
-        end
+      unless api_key.allow?(@env)
+        raise Discourse::InvalidAccess
+      end
+
+      if client_id.present? && client_id != api_key.client_id
+        api_key.update_columns(client_id: client_id)
      end

      api_key.user
--- a/spec/components/auth/default_current_user_provider_spec.rb
+++ b/spec/components/auth/default_current_user_provider_spec.rb
@ -163,9 +163,7 @@ describe Auth::DefaultCurrentUserProvider do
      UserApiKey.create!(
        application_name: 'my app',
        client_id: '1234',
-        read: true,
-        write: false,
-        push: false,
+        scopes: ['read'],
        key: SecureRandom.hex,
        user_id: user.id
      )
--- a/spec/controllers/user_api_keys_controller_spec.rb
+++ b/spec/controllers/user_api_keys_controller_spec.rb
@ -35,7 +35,7 @@ TXT

  let :args do
    {
-      access: 'r',
+      scopes: 'read',
      client_id: "x"*32,
      auth_redirect: 'http://over.the/rainbow',
      application_name: 'foo',
@ -48,7 +48,7 @@ TXT
    it "supports a head request cleanly" do
      head :new
      expect(response.code).to eq("200")
-      expect(response.headers["Auth-Api-Version"]).to eq("1")
+      expect(response.headers["Auth-Api-Version"]).to eq("2")
    end
  end

@ -67,7 +67,6 @@ TXT
    end

    it "will allow tokens for staff without TL" do
-
      SiteSetting.min_trust_level_for_user_api_key = 2
      SiteSetting.allowed_user_api_auth_redirects = args[:auth_redirect]

@ -96,7 +95,7 @@ TXT

      SiteSetting.min_trust_level_for_user_api_key = 0
      SiteSetting.allowed_user_api_auth_redirects = args[:auth_redirect]
-      SiteSetting.allow_read_user_api_keys = false
+      SiteSetting.allow_user_api_key_scopes = "write"

      user = Fabricate(:user, trust_level: 0)

@ -143,7 +142,7 @@ TXT
      SiteSetting.min_trust_level_for_user_api_key = 0
      SiteSetting.allowed_user_api_auth_redirects = args[:auth_redirect]

-      args[:access] = "pr"
+      args[:scopes] = "push,read"
      args[:push_url] = "https://push.it/here"

      user = Fabricate(:user, trust_level: 0)
@ -164,22 +163,21 @@ TXT
      parsed = JSON.parse(key.private_decrypt(encrypted))

      expect(parsed["nonce"]).to eq(args[:nonce])
-      expect(parsed["access"].split('').sort).to eq(['r'])
+      expect(parsed["push"]).to eq(false)
+      expect(parsed["api"]).to eq(2)

      key = user.user_api_keys.first
-      expect(key.push).to eq(true)
+      expect(key.scopes).to include("push")
      expect(key.push_url).to eq("https://push.it/here")

    end

    it "will redirect correctly with valid token" do
-
      SiteSetting.min_trust_level_for_user_api_key = 0
      SiteSetting.allowed_user_api_auth_redirects = args[:auth_redirect]
      SiteSetting.allowed_user_api_push_urls = "https://push.it/here"
-      SiteSetting.allow_write_user_api_keys = true

-      args[:access] = "prw"
+      args[:scopes] = "push,notifications,message_bus,session_info"
      args[:push_url] = "https://push.it/here"

      user = Fabricate(:user, trust_level: 0)
@ -200,14 +198,12 @@ TXT
      parsed = JSON.parse(key.private_decrypt(encrypted))

      expect(parsed["nonce"]).to eq(args[:nonce])
-      expect(parsed["access"].split('').sort).to eq(['p','r', 'w'])
+      expect(parsed["push"]).to eq(true)

      api_key = UserApiKey.find_by(key: parsed["key"])

      expect(api_key.user_id).to eq(user.id)
-      expect(api_key.read).to eq(true)
-      expect(api_key.write).to eq(true)
-      expect(api_key.push).to eq(true)
+      expect(api_key.scopes.sort).to eq(["push", "message_bus", "notifications", "session_info"].sort)
      expect(api_key.push_url).to eq("https://push.it/here")


--- a/spec/fabricators/user_api_key_fabricator.rb
+++ b/spec/fabricators/user_api_key_fabricator.rb
@ -1,8 +1,6 @@
 Fabricator(:readonly_user_api_key, from: :user_api_key) do
  user
-  read true
-  write false
-  push false
+  scopes ['read']
  client_id { SecureRandom.hex }
  key { SecureRandom.hex }
  application_name 'some app'
--- a/spec/models/user_api_key_spec.rb
+++ b/spec/models/user_api_key_spec.rb
@ -0,0 +1,28 @@
+require 'rails_helper'
+
+describe UserApiKey do
+  context "#allow?" do
+    it "can look up permissions correctly" do
+      key = UserApiKey.new(scopes: ['message_bus', 'notifications'])
+
+      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)
+
+      expect(key.allow?("action_dispatch.request.path_parameters" => {:controller => "notifications", :action => "mark_read"},
+                        "PATH_INFO" => "/xyz", "REQUEST_METHOD" => "PUT")).to eq(true)
+
+
+      expect(key.allow?("action_dispatch.request.path_parameters" => {:controller => "user_api_keys", :action => "revoke"},
+                        "PATH_INFO" => "/xyz", "REQUEST_METHOD" => "POST")).to eq(true)
+
+    end
+
+    it "can allow blanket read" do
+
+      key = UserApiKey.new(scopes: ['read'])
+
+      expect(key.allow?("PATH_INFO" => "/random", "REQUEST_METHOD" => "GET")).to eq(true)
+      expect(key.allow?("PATH_INFO" => "/random", "REQUEST_METHOD" => "PUT")).to eq(false)
+    end
+  end
+end
--- a/spec/services/post_alerter_spec.rb
+++ b/spec/services/post_alerter_spec.rb
@ -338,9 +338,7 @@ describe PostAlerter do
                           client_id: "xxx#{i}",
                           key: "yyy#{i}",
                           application_name: "iPhone#{i}",
-                           read: true,
-                           write: true,
-                           push: true,
+                           scopes: ['push'],
                           push_url: "https://site2.com/push")
      end