Log error on failed resource/create

[asmahood]

Hi there,

I'm working on adding SAML SSO support into our service provider and the lack of error during creating/updating a resource makes it nearly impossible to debug what is going on.

Could a log be added to this rescue

devise_saml_authenticatable/lib/devise_saml_authenticatable/model.rb

Lines 71 to 74 in 09221d5

	rescue
	logger.info("User(#{auth_value}) failed to create or update.")
	return nil
	end

[adamstegman]

Thanks for the suggestion! Can you add a little more clarity about what you'd like to see there?

You can also define saml_update_resource_hook yourself to add whatever logging you need:

Devise.saml_update_resource_hook = Proc.new do |user, saml_response, auth_value|
  # Copied from the default behavior: https://github.com/apokalipto/devise_saml_authenticatable/blob/09221d59e434bc947327e478a388013a70add4f7/lib/devise_saml_authenticatable.rb#L101
  saml_response.attributes.resource_keys.each do |key|
    user.send "#{key}=", saml_response.attribute_value_by_resource_key(key)
  end

  if (Devise.saml_use_subject)
    user.send "#{Devise.saml_default_user_key}=", auth_value
  end

  user.save!
rescue => e
  # Add additional logging here
  raise e
end

[asmahood]

Ah okay I did not think about adding a rescue in the update hook, I think I'll go with that

In terms of improvement, instead of just the message User(auth_value) failed to create or update possibly adding in the error message in the same log would be helpful to know why the create/update failed

[neroleung]

I'm affected by this rescue in a (probably) different manner. In 1.6.0, I raise some custom errors in the saml_update_resource_hook proc, and then rescue them (using rescue_from) in the controller so that I have better control of what to respond in regards to different errors. However, since this is now rescuing everything, my solution no longer works.

Is there a better way to do what I am trying to do, or would this perspective change your view about rescuing everything there? Please let me know. Thanks!

[adamstegman]

However, since this is now rescuing everything, my solution no longer works.

Is there a better way to do what I am trying to do, or would this perspective change your view about rescuing everything there?

Can you clarify what you mean here? What is no longer working?

[neroleung]

@adamstegman, my apologies. I meant something like the following:

# devise.rb
Devise.setup do |config|
  ...

  config.saml_update_resource_hook = proc do |user, saml_response, auth_value|
    raise SsoError1 unless condition1
    raise SsoError2 unless condition2

    ...
  end
end

# saml_sessions_controller.rb
class SamlSessionsController < Devise::SamlSessionsController
  rescue_from SsoError1, with: :handle_sso_error1
  rescue_from SsoError2, with: :handle_sso_error2

  def handle_sso_error1(error)
    render json: { error: "error1" }, status: 404
  end

  def handle_sso_error2(error)
    render json: { error: "error2" }, status: 422
  end
end

Since the errors will now be rescued by

devise_saml_authenticatable/lib/devise_saml_authenticatable/model.rb

Lines 71 to 74 in 09221d5

	rescue
	logger.info("User(#{auth_value}) failed to create or update.")
	return nil
	end

, they'd no longer reach the controller.

[adamstegman]

@neroleung Thanks for pointing that out. It seems like we need to revert #176. @luke-zhou if we do that, it will raise an exception instead of returning null. Would that behavior work in your use case?

[hlascelles]

Speaking for us, yes it would... We were seeing infinite login loops when the user create failed silently and no idea what was going on. We'd very much like to see some explicit warnings, or preferably an outright error. We are now doing the saml_update_resource_hook that raises an error.

[neroleung]

@hlascelles, looking at both #176 and #144, it feels like certain kind of validations and/or errors were raised inside your saml_update_resource_hook proc which caused the infinite redirects. Rescuing all the errors is one way of dealing with this but I'd like to argue that we should avoid rescuing all errors whenever possible as it might mask future unforeseen errors.

Would the rescue_from pattern that I added above work in your case? I feel like that might give you better info and hence better control over what you can do based on different errors that you are rescuing. Please let me know what you think. Thanks!

[hlascelles]

I have tried @adamstegman's suggestion on another issue and it works great: #182 (comment)

My handler now looks like:

class SamlFailedAuthCallback
  def handle(response, strategy)
    ::Rails.logger.warn "There was an error trying to log in the SAML user '#{response.name_id}'"
    strategy.redirect! "/saml_error"
  end
end

No need to extend the controller class.

[adamstegman]

Fixed in #184, released in 1.6.2.