Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Raise error when attempting to render, but rendering was disabled #458

Merged
merged 2 commits into from
Dec 13, 2024

Conversation

janko
Copy link
Contributor

@janko janko commented Dec 9, 2024

When configuring Rodauth plugin with render: false, it could happen that Rodauth would attempt to render a built-in view/email template:

  • attempting to render a built-in email template is possible for JSON or internal requests
  • external gem might incorrectly attempt to render a view template in JSON or internal requests
  • in Rails apps, people would typically use Action View for rendering view/email templates, and they might miss some when disabling Tilt

Currently, Rodauth raises a rather cryptic:

NoMethodError: undefined method `parse_template_opts' for an instance of #<Class:0x000000011d9e3f40>

I initially started adding the error in rodauth-rails, but I thought it would be beneficial to have this in Rodauth itself. And for this it made sense to me to add a custom error class.

Technically, this doesn't cover the #button case, but those are only rendered within view templates, so the error should already be raised by #_view beforehand.

Copy link
Owner

@jeremyevans jeremyevans left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the patch. Can we rename the custom error class from Error to ConfigurationError to make clear this is an error in the user's configuration?

@janko janko force-pushed the better-no-render-error-message branch 2 times, most recently from eb2f7d7 to 1b4eba5 Compare December 10, 2024 22:16
@janko
Copy link
Contributor Author

janko commented Dec 10, 2024

Agreed, renamed 👍🏻

I was thinking that afterwards we could reuse this error class in other places:

def require_response(meth)
send(meth)
raise RuntimeError, "#{meth.to_s.sub(/\A_/, '')} overridden without returning a response (should use redirect or request.halt). This is a bug in your Rodauth configuration, not a bug in Rodauth itself."
end
def compute_raw_hmac(data)
raise ArgumentError, "hmac_secret not set" unless hmac_secret
compute_raw_hmac_with_secret(data, hmac_secret)
end
def jwt_secret
raise ArgumentError, "jwt_secret not set"
end
def sms_send(phone, message)
raise NotImplementedError, "sms_send needs to be defined in the Rodauth configuration for SMS sending to work"
end

@jeremyevans
Copy link
Owner

All of those locations are great places to use this error, thank you for doing the research.

@janko janko force-pushed the better-no-render-error-message branch from 1b4eba5 to 7ec0a54 Compare December 12, 2024 14:29
@janko
Copy link
Contributor Author

janko commented Dec 12, 2024

Updated the mentioned places with the new configuration error ✅

@jeremyevans jeremyevans merged commit 7ec0a54 into jeremyevans:master Dec 13, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants