Merge pull request #100 from graphql-devise/gqld-98-insecure-password-reset-mutation

mcelicalderon · web-flow · commit 5d2d43775cc2 · 2020-06-12T12:27:59.000-05:00
Avoid returning user information on password reset mutation
diff --git a/config/locales/en.yml b/config/locales/en.yml
@@ -14,6 +14,7 @@ en:
       password_not_required: "This account does not require a password. Sign in using your '%{provider}' account instead."
       reset_token_not_found: "No user found for the specified reset token."
       reset_token_expired: "Reset password token is no longer valid."
+      send_instructions: "You will receive an email with instructions on how to reset your password in a few minutes."
     sessions:
       bad_credentials: "Invalid login credentials. Please try again."
       not_confirmed: "A confirmation email was sent to your account at '%{email}'. You must follow the instructions in the email before your account can be activated"
diff --git a/lib/graphql_devise/default_operations/mutations.rb b/lib/graphql_devise/default_operations/mutations.rb
@@ -9,12 +9,12 @@
 module GraphqlDevise
   module DefaultOperations
     MUTATIONS = {
-      login:               GraphqlDevise::Mutations::Login,
-      logout:              GraphqlDevise::Mutations::Logout,
-      sign_up:             GraphqlDevise::Mutations::SignUp,
-      update_password:     GraphqlDevise::Mutations::UpdatePassword,
-      send_password_reset: GraphqlDevise::Mutations::SendPasswordReset,
-      resend_confirmation: GraphqlDevise::Mutations::ResendConfirmation
+      login:               { klass: GraphqlDevise::Mutations::Login, authenticatable: true },
+      logout:              { klass: GraphqlDevise::Mutations::Logout, authenticatable: true },
+      sign_up:             { klass: GraphqlDevise::Mutations::SignUp, authenticatable: true },
+      update_password:     { klass: GraphqlDevise::Mutations::UpdatePassword, authenticatable: true },
+      send_password_reset: { klass: GraphqlDevise::Mutations::SendPasswordReset, authenticatable: false },
+      resend_confirmation: { klass: GraphqlDevise::Mutations::ResendConfirmation, authenticatable: false }
     }.freeze
   end
 end
diff --git a/lib/graphql_devise/default_operations/resolvers.rb b/lib/graphql_devise/default_operations/resolvers.rb
@@ -5,8 +5,8 @@
 module GraphqlDevise
   module DefaultOperations
     QUERIES = {
-      confirm_account:      GraphqlDevise::Resolvers::ConfirmAccount,
-      check_password_token: GraphqlDevise::Resolvers::CheckPasswordToken
+      confirm_account:      { klass: GraphqlDevise::Resolvers::ConfirmAccount },
+      check_password_token: { klass: GraphqlDevise::Resolvers::CheckPasswordToken }
     }.freeze
   end
 end
diff --git a/lib/graphql_devise/mount_method/operation_preparers/default_operation_preparer.rb b/lib/graphql_devise/mount_method/operation_preparers/default_operation_preparer.rb
@@ -10,14 +10,18 @@ def initialize(selected_operations:, custom_keys:, mapping_name:, preparer:)
         end
 
         def call
-          @selected_operations.except(*@custom_keys).each_with_object({}) do |(action, operation), result|
+          @selected_operations.except(*@custom_keys).each_with_object({}) do |(action, operation_info), result|
             mapped_action = "#{@mapping_name}_#{action}"
+            operation     = operation_info[:klass]
+            options       = operation_info.except(:klass)
 
             result[mapped_action.to_sym] = [
               OperationPreparers::GqlNameSetter.new(mapped_action),
               @preparer,
               OperationPreparers::ResourceNameSetter.new(@mapping_name)
-            ].reduce(child_class(operation)) { |prepared_operation, preparer| preparer.call(prepared_operation) }
+            ].reduce(child_class(operation)) do |prepared_operation, preparer|
+              preparer.call(prepared_operation, **options)
+            end
           end
         end
 
diff --git a/lib/graphql_devise/mount_method/operation_preparers/gql_name_setter.rb b/lib/graphql_devise/mount_method/operation_preparers/gql_name_setter.rb
@@ -6,7 +6,7 @@ def initialize(mapping_name)
           @mapping_name = mapping_name
         end
 
-        def call(operation)
+        def call(operation, **)
           operation.graphql_name(graphql_name)
 
           operation
diff --git a/lib/graphql_devise/mount_method/operation_preparers/mutation_field_setter.rb b/lib/graphql_devise/mount_method/operation_preparers/mutation_field_setter.rb
@@ -6,9 +6,10 @@ def initialize(authenticatable_type)
           @authenticatable_type = authenticatable_type
         end
 
-        def call(mutation)
-          mutation.field(:authenticatable, @authenticatable_type, null: false)
+        def call(mutation, authenticatable: true)
+          return mutation unless authenticatable
 
+          mutation.field(:authenticatable, @authenticatable_type, null: false)
           mutation
         end
       end
diff --git a/lib/graphql_devise/mount_method/operation_preparers/resolver_type_setter.rb b/lib/graphql_devise/mount_method/operation_preparers/resolver_type_setter.rb
@@ -6,7 +6,7 @@ def initialize(authenticatable_type)
           @authenticatable_type = authenticatable_type
         end
 
-        def call(resolver)
+        def call(resolver, **)
           resolver.type(@authenticatable_type, null: false)
 
           resolver
diff --git a/lib/graphql_devise/mount_method/operation_preparers/resource_name_setter.rb b/lib/graphql_devise/mount_method/operation_preparers/resource_name_setter.rb
@@ -6,7 +6,7 @@ def initialize(name)
           @name = name
         end
 
-        def call(operation)
+        def call(operation, **)
           operation.instance_variable_set(:@resource_name, @name)
 
           operation
diff --git a/lib/graphql_devise/mutations/resend_confirmation.rb b/lib/graphql_devise/mutations/resend_confirmation.rb
@@ -22,10 +22,7 @@ def resolve(email:, redirect_url:)
             template_path: ['graphql_devise/mailer']
           )
 
-          {
-            authenticatable: resource,
-            message:       I18n.t('graphql_devise.confirmations.send_instructions', email: email)
-          }
+          { message: I18n.t('graphql_devise.confirmations.send_instructions', email: email) }
         else
           raise_user_error(I18n.t('graphql_devise.confirmations.user_not_found', email: email))
         end
diff --git a/lib/graphql_devise/mutations/send_password_reset.rb b/lib/graphql_devise/mutations/send_password_reset.rb
@@ -4,6 +4,8 @@ class SendPasswordReset < Base
       argument :email,        String, required: true
       argument :redirect_url, String, required: true
 
+      field :message, String, null: false
+
       def resolve(email:, redirect_url:)
         resource = find_resource(:email, get_case_insensitive_field(:email, email))
 
@@ -18,7 +20,7 @@ def resolve(email:, redirect_url:)
           )
 
           if resource.errors.empty?
-            { authenticatable: resource }
+            { message: I18n.t('graphql_devise.passwords.send_instructions') }
           else
             raise_user_error_list(I18n.t('graphql_devise.invalid_resource'), errors: resource.errors.full_messages)
           end
diff --git a/spec/requests/mutations/resend_confirmation_spec.rb b/spec/requests/mutations/resend_confirmation_spec.rb
@@ -15,10 +15,6 @@
           redirectUrl:"#{redirect}"
         ) {
           message
-          authenticatable {
-            id
-            email
-          }
         }
       }
     GRAPHQL
@@ -28,11 +24,7 @@
     it 'sends an email to the user with confirmation url and returns a success message' do
       expect { post_request }.to change(ActionMailer::Base.deliveries, :count).by(1)
       expect(json_response[:data][:userResendConfirmation]).to include(
-        message:         'You will receive an email with instructions for how to confirm your email address in a few minutes.',
-        authenticatable: {
-          id:    id,
-          email: email
-        }
+        message: 'You will receive an email with instructions for how to confirm your email address in a few minutes.'
       )
 
       email = Nokogiri::HTML(ActionMailer::Base.deliveries.last.body.encoded)
@@ -56,11 +48,7 @@
       it 'honors devise configuration for case insensitive fields' do
         expect { post_request }.to change(ActionMailer::Base.deliveries, :count).by(1)
         expect(json_response[:data][:userResendConfirmation]).to include(
-          message:         'You will receive an email with instructions for how to confirm your email address in a few minutes.',
-          authenticatable: {
-            id:    id,
-            email: user.email
-          }
+          message: 'You will receive an email with instructions for how to confirm your email address in a few minutes.'
         )
       end
     end
diff --git a/spec/requests/mutations/send_password_reset_spec.rb b/spec/requests/mutations/send_password_reset_spec.rb
@@ -13,9 +13,7 @@
           email:       "#{email}",
           redirectUrl: "#{redirect_url}"
         ) {
-          authenticatable {
-            email
-          }
+          message
         }
       }
     GRAPHQL
@@ -25,6 +23,10 @@
     it 'sends password reset  email' do
       expect { post_request }.to change(ActionMailer::Base.deliveries, :count).by(1)
 
+      expect(json_response[:data][:userSendPasswordReset]).to include(
+        message: 'You will receive an email with instructions on how to reset your password in a few minutes.'
+      )
+
       email = Nokogiri::HTML(ActionMailer::Base.deliveries.last.body.encoded)
       link  = email.css('a').first
 
@@ -41,6 +43,9 @@
 
     it 'honors devise configuration for case insensitive fields' do
       expect { post_request }.to change(ActionMailer::Base.deliveries, :count).by(1)
+      expect(json_response[:data][:userSendPasswordReset]).to include(
+        message: 'You will receive an email with instructions on how to reset your password in a few minutes.'
+      )
     end
   end
 
diff --git a/spec/services/mount_method/operation_preparer_spec.rb b/spec/services/mount_method/operation_preparer_spec.rb
@@ -14,10 +14,15 @@
 
     let(:logout_class) { Class.new(GraphQL::Schema::Resolver) }
     let(:mapping)      { :user }
-    let(:selected)     { { login: double(:login_default), logout: logout_class } }
     let(:preparer)     { double(:preparer, call: logout_class) }
     let(:custom)       { { login: double(:custom_login, graphql_name: nil) } }
     let(:additional)   { { user_additional: double(:user_additional) } }
+    let(:selected) do
+      {
+        login: { klass: double(:login_default) },
+        logout:{ klass: logout_class }
+      }
+    end
 
     it 'is expected to return all provided operation keys' do
       expect(prepared_operations.keys).to contain_exactly(
diff --git a/spec/services/mount_method/operation_preparers/default_operation_preparer_spec.rb b/spec/services/mount_method/operation_preparers/default_operation_preparer_spec.rb
@@ -11,18 +11,25 @@
     let(:logout_operation)  { double(:sign_up_operation, graphql_name: nil) }
     let(:mapping_name)      { :user }
     let(:preparer)          { double(:preparer) }
-    let(:operations)        { { login: login_operation, logout: logout_operation, sign_up: sign_up_operation, confirm: confirm_operation } }
     let(:custom_keys)       { [:login, :logout] }
+    let(:operations) do
+      {
+        confirm: { klass: confirm_operation, authenticatable: false },
+        sign_up: { klass: sign_up_operation, authenticatable: true },
+        login:   { klass: login_operation, authenticatable: true },
+        logout:  { klass: logout_operation, authenticatable: true }
+      }
+    end
 
     before do
       allow(default_preparer).to receive(:child_class).with(confirm_operation).and_return(confirm_operation)
       allow(default_preparer).to receive(:child_class).with(sign_up_operation).and_return(sign_up_operation)
       allow(default_preparer).to receive(:child_class).with(login_operation).and_return(login_operation)
       allow(default_preparer).to receive(:child_class).with(logout_operation).and_return(logout_operation)
-      allow(preparer).to receive(:call).with(confirm_operation).and_return(confirm_operation)
-      allow(preparer).to receive(:call).with(sign_up_operation).and_return(sign_up_operation)
-      allow(preparer).to receive(:call).with(login_operation).and_return(login_operation)
-      allow(preparer).to receive(:call).with(logout_operation).and_return(logout_operation)
+      allow(preparer).to receive(:call).with(confirm_operation, authenticatable: false).and_return(confirm_operation)
+      allow(preparer).to receive(:call).with(sign_up_operation, authenticatable: true).and_return(sign_up_operation)
+      allow(preparer).to receive(:call).with(login_operation, authenticatable: true).and_return(login_operation)
+      allow(preparer).to receive(:call).with(logout_operation, authenticatable: true).and_return(logout_operation)
     end
 
     it 'returns only those operations with no custom operation provided' do
@@ -32,8 +39,8 @@
     it 'prepares default operations' do
       expect(confirm_operation).to receive(:graphql_name).with('UserConfirm')
       expect(sign_up_operation).to receive(:graphql_name).with('UserSignUp')
-      expect(preparer).to receive(:call).with(confirm_operation)
-      expect(preparer).to receive(:call).with(sign_up_operation)
+      expect(preparer).to receive(:call).with(confirm_operation, authenticatable: false)
+      expect(preparer).to receive(:call).with(sign_up_operation, authenticatable: true)
 
       prepared
 
diff --git a/spec/services/mount_method/operation_preparers/mutation_field_setter_spec.rb b/spec/services/mount_method/operation_preparers/mutation_field_setter_spec.rb
@@ -2,15 +2,29 @@
 
 RSpec.describe GraphqlDevise::MountMethod::OperationPreparers::MutationFieldSetter do
   describe '#call' do
-    subject(:prepared_operation) { described_class.new(field_type).call(operation) }
+    subject(:prepared_operation) { described_class.new(field_type).call(operation, authenticatable: authenticatable) }
 
     let(:operation)  { double(:operation) }
     let(:field_type) { double(:type) }
 
-    it 'sets a field for the mutation' do
-      expect(operation).to receive(:field).with(:authenticatable, field_type, null: false)
+    context 'when resource is authtenticable' do
+      let(:authenticatable) { true }
 
-      prepared_operation
+      it 'sets a field for the mutation' do
+        expect(operation).to receive(:field).with(:authenticatable, field_type, null: false)
+
+        prepared_operation
+      end
+    end
+
+    context 'when resource is *NOT* authtenticable' do
+      let(:authenticatable) { false }
+
+      it 'does *NOT* set a field for the mutation' do
+        expect(operation).not_to receive(:field)
+
+        prepared_operation
+      end
     end
   end
 end
