From 3791fbd9196c5a10d2723e3c46e7cf8f008caa4c Mon Sep 17 00:00:00 2001
From: Bianca Nenciu <nbianca@users.noreply.github.com>
Date: Wed, 10 Nov 2021 17:48:00 +0200
Subject: [PATCH] FEATURE: Add read-only scope to API keys (#14856)

This commit adds a global read-only scope that can be used to create
new API keys.
---
 .../addon/controllers/admin-api-keys-new.js   | 25 +++++-
 .../admin/addon/templates/api-keys-new.hbs    | 16 ++--
 .../admin/addon/templates/api-keys-show.hbs   |  2 +-
 app/assets/stylesheets/common/admin/api.scss  |  3 -
 app/models/api_key_scope.rb                   | 63 ++++++++------
 config/locales/client.en.yml                  |  8 +-
 spec/models/api_key_spec.rb                   | 86 +++++++++++--------
 spec/requests/admin/api_controller_spec.rb    |  2 +-
 8 files changed, 132 insertions(+), 73 deletions(-)

diff --git a/app/assets/javascripts/admin/addon/controllers/admin-api-keys-new.js b/app/assets/javascripts/admin/addon/controllers/admin-api-keys-new.js
index 961c1a5c4b2..a95c7845c08 100644
--- a/app/assets/javascripts/admin/addon/controllers/admin-api-keys-new.js
+++ b/app/assets/javascripts/admin/addon/controllers/admin-api-keys-new.js
@@ -10,7 +10,8 @@ import { ajax } from "discourse/lib/ajax";
 
 export default Controller.extend({
   userModes: null,
-  useGlobalKey: false,
+  scopeModes: null,
+  globalScopes: null,
   scopes: null,
 
   init() {
@@ -20,6 +21,13 @@ export default Controller.extend({
       { id: "all", name: I18n.t("admin.api.all_users") },
       { id: "single", name: I18n.t("admin.api.single_user") },
     ]);
+
+    this.set("scopeModes", [
+      { id: "granular", name: I18n.t("admin.api.scopes.granular") },
+      { id: "read_only", name: I18n.t("admin.api.scopes.read_only") },
+      { id: "global", name: I18n.t("admin.api.scopes.global") },
+    ]);
+
     this._loadScopes();
   },
 
@@ -49,14 +57,23 @@ export default Controller.extend({
     this.set("userMode", userMode);
   },
 
+  @action
+  changeScopeMode(scopeMode) {
+    this.set("scopeMode", scopeMode);
+  },
+
   @action
   save() {
-    if (!this.useGlobalKey) {
+    if (this.scopeMode === "granular") {
       const selectedScopes = Object.values(this.scopes)
         .flat()
         .filterBy("selected");
 
       this.model.set("scopes", selectedScopes);
+    } else if (this.scopeMode === "read_only") {
+      this.model.set("scopes", [this.globalScopes.findBy("key", "read")]);
+    } else if (this.scopeMode === "all") {
+      this.model.set("scopes", null);
     }
 
     return this.model.save().catch(popupAjaxError);
@@ -78,6 +95,10 @@ export default Controller.extend({
   _loadScopes() {
     return ajax("/admin/api/keys/scopes.json")
       .then((data) => {
+        // remove global scopes because there is a different dropdown
+        this.set("globalScopes", data.scopes.global);
+        delete data.scopes.global;
+
         this.set("scopes", data.scopes);
       })
       .catch(popupAjaxError);
diff --git a/app/assets/javascripts/admin/addon/templates/api-keys-new.hbs b/app/assets/javascripts/admin/addon/templates/api-keys-new.hbs
index 2b9e40cf80a..7f703c3bc21 100644
--- a/app/assets/javascripts/admin/addon/templates/api-keys-new.hbs
+++ b/app/assets/javascripts/admin/addon/templates/api-keys-new.hbs
@@ -36,12 +36,18 @@
       {{/admin-form-row}}
     {{/if}}
 
-    {{#admin-form-row label="admin.api.use_global_key"}}
-      {{input type="checkbox" checked=useGlobalKey}}
+    {{#admin-form-row label="admin.api.scope_mode"}}
+      {{combo-box content=scopeModes value=scopeMode onChange=(action "changeScopeMode")}}
+
+      {{#if (eq scopeMode "read_only")}}
+        <p>{{i18n "admin.api.scopes.descriptions.global.read"}}</p>
+      {{else if (eq scopeMode "global")}}
+        <p>{{i18n "admin.api.scopes.global_description"}}</p>
+      {{/if}}
     {{/admin-form-row}}
 
-    {{#unless useGlobalKey}}
-      <div class="scopes-title">{{i18n "admin.api.scopes.title"}}</div>
+    {{#if (eq scopeMode "granular")}}
+      <h2 class="scopes-title">{{i18n "admin.api.scopes.title"}}</h2>
       <p>{{i18n "admin.api.scopes.description"}}</p>
       <table class="scopes-table grid">
         <thead>
@@ -82,7 +88,7 @@
           {{/each-in}}
         </tbody>
       </table>
-    {{/unless}}
+    {{/if}}
 
     {{d-button icon="check" label="admin.api.save" action=(action "save") class="btn-primary" disabled=saveDisabled}}
   {{/if}}
diff --git a/app/assets/javascripts/admin/addon/templates/api-keys-show.hbs b/app/assets/javascripts/admin/addon/templates/api-keys-show.hbs
index 771ecbd1c4b..436e0cf0048 100644
--- a/app/assets/javascripts/admin/addon/templates/api-keys-show.hbs
+++ b/app/assets/javascripts/admin/addon/templates/api-keys-show.hbs
@@ -83,7 +83,7 @@
   {{/admin-form-row}}
 
   {{#if model.api_key_scopes.length}}
-    <div class="scopes-title">{{i18n "admin.api.scopes.title"}}</div>
+    <h2 class="scopes-title">{{i18n "admin.api.scopes.title"}}</h2>
 
     <table class="scopes-table grid">
       <thead>
diff --git a/app/assets/stylesheets/common/admin/api.scss b/app/assets/stylesheets/common/admin/api.scss
index 91d25c87a5c..f664986c045 100644
--- a/app/assets/stylesheets/common/admin/api.scss
+++ b/app/assets/stylesheets/common/admin/api.scss
@@ -179,9 +179,6 @@ table.api-keys {
       width: 50%;
     }
     .scopes-title {
-      font-size: $font-up-2;
-      font-weight: bold;
-      text-decoration: underline;
       margin-top: 20px;
     }
   }
diff --git a/app/models/api_key_scope.rb b/app/models/api_key_scope.rb
index f4cb44e2f8c..9016da3b68e 100644
--- a/app/models/api_key_scope.rb
+++ b/app/models/api_key_scope.rb
@@ -17,6 +17,9 @@ class ApiKeyScope < ActiveRecord::Base
       return @default_mappings unless @default_mappings.nil?
 
       mappings = {
+        global: {
+          read: { methods: %i[get] }
+        },
         topics: {
           write: { actions: %w[posts#create], params: %i[topic_id] },
           read: {
@@ -48,12 +51,7 @@ class ApiKeyScope < ActiveRecord::Base
         }
       }
 
-      mappings.each_value do |resource_actions|
-        resource_actions.each_value do |action_data|
-          action_data[:urls] = find_urls(action_data[:actions])
-        end
-      end
-
+      parse_resources!(mappings)
       @default_mappings = mappings
     end
 
@@ -62,33 +60,48 @@ class ApiKeyScope < ActiveRecord::Base
       return default_mappings if plugin_mappings.empty?
 
       default_mappings.deep_dup.tap do |mappings|
-
-        plugin_mappings.each do |resource|
-          resource.each_value do |resource_actions|
-            resource_actions.each_value do |action_data|
-              action_data[:urls] = find_urls(action_data[:actions])
-            end
-          end
-
-          mappings.deep_merge!(resource)
+        plugin_mappings.each do |plugin_mapping|
+          parse_resources!(plugin_mapping)
+          mappings.deep_merge!(plugin_mapping)
         end
       end
     end
 
-    def find_urls(actions)
-      Rails.application.routes.routes.reduce([]) do |memo, route|
-        defaults = route.defaults
-        action = "#{defaults[:controller].to_s}##{defaults[:action]}"
-        path = route.path.spec.to_s.gsub(/\(\.:format\)/, '')
-        api_supported_path = path.end_with?('.rss') || route.path.requirements[:format]&.match?('json')
-        excluded_paths = %w[/new-topic /new-message /exception]
+    def parse_resources!(mappings)
+      mappings.each_value do |resource_actions|
+        resource_actions.each_value do |action_data|
+          action_data[:urls] = find_urls(actions: action_data[:actions], methods: action_data[:methods])
+        end
+      end
+    end
 
-        memo.tap do |m|
-          if actions.include?(action) && api_supported_path && !excluded_paths.include?(path)
-            m << "#{path} (#{route.verb})"
+    def find_urls(actions:, methods:)
+      action_urls = []
+      method_urls = []
+
+      if actions.present?
+        Rails.application.routes.routes.reduce([]) do |memo, route|
+          defaults = route.defaults
+          action = "#{defaults[:controller].to_s}##{defaults[:action]}"
+          path = route.path.spec.to_s.gsub(/\(\.:format\)/, '')
+          api_supported_path = path.end_with?('.rss') || route.path.requirements[:format]&.match?('json')
+          excluded_paths = %w[/new-topic /new-message /exception]
+
+          memo.tap do |m|
+            if actions.include?(action) && api_supported_path && !excluded_paths.include?(path)
+              m << "#{path} (#{route.verb})"
+            end
           end
         end
       end
+
+      if methods.present?
+        methods.each do |method|
+          method_urls << "* (#{method})"
+        end
+      end
+
+      action_urls + method_urls
     end
   end
 
diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml
index e88a15ced35..8a6b07bd90c 100644
--- a/config/locales/client.en.yml
+++ b/config/locales/client.en.yml
@@ -4144,6 +4144,7 @@ en:
         no_description: (no description)
         all_api_keys: All API Keys
         user_mode: User Level
+        scope_mode: Scope
         impersonate_all_users: Impersonate any user
         single_user: "Single User"
         user_placeholder: Enter username
@@ -4154,12 +4155,15 @@ en:
         delete: Permanently Delete
         not_shown_again: This key will not be displayed again. Make sure you take a copy before continuing.
         continue: Continue
-        use_global_key: Global Key (allows all actions)
         scopes:
           description: |
             When using scopes, you can restrict an API key to a specific set of endpoints.
             You can also define which parameters will be allowed. Use commas to separate multiple values.
           title: Scopes
+          granular: Granular
+          read_only: Read-only
+          global: Global
+          global_description: API key has no restriction and all endpoints are accessible.
           resource: Resource
           action: Action
           allowed_parameters: Allowed Parameters
@@ -4167,6 +4171,8 @@ en:
           any_parameter: (any parameter)
           allowed_urls: Allowed URLs
           descriptions:
+            global:
+              read: Restrict API key to read-only endpoints.
             topics:
               read: Read a topic or a specific post in it. RSS is also supported.
               write: Create a new topic or post to an existing one.
diff --git a/spec/models/api_key_spec.rb b/spec/models/api_key_spec.rb
index 88d0aa82d6f..461cc6df882 100644
--- a/spec/models/api_key_spec.rb
+++ b/spec/models/api_key_spec.rb
@@ -107,47 +107,63 @@ describe ApiKey do
 
     let(:env) { request.env }
 
-    let(:scope) do
-      ApiKeyScope.new(resource: 'topics', action: 'read', allowed_parameters: { topic_id: '3' })
-    end
-
     let(:key) { ApiKey.new(api_key_scopes: [scope]) }
 
-    it 'allows the request if there are no allowed IPs' do
-      key.allowed_ips = nil
-      key.api_key_scopes = []
-      expect(key.request_allowed?(env)).to eq(true)
+    context 'with regular scopes' do
+      let(:scope) do
+        ApiKeyScope.new(resource: 'topics', action: 'read', allowed_parameters: { topic_id: '3' })
+      end
+
+      it 'allows the request if there are no allowed IPs' do
+        key.allowed_ips = nil
+        key.api_key_scopes = []
+        expect(key.request_allowed?(env)).to eq(true)
+      end
+
+      it 'rejects the request if the IP is not allowed' do
+        key.allowed_ips = %w[115.65.76.87]
+        expect(key.request_allowed?(env)).to eq(false)
+      end
+
+      it 'allow the request if there are not allowed params' do
+        scope.allowed_parameters = nil
+        expect(key.request_allowed?(env)).to eq(true)
+      end
+
+      it 'rejects the request when params are different' do
+        request.path_parameters = { controller: "topics", action: "show", topic_id: "4" }
+        expect(key.request_allowed?(env)).to eq(false)
+      end
+
+      it 'accepts the request if one of the parameters match' do
+        request.path_parameters = { controller: "topics", action: "show", topic_id: "4" }
+        scope.allowed_parameters = { topic_id: %w[3 4] }
+        expect(key.request_allowed?(env)).to eq(true)
+      end
+
+      it 'allow the request when the scope has an alias' do
+        request.path_parameters = { controller: "topics", action: "show", id: "3" }
+        expect(key.request_allowed?(env)).to eq(true)
+      end
+
+      it 'rejects the request when the main parameter and the alias are both used' do
+        request.path_parameters = { controller: "topics", action: "show", topic_id: "3", id: "3" }
+        expect(key.request_allowed?(env)).to eq(false)
+      end
     end
 
-    it 'rejects the request if the IP is not allowed' do
-      key.allowed_ips = %w[115.65.76.87]
-      expect(key.request_allowed?(env)).to eq(false)
-    end
+    context 'with global:read scope' do
+      let(:scope) do
+        ApiKeyScope.new(resource: 'global', action: 'read')
+      end
 
-    it 'allow the request if there are not allowed params' do
-      scope.allowed_parameters = nil
-      expect(key.request_allowed?(env)).to eq(true)
-    end
+      it 'allows only GET requests for global:read' do
+        request.request_method = 'GET'
+        expect(key.request_allowed?(env)).to eq(true)
 
-    it 'rejects the request when params are different' do
-      request.path_parameters = { controller: "topics", action: "show", topic_id: "4" }
-      expect(key.request_allowed?(env)).to eq(false)
-    end
-
-    it 'accepts the request if one of the parameters match' do
-      request.path_parameters = { controller: "topics", action: "show", topic_id: "4" }
-      scope.allowed_parameters = { topic_id: %w[3 4] }
-      expect(key.request_allowed?(env)).to eq(true)
-    end
-
-    it 'allow the request when the scope has an alias' do
-      request.path_parameters = { controller: "topics", action: "show", id: "3" }
-      expect(key.request_allowed?(env)).to eq(true)
-    end
-
-    it 'rejects the request when the main parameter and the alias are both used' do
-      request.path_parameters = { controller: "topics", action: "show", topic_id: "3", id: "3" }
-      expect(key.request_allowed?(env)).to eq(false)
+        request.request_method = 'POST'
+        expect(key.request_allowed?(env)).to eq(false)
+      end
     end
   end
 end
diff --git a/spec/requests/admin/api_controller_spec.rb b/spec/requests/admin/api_controller_spec.rb
index 4e088a32649..e1f2d4d4e2b 100644
--- a/spec/requests/admin/api_controller_spec.rb
+++ b/spec/requests/admin/api_controller_spec.rb
@@ -237,7 +237,7 @@ describe Admin::ApiController do
 
         scopes = response.parsed_body['scopes']
 
-        expect(scopes.keys).to contain_exactly('topics', 'users', 'email', 'posts')
+        expect(scopes.keys).to contain_exactly('topics', 'users', 'email', 'posts', 'global')
       end
     end
   end