Add validation for scopes attribute for secure parameters

Author: aishkan

config/locales/en.yml

@@ -281,6 +281,11 @@ en:
             invalid_url: '%{field} must be a valid URL, got "%{value}".'
             password_parameter_type_deprecated: 'Password parameter type can no longer
               be used. Use Secure settings instead. Learn more: %{link}.'
+            field_requires_secure_parameter: "%{field} cannot be defined in a non-secure
+              parameter."
+            field_cannot_be_empty: "%{field} cannot be empty."
+            field_contains_invalid_values: "%{field} contains invalid values: %{values}."
+            field_contains_invalid_keys: "%{field} contains invalid keys: %{keys}."
         warning:
           app_build:
             deprecated_version: You are targeting a deprecated version of the framework.

config/locales/translations/zendesk_apps_support.yml

@@ -633,3 +633,19 @@ parts:
       key: "txt.apps.admin.error.app_build.password_parameter_type_deprecated"
       title: "App builder job: Password parameter type is deprecated"
       value: "Password parameter type can no longer be used. Use Secure settings instead. Learn more: %{link}."
+  - translation:
+      key: "txt.apps.admin.error.app_build.field_requires_secure_parameter"
+      title: "App builder job: scopes can be defined only on secure parameters"
+      value: "%{field} cannot be defined in a non-secure parameter."
+  - translation:
+      key: "txt.apps.admin.error.app_build.field_cannot_be_empty"
+      title: "App builder job: field cannot be empty"
+      value: "%{field} cannot be empty."
+  - translation:
+      key: "txt.apps.admin.error.app_build.field_contains_invalid_values"
+      title: "App builder job: invalid field values"
+      value: "%{field} contains invalid values: %{values}."
+  - translation:
+      key: "txt.apps.admin.error.app_build.field_contains_invalid_keys"
+      title: "App builder job: invalid field keys"
+      value: "%{field} contains invalid keys: %{keys}."

lib/zendesk_apps_support/manifest/parameter.rb

@@ -4,7 +4,7 @@ module ZendeskAppsSupport
   class Manifest
     class Parameter
       TYPES = %w[text password checkbox url number multiline hidden oauth].freeze
-      ATTRIBUTES = %i[name type required secure default].freeze
+      ATTRIBUTES = %i[name type required secure default scopes].freeze
       attr_reader(*ATTRIBUTES)
       def default?
         @has_default
@@ -17,6 +17,7 @@ def initialize(p)
         @secure = p['secure'] || false
         @has_default = p.key? 'default'
         @default = p['default'] if @has_default
+        @scopes = p['scopes']
       end
     end
   end

lib/zendesk_apps_support/validations/manifest.rb

@@ -11,9 +11,10 @@ module Manifest
       OAUTH_REQUIRED_FIELDS = %w[client_id client_secret authorize_uri access_token_uri].freeze
       PARAMETER_TYPES = ZendeskAppsSupport::Manifest::Parameter::TYPES
       OAUTH_MANIFEST_LINK = 'https://developer.zendesk.com/apps/docs/developer-guide/manifest#oauth'
+      SECURE_PARAM_SCOPES = %w[header body url_host url_path jwt_secret_key jwt_claim basic_auth_username basic_auth_password].freeze
 
       class << self
-        def call(package, error_on_password_parameter: false)
+        def call(package, error_on_password_parameter: false, validate_scopes_for_secure_parameter: false)
           unless package.has_file?('manifest.json')
             nested_manifest = package.files.find { |file| file =~ %r{\A[^/]+?/manifest\.json\Z} }
             if nested_manifest
@@ -24,7 +25,7 @@ def call(package, error_on_password_parameter: false)
 
           package.warnings << password_parameter_warning if !error_on_password_parameter && password_param_present?(package.manifest)
 
-          collate_manifest_errors(package, error_on_password_parameter)
+          collate_manifest_errors(package, error_on_password_parameter, validate_scopes_for_secure_parameter)
         rescue JSON::ParserError => e
           return [ValidationError.new(:manifest_not_json, errors: e)]
         rescue ZendeskAppsSupport::Manifest::OverrideError => e
@@ -37,7 +38,7 @@ def password_param_present?(manifest)
           manifest.parameters.any? { |p| p.type == 'password' }
         end
 
-        def collate_manifest_errors(package, error_on_password_parameter)
+        def collate_manifest_errors(package, error_on_password_parameter, validate_scopes_for_secure_parameter)
           manifest = package.manifest
 
           errors = [
@@ -46,7 +47,7 @@ def collate_manifest_errors(package, error_on_password_parameter)
             oauth_error(manifest),
             default_locale_error(manifest, package),
             validate_urls(manifest),
-            validate_parameters(manifest),
+            validate_parameters(manifest, validate_scopes_for_secure_parameter),
             if manifest.requirements_only? || manifest.marketing_only?
               [ ban_location(manifest),
                 ban_framework_version(manifest) ]
@@ -87,7 +88,7 @@ def validate_urls(manifest)
           errors
         end
 
-        def validate_parameters(manifest)
+        def validate_parameters(manifest, validate_scopes_for_secure_parameter)
           if manifest.marketing_only?
             marketing_only_errors(manifest)
           else
@@ -97,7 +98,8 @@ def validate_parameters(manifest)
               invalid_type_error(manifest),
               too_many_oauth_parameters(manifest),
               oauth_cannot_be_secure(manifest),
-              name_as_parameter_name_error(manifest)
+              name_as_parameter_name_error(manifest),
+              *(validate_scopes_for_secure_parameter ? invalid_secure_param_scopes_errors(manifest) : invalid_scopes_key_error(manifest)),
             ]
           end
         end
@@ -110,6 +112,50 @@ def oauth_cannot_be_secure(manifest)
           end
         end
 
+        def invalid_scopes_key_error(manifest)
+          errors = []
+          manifest.parameters.each do |parameter|
+            next if parameter.scopes.nil?
+
+            errors << ValidationError.new(:field_contains_invalid_keys,
+                                        field: "parameters[name=\"#{parameter.name}\"]",
+                                        keys: ['scopes'])
+          end
+          errors
+        end
+
+        def invalid_secure_param_scopes_errors(manifest)
+          errors = []
+
+          manifest.parameters.each do |parameter|
+            next if parameter.scopes.nil?
+
+            unless parameter.secure
+              errors << ValidationError.new(:field_requires_secure_parameter, field: "parameters[name=\"#{parameter.name}\"].scopes")
+            end
+
+            unless parameter.scopes.is_a?(Array)
+              errors << ValidationError.new(:unacceptable_array,
+                                          field: "parameters[name=\"#{parameter.name}\"].scopes",
+                                          value: parameter.scopes.class.to_s)
+              next
+            end
+
+            if parameter.scopes.empty?
+              errors << ValidationError.new(:field_cannot_be_empty, field: "parameters[name=\"#{parameter.name}\"].scopes")
+            end
+
+            invalid_scopes = parameter.scopes - SECURE_PARAM_SCOPES
+            if invalid_scopes.any?
+              errors << ValidationError.new(:field_contains_invalid_values,
+                                            field: "parameters[name=\"#{parameter.name}\"]",
+                                            values: invalid_scopes)
+            end
+          end
+
+          errors
+        end
+
         def marketing_only_errors(manifest)
           [
             ban_parameters(manifest),
@@ -131,7 +177,7 @@ def type_checks(manifest)
             )
           end
           whitelist = manifest.domain_whitelist
-          unless whitelist.nil? || whitelist.is_a?(Array) && whitelist.all? { |dom| dom.is_a? String }
+          unless whitelist.nil? || (whitelist.is_a?(Array) && whitelist.all? { |dom| dom.is_a? String })
             errors << ValidationError.new(:unacceptable_array_of_strings, field: 'domainWhitelist')
           end
           parameters = manifest.original_parameters
@@ -261,12 +307,12 @@ def parameters_error(manifest)
           end
 
           invalid_required = manifest.parameters.map do |parameter|
-            validate_boolean(parameter.required, "parameters.#{parameter.name}.required")
+            validate_boolean(parameter.required, "parameters[name=\"#{parameter.name}\"].required")
           end.compact
           return invalid_required if invalid_required.any?
 
           invalid_secure = manifest.parameters.map do |parameter|
-            validate_boolean(parameter.secure, "parameters.#{parameter.name}.secure")
+            validate_boolean(parameter.secure, "parameters[name=\"#{parameter.name}\"].secure")
           end.compact
           return invalid_secure if invalid_secure.any?
         end

spec/validations/manifest_spec.rb

@@ -24,7 +24,7 @@ def create_package(parameter_hash)
 
   RSpec::Matchers.define :have_error do |error|
     match do |package|
-      errors = ZendeskAppsSupport::Validations::Manifest.call(package)
+      errors = ZendeskAppsSupport::Validations::Manifest.call(package, validate_scopes_for_secure_parameter: true)
       errors.map!(&:to_s) unless error.is_a? Symbol
       @actual = errors.compact
 
@@ -864,4 +864,133 @@ def create_package(parameter_hash)
       end
     end
   end
+
+  context 'scope parameter validations' do
+    context 'when validate_scopes_for_secure_parameter is false' do
+      it 'should have an invalid_keys error when scopes key is present' do
+        @manifest_hash = {
+          'parameters' => [
+            {
+              'name' => 'api_token',
+              'type' => 'text',
+              'secure' => true,
+              'scopes' => ['header']
+            }
+          ]
+        }
+        package = create_package(@manifest_hash)
+        errors = ZendeskAppsSupport::Validations::Manifest.call(package, validate_scopes_for_secure_parameter: false)
+        expect(errors.map(&:to_s)).to include('parameters[name="api_token"] contains invalid keys: ["scopes"].')
+      end
+    end
+
+    context 'when validate_scopes_for_secure_parameter is true' do
+      context 'when a parameter has scopes but is not secure' do
+        it 'should have an error requiring secure parameter for scopes' do
+          @manifest_hash = {
+            'parameters' => [
+              {
+                'name' => 'api_token',
+                'type' => 'text',
+                'secure' => false,
+                'scopes' => ['header']
+              }
+            ]
+          }
+          expect(create_package(@manifest_hash)).to have_error('parameters[name="api_token"].scopes cannot be defined in a non-secure parameter.')
+        end
+      end
+
+      context 'when a parameter has scopes and is secure' do
+        it 'should not have an error for valid scope values' do
+          @manifest_hash = {
+            'defaultLocale' => 'en',
+            'location' => {
+              'support' => {
+                'ticket_sidebar' => 'https://example.com/app'
+              }
+            },
+            'parameters' => [
+              {
+                'name' => 'api_token',
+                'type' => 'text',
+                'secure' => true,
+                'scopes' => %w[header body]
+              }
+            ]
+          }
+          package = create_package(@manifest_hash)
+          translation_files = double('AppFile', relative_path: 'translations/en.json')
+          allow(package).to receive_messages(translation_files: [translation_files])
+          expect(package).not_to have_error
+        end
+
+        it 'should have an error for invalid scope values with all invalid scopes in the error' do
+          @manifest_hash = {
+            'parameters' => [
+              {
+                'name' => 'api_token',
+                'type' => 'text',
+                'secure' => true,
+                'scopes' => %w[invalid_scope1 header invalid_scope2 body invalid_scope3]
+              }
+            ]
+          }
+          expect(create_package(@manifest_hash)).to have_error('parameters[name="api_token"] contains invalid values: ["invalid_scope1", "invalid_scope2", "invalid_scope3"].')
+        end
+
+        it 'should have an error when scopes are empty' do
+          @manifest_hash = {
+            'parameters' => [
+              {
+                'name' => 'api_token',
+                'type' => 'text',
+                'secure' => true,
+                'scopes' => []
+              }
+            ]
+          }
+          expect(create_package(@manifest_hash)).to have_error('parameters[name="api_token"].scopes cannot be empty.')
+        end
+
+        it 'should have an error when scopes is not an array' do
+          @manifest_hash = {
+            'parameters' => [
+              {
+                'name' => 'api_token',
+                'type' => 'text',
+                'secure' => true,
+                'scopes' => 'header'
+              }
+            ]
+          }
+          expect(create_package(@manifest_hash)).to have_error('The manifest field parameters[name="api_token"].scopes needs to be an array, but it was String')
+        end
+      end
+
+      context 'when a parameter has no scopes' do
+        it 'should not have any scope-related errors' do
+          @manifest_hash = {
+            'defaultLocale' => 'en',
+            'location' => {
+              'support' => {
+                'ticket_sidebar' => 'https://example.com/app'
+              }
+            },
+            'parameters' => [
+              {
+                'name' => 'api_token',
+                'type' => 'text',
+                'secure' => true
+              }
+            ]
+          }
+          package = create_package(@manifest_hash)
+          translation_files = double('AppFile', relative_path: 'translations/en.json')
+          allow(package).to receive_messages(translation_files: [translation_files])
+          expect(package).not_to have_error
+        end
+      end
+    end
+  end
 end