jeremyevans / rodauth

Ruby's Most Advanced Authentication Framework
http://rodauth.jeremyevans.net
MIT License
1.67k stars 95 forks source link

Make `email_from`, `email_to`, and `token_param_value` methods public #283

Closed janko closed 1 year ago

janko commented 1 year ago

The email_from and email_to methods make it easier to create custom emails in a background job, where the Rodauth instance needs to be re-created.

The token_param_value method makes it easier to generate an email token directly, to be used for a custom email link to a frontend app, for sending it in an HTTP request to a different service, or anything else.

Below is an example Rails setup where these would be useful:

class RodauthMain < Rodauth::Rails::Auth
  configure do
    send_verify_account_email do
      RodauthMailer.verify_account(account_id, verify_account_key_value).deliver_later
    end
  end
end
class RodauthMailer < ActionMailer::Base
  def verify_account(account_id, key)
    @rodauth = rodauth(account_id)
    @email_link = "https://frontend.example.com/account/verify?token=#{@rodauth.token_param_value(key)}"

    mail from: @rodauth.email_from, to: @rodauth.email_to, subject: @rodauth.verify_account_email_subject
  end

  private

  def rodauth(account_id)
    instance = RodauthApp.rodauth.allocate
    instance.instance_eval { @account = account_ds(account_id).first }
    instance
  end
end
jeremyevans commented 1 year ago

For email_from and email_to, this definitely makes sense, because if you used the related configuration methods, I think it would define public methods. That appears to be an issue with send_email and create_email, FWIW. I guess I don't have an issue making token_param_value public, but it kind of seems weird to make token_param_value public and not token_link. Thoughts?

janko commented 1 year ago

I suppose we could make token_link public as well, I just didn't really find specific use cases. Theoretically, it could allow you to build URLs to a separate frontend (assuming you change base_url/domain), but you would be constrained with the path prefix, meaning the frontend app would need to have the same one as the backend. Let me know if you have some use cases, as it would help me with the docs 😉

jeremyevans commented 1 year ago

I don't have a use case, it just seems inconsistent to have token_param_value public and token_link private. But maybe it's best not to make token_link public without a use case. In general, nobody should be calling it directly, they should be using the *_email_link methods.

In your earlier example, is there a reason you are not doing:

@email_link = @rodauth.verify_account_email_link

That seems simpler, so I assume there is a reason you are avoiding it.

janko commented 1 year ago

Yes, in the example I'm showing how to generate a link to a different app (e.g. a frontend app), whose endpoint doesn't necessarily correspond to Rodauth's verify account endpoint. So, that's why I'm not using the *_email_link method. FYI, this PR originated from https://github.com/janko/rodauth-rails/discussions/177.

jeremyevans commented 1 year ago

Are there other cases in your app where you are using base_url and want to refer to the backend instead of the frontend? If not, I would assume you could just set base_url to the frontend and use the normal *_email_link methods. Assuming you did need to separate the email base url from the standard base url, we could add an email_base_url method that would be used for emails.

jeremyevans commented 1 year ago

In terms of generic background emails, seems simplest to have a custom delivery_method used by the mail gem that creates a background job for delivery. If you really want to generate the email in a background job instead of just deliver it in a background job, that's more work.

janko commented 1 year ago

I updated the PR to just make email_from and email_to public, as I did have a real need for calling those within the Action Mailer subclass generated by rodauth-rails, so that changes to email_from and email_to are propagated with this setup (rodauth-rails overrides send_*_email methods, so the email is generated inside the background job). BTW, I believe currently changing email_from & email_to would still keep them private, because Rodauth keeps method visibility when calling auth methods.

I removed token_param_value, because I realized that *_email_link can be overridden to point to the frontend app, and in that case token_param_value can be called if needed, as private methods can be called within Rodauth context. So, I didn't have a use case where token_param_value would need to be called from the outside.

Are there other cases in your app where you are using base_url and want to refer to the backend instead of the frontend?

I don't have this myself, but I would imagine I would want base_url to keep pointing to the backend, because it's used by route_url, and I would want *_path & *_url to keep returning backend links, so that features like /multifactor-auth & /multifactor-setup in JSON mode would still return correct links.

Assuming you did need to separate the email base url from the standard base url, we could add an email_base_url method that would be used for emails.

That's an interesting idea. In rodauth-rails, when base_url & domain are missing, I'm grabbing config.action_mailer.default_url_options and using those, which Action Mailer will use by default when generating URLs. That will be used when Rodauth object is allocated directly (backgound jobs), and by the internal request feature as well, which may or may not be desired for all scenarios. I will give this idea some more thought, thanks for suggesting it 🙂

In terms of generic background emails, seems simplest to have a custom delivery_method used by the mail gem that creates a background job for delivery.

I wanted to avoid this in default rodauth-rails setup to avoid sending sensitive information in background job arguments, as they would likely end up in logs. Not including emails seems more GDPR-friendly, and not including tokens seems more secure, as an attacker might get access to them. So, I went the more difficult route 🙂

jeremyevans commented 1 year ago

OK, this looks good. It should have been this way from the start so that using the configuration method does not change the resulting method visibility. I'll merge this shortly.