Add validation for scopes attribute for secure param

Author: aishkan

config/locales/en.yml

@@ -238,10 +238,12 @@ 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}.'
-            scope_requires_secure_parameter: Scopes can only be defined on secure
-              parameters.
-            scopes_cannot_be_empty: Scopes cannot be empty when defined.
-            invalid_secure_parameter_scopes: 'Invalid secure parameter scopes: %{invalid_scope}.'
+            scopes_require_secure_parameter: Scopes can only be defined on secure
+              parameters for %{field}.
+            field_cannot_be_empty: "%{field} cannot be empty."
+            field_contains_invalid_values: "%{field} contains invalid values: %{values}."
+            field_contains_invalid_attributes: "%{field} contains invalid attributes:
+              %{attributes}."
         warning:
           app_build:
             deprecated_version: You are targeting a deprecated version of the framework.

config/locales/translations/zendesk_apps_support.yml

@@ -628,14 +628,18 @@ parts:
       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.scope_requires_secure_parameter"
+      key: "txt.apps.admin.error.app_build.scopes_require_secure_parameter"
       title: "App builder job: scopes can be defined only on secure parameters"
-      value: "Scopes can only be defined on secure parameters."
+      value: "Scopes can only be defined on secure parameters for %{field}."
   - translation:
-      key: "txt.apps.admin.error.app_build.scopes_cannot_be_empty"
-      title: "App builder job: scopes cannot be empty when defined"
-      value: "Scopes cannot be empty when defined."
+      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.invalid_secure_parameter_scopes"
-      title: "App builder job: invalid secure parameter scopes"
-      value: "Invalid secure parameter scopes: %{invalid_scope}."
+      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_attributes"
+      title: "App builder job: invalid field attributes"
+      value: "%{field} contains invalid attributes: %{attributes}."

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_scope_attribute_error(manifest)),
             ]
           end
         end
@@ -110,6 +112,49 @@ def oauth_cannot_be_secure(manifest)
           end
         end
 
+        def invalid_scope_attribute_error(manifest)
+          errors = []
+          manifest.parameters.each do |parameter|
+            next if parameter.scopes.nil?
+            
+            errors << ValidationError.new(:field_contains_invalid_attributes, 
+                                        field: "parameters.[name=#{parameter.name}]", 
+                                        attributes: ['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(:scopes_require_secure_parameter, field: "parameters.[name=#{parameter.name}]")
+            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),

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,152 @@ 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 not validate scopes even if parameter has invalid scopes' do
+        @manifest_hash = {
+          'parameters' => [
+            {
+              'name' => 'api_token',
+              'type' => 'text',
+              'secure' => false,
+              'scopes' => ['header']
+            }
+          ]
+        }
+        package = create_package(@manifest_hash)
+        errors = ZendeskAppsSupport::Validations::Manifest.call(package, validate_scopes_for_secure_parameter: false)
+        expect(errors.find { |e| e.key == :scopes_require_secure_parameter }).to be_nil
+      end
+
+      it 'should have an invalid_attributes error when scope attribute 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 attributes: \["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']
+              }
+            ]
+          }
+          package = create_package(@manifest_hash)
+          errors = ZendeskAppsSupport::Validations::Manifest.call(package, validate_scopes_for_secure_parameter: true)
+          expect(errors.find { |e| e.key == :scopes_require_secure_parameter }).not_to be_nil
+        end
+      end
+    end
+
+    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(:scopes_require_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 = {
+          'parameters' => [
+            {
+              'name' => 'api_token',
+              'type' => 'text',
+              'secure' => true,
+              'scopes' => ['header', 'body']
+            }
+          ]
+        }
+        expect(create_package(@manifest_hash)).not_to have_error(:scopes_require_secure_parameter)
+        expect(create_package(@manifest_hash)).not_to have_error(:invalid_secure_parameter_scopes)
+      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' => ['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(:unacceptable_array)
+      end
+
+    end
+
+    context 'when a parameter has no scopes' do
+      it 'should not have any scope-related errors' do
+        @manifest_hash = {
+          'parameters' => [
+            {
+              'name' => 'api_token',
+              'type' => 'text',
+              'secure' => true
+            }
+          ]
+        }
+        expect(create_package(@manifest_hash)).not_to have_error(:scopes_require_secure_parameter)
+        expect(create_package(@manifest_hash)).not_to have_error(:invalid_secure_parameter_scopes)
+      end
+    end
+  end
 end