Hi Jeremy & Janko,I like that proposal. It's somewhat a logical next step in the direction Janko and I have already gone--that is, creating a Rodauth instance with a fake Rack env--just going a bit further with that fake request.Can Rodauth features easily add class methods to the auth class today or would it need some new "macros" to the feature declaration API? I imagine it shouldn't be too hard, but we'd likely want an upstreamed way to "new" up a Rodauth instance which would look not too dissimilar from what Janko introduced, except that it perhaps takes details of a route (I think just path and method should suffice).
From a consumer point of view for this library, I think it should work but for perhaps one or two allowances. I think it would be critical to be able to pass in a base rack env upon which the fake request would be built. We rely heavily on Rodauth's audit feature and augment it with a lot of metadata from the request environment, such as IP address. Furthermore, I have a feature which records the actor of an action, which may be distinct from the logged in user, for things like resetting a password or changing an email address (when performed by an admin). Here's a screenshot of the tool demonstrating how we use and rely on the ability to augment "API" actions with this metadata: https://i.imgur.com/hFZYVsH.png.
While today I do something like (simplified from actual):Rodauth::Rails.rodauth(:user, env: request.env) do# These defined by a custom feature of mine and augment metadataself.actor = current_adminself.audit_metadata = { reason: "https://support.domain.com/ticket/1234" }account_from_login(account.email)before_change_loginunless change_login(new_login)return ActiveRecord::RecordInvalid.new(account)endafter_change_loginendAs long as a base `env` can be passed in, I could meet my needs for both collecting IP as well as other metadata such as the actor, doing something like:env = request.env.merge({'audit.actor' => current_admin,'audit.metadata' => { reason: 'https://support.domain.com/ticket/1234' },})Rodauth.api(:user, env: env) # or however this is instantiated.change_login(account_id, 'new-login')What do you two think? I understand this might make for a bit more complexity as the requests would have to be built on a potentially dirty rack env (with existing query params, `rack.input`, and polluted session). However, I think with care it could be quite powerful and simple compared to alternative ways of capturing parent request info or other additional context. Further, this is what I am doing today with `Rodauth::Rails.rodauth` to good effect.
My idea is similar, but slightly different. The base method would be something like:Roda.rodauth.internal_request(:change_login, account_id: account_id, env: env, session: session, login: 'new-login')
env :: Sets default rack envsession :: Sets default sessionaccount_id :: Updates session key to set logged in account for internal requestlogin :: Sets login_param of request (for change password, this would be :password instead of :login)The reason for this design is this is only for handling entire requests. I don't want to offer a method such as Rodauth.api that returns a fake Rodauth::Auth instance, and then have people call methods on that. I think that is too easy to misuse. There will only be a class method, and it should probably return nil on success, and raise exception on failure.We will need to support a separate configuration for internal requests, that gets layered on top of base configuration. For example, internal requests are probably going to want to have modifications_require_password? set to false (since you aren't going to know the user's password) and require_login_confirmation? set to false (unless you want to pass both :login and :login_confirm when calling internal_request.It would be helpful to support the ability for features to set separate defaults for internal requests than they set for non-internal requests, so the user doesn't need to override as much to get something useful. However, supporting that would require moving this from a separate optional feature to making this part of the core, and I'm not sure I'm comfortable with that.Anyway, I'll probably start testing these ideas after the next release, and see if they actually would work in practice. Additional feedback would definitely be helpful.
On Fri, Jun 18, 2021 at 11:32 PM Jeremy Evans <jeremy...@gmail.com> wrote:My idea is similar, but slightly different. The base method would be something like:Roda.rodauth.internal_request(:change_login, account_id: account_id, env: env, session: session, login: 'new-login')env :: Sets default rack envsession :: Sets default sessionaccount_id :: Updates session key to set logged in account for internal requestlogin :: Sets login_param of request (for change password, this would be :password instead of :login)The reason for this design is this is only for handling entire requests. I don't want to offer a method such as Rodauth.api that returns a fake Rodauth::Auth instance, and then have people call methods on that. I think that is too easy to misuse. There will only be a class method, and it should probably return nil on success, and raise exception on failure.
We will need to support a separate configuration for internal requests, that gets layered on top of base configuration. For example, internal requests are probably going to want to have modifications_require_password? set to false (since you aren't going to know the user's password) and require_login_confirmation? set to false (unless you want to pass both :login and :login_confirm when calling internal_request.
It would be helpful to support the ability for features to set separate defaults for internal requests than they set for non-internal requests, so the user doesn't need to override as much to get something useful. However, supporting that would require moving this from a separate optional feature to making this part of the core, and I'm not sure I'm comfortable with that.
Anyway, I'll probably start testing these ideas after the next release, and see if they actually would work in practice. Additional feedback would definitely be helpful.So I started testing this idea today, and I think it will work. WIP patch available at https://gist.github.com/jeremyevans/dabf95f367109428686efa1c06b59071It would be great to get feedback on whether this approach is suitable. It's not complete yet. Most of the remaining parts are related to actual authentication. For example, a method like:
Roda.rodauth.login(:login=>'login', :password=>'pass')is kind of wonky, since it doesn't actually log in the session. It could raise an exception if invalid and return nil if valid, but I'm not sure that's a good API. For these I was thinking something like:Roda.rodauth.valid_login?(:login=>'login', :password=>'pass')which would return true or false. Currently, that isn't possible with the WIP patch, but it's not a difficult change to allow it.
I plan to continue working on this, but the sooner I can get feedback regarding it, the better.
Thanks,Jeremy
On Wednesday, June 23, 2021 at 9:19:32 AM UTC+10 jeremy...@gmail.com wrote:It would be helpful to support the ability for features to set separate defaults for internal requests than they set for non-internal requests, so the user doesn't need to override as much to get something useful. However, supporting that would require moving this from a separate optional feature to making this part of the core, and I'm not sure I'm comfortable with that.Perhaps that optional feature could set known good defaults as overrides instead? It would likely have to use post_configure or some new feature ability to ensure it takes precedence over the app config. That could be surprising though.
Roda.rodauth.login(:login=>'login', :password=>'pass')is kind of wonky, since it doesn't actually log in the session. It could raise an exception if invalid and return nil if valid, but I'm not sure that's a good API. For these I was thinking something like:Roda.rodauth.valid_login?(:login=>'login', :password=>'pass')which would return true or false. Currently, that isn't possible with the WIP patch, but it's not a difficult change to allow it.Hmm I think the nil-or-exception as a general behaviour would be a bit clunky to use for the intended use cases. It'd be something that I, and I believe others, would wrap with their own classes. But again, still better than what home grown approach I have now. Are you imagining a generic "it failed" exception or something tailored to each case. For instance, there may be several reasons why a login or password couldn't be changed.
I plan to continue working on this, but the sooner I can get feedback regarding it, the better.I'll read through the gist, perhaps experiment with it, and continue to mull on it and come back with any takeaways. Thanks for picking this up so promptly!
On Fri, Jun 18, 2021 at 11:32 PM Jeremy Evans <jeremy...@gmail.com> wrote:Anyway, I'll probably start testing these ideas after the next release, and see if they actually would work in practice. Additional feedback would definitely be helpful.So I started testing this idea today, and I think it will work. WIP patch available at https://gist.github.com/jeremyevans/dabf95f367109428686efa1c06b59071
On 24 Jun 2021, at 10:30, Jeremy Evans <jeremy...@gmail.com> wrote:
--
You received this message because you are subscribed to a topic in the Google Groups "Rodauth" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/rodauth/Ugod2tnbUXQ/unsubscribe.
To unsubscribe from this group and all its topics, send an email to rodauth+u...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/rodauth/CADGZSSecadxYbhnM94VOJZGmy9_oV2aR4T8%2BBDTBPAxYjMuUZA%40mail.gmail.com.
If anybody else wants to test this, I've pushed it to a branch on my up-to-date fork https://github.com/bjeanes/rodauth/tree/internal-requests (which was necessary to effectively test this inside my Dockerised project)Just as I sent my previous email, my project finished building.I'm about 10 minutes away from my destination actually so I've only had a chance to test momentarily:[1] pry(main)> Authentication::RodauthConfig.rodauth(:user).change_password(account_id: User.first.id, password: 'test-xyz!!')Rodauth::InternalRequestError: invalid_previous_password
from /bundle/bundler/gems/rodauth-ca1aa3a4a48c/lib/rodauth/features/internal_request.rb:120:in `_handle_internal_request_error'I don't see 'change_password_requires_password?' set to false in the patch, which might be worth considering.Likewise, `change_login_requires_password?` isn't overridden so:[2] pry(main)> Authentication::RodauthConfig.rodauth(:user).change_login(account_id: User.first.id, new_login: 'tes...@example.com')Rodauth::InternalRequestError: invalid_password
from /bundle/bundler/gems/rodauth-ca1aa3a4a48c/lib/rodauth/features/internal_request.rb:120:in `_handle_internal_request_error'Unfortunately, that's all the time I have for now.Thanks again. I'll try to do more testing in the coming days.
I also figured out a few other ways to simplify the patch so it doesn't require as many changes to Rodauth itself. Just need to finish the documentation and then this can be merged.
Hi Jeremy and Bo,I found time for the past few days to study the implementation of the new internal_request feature, and it's very interesting and well thought-through, thank you for the work!I like the solution of passing params directly to the internal request methods, it seems very ergonomic to me. On that note, there are cases where people might want to use additional params (e.g. a user's name field when creating the account), so I was wondering whether you'd accept a new `:params` option which would allow specifying raw params, e.g:RodauthApp.rodauth.create_account(login: "us...@example.com", password: "secret", params: {."name" => "Full Name" })
I also had one implementation question regarding `.configuration_name`. I noticed it's used in registering the internal subclass on the Roda class, however, in the `.internal_request` method, instead of:rodauth = roda_class.new(env).rodauth(configuration_name)could we instantiate the Rodauth object directly:scope = roda_class.new(env)rodauth = new(scope)I was curious if there were any benefits of going through the Roda#rodauth method, because the latter approach would help the internal class remain more internal (it wouldn't be visible in the rodauths hash on the Roda class).
Setting those keys allowed the code to proceed. In this instance, I plucked them from the parent request, but this wouldn’t work in a console so conveniently:
[25] pry(main)> Authentication::RodauthConfig.rodauth(:admin).change_password(env: request.env.slice('rodauth.user', 'rodauth.admin'), account_id: Admin.first.id, new_password: 'test-password-123!')
D, [2021-07-06T02:52:44.435235 #26] DEBUG -- : Sequel (4.8ms) SELECT * FROM "auth"."auth"."accounts" WHERE ("id" = '037c4802-7ab1-4352-be41-6da533bb2ef5') LIMIT 1
D, [2021-07-06T02:52:44.436520 #26] DEBUG -- : Query Trace:
app/lib/rodauth/features/app_db.rb:75:in `account_without_status_checks'
app/lib/rodauth/features/app_db.rb:53:in `app_db_record'
app/lib/rodauth/features/audit_metadata.rb:51:in `validate_actor_for_auditing!'
app/lib/rodauth/features/audit_metadata.rb:69:in `hook_action'
app/core/request_context.rb:13:in `with'
Completed 500 Internal Server Error in 22ms (ActiveRecord: 7.8ms)
Sequel::DatabaseError: PG::FeatureNotSupported: ERROR: cross-database references are not implemented: "auth.auth.accounts"
LINE 1: SELECT * FROM "auth"."auth"."accounts" WHERE ("id" = '037c48...
^
from /bundle/gems/rack-mini-profiler-2.3.2/lib/patches/db/pg.rb:110:in `async_exec'
Caused by PG::FeatureNotSupported: ERROR: cross-database references are not implemented: "auth.auth.accounts"
LINE 1: SELECT * FROM "auth"."auth"."accounts" WHERE ("id" = '037c48...
^
from /bundle/gems/rack-mini-profiler-2.3.2/lib/patches/db/pg.rb:110:in `async_exec'
However, as you can see, it still failed. This is something that I can probably work around and would be quite atypical, but it may represent a deeper issue, so I’ll elaborate
I have a post_configure
block to namespace all tables into a different PG schema in a somewhat shotgun approach:
# Adjust tables in-use to be under the `auth` schema:
methods.grep(/_table$/).each do |table_conf|
if method(table_conf).owner != self.class
self.class.define_method(table_conf) { Sequel[:auth][super()] }
end
end
I do it this way as part of my awkward way of sharing some configuration between the user config and the admin config, which have different features enabled (and thus use different tables). Others likely won’t do this and there may be a better way to achieve this (or shared configuration in general).
However, the exception above implies to me that the post_configure
block may be running twice which could cause various unpredictable problems if they are not idempotent (a guarantee which is, to the best of my knowledge, not part of the requirements for post_configure method before now).
Cheers,
Bo
Hi all,Spending a bit more time with this today. I had planned to earlier but have been a bit under the weather so not at the computer much.Straight away, I am hitting something I don't remember hitting on the train the other week. I haven't ruled out PEBCAK here, but with rodauth `master` + latest released rodauth-rails, I get a KeyError exception raised here https://github.com/janko/rodauth-rails/blob/64283292fecce4641b6c79678d4cb9731b8a3a4c/lib/rodauth/rails/controller_methods.rb#L12I stepped through it and it seems as though the internal rack env does not include a reference to Rodauth, as it would when mounted as middleware:```[3] pry(#<Authentication::RodauthController>)> request.env.keys.grep /rodauth/
=> []```I'm assuming something like `env.merge!(['rodauth', *configuration_name].join('.') => self)` belongs in https://github.com/jeremyevans/rodauth/blob/7f2a37780210694160fa418cf006109b99849739/lib/rodauth/features/internal_request.rb#L231-L240Though actually, to be consistent, I think we'd need to set a `rodauth.*` key for _each_ defined configuration, not just the current one. In regular usage, I have both a `rodauth.user` and `rodauth.admin` key in my env, allowing me to work with eitherOthers may not hit this initially, but it's because I have a `before_action` in `ApplicationController` that checks for a `current_user`, which consults the rodauth instance stored in `request.env['rodauth.user']`
> I do it this way as part of my awkward way of sharing some configuration between the user config and the admin config, which have different features enabled (and thus use different tables). Others likely won’t do this and there may be a better way to achieve this (or shared configuration in general).
Hey both,
Regarding the table config specifically, is it possible to perform it on the class level, as I suggested in a recent discussion thread? That would avoid the issue of #post_configure being re-executed for the internal rodauth class.
Ack.
Note that post configure is not necessarily called only once anyway.
Ack and good to know. I haven’t seen it run more than once for me yet so hadn’t seen this edge case, but I’ll make sure my post_configure
s are idempotent.
I’m not sure why internal requests would want to use it, since the value of the env key would be the same as self.
Nor do I. The reason this is coming up is because those internal requests still trigger the around_rodauth
code in rodauth-rails. Therein, there is code which is outside of Rodauth which uses the rodauth instance in those keys to do things like check if the user is logged in and to provide accurate links to rodauth pages.
I was mistaken about where those rack keys are set, so I suppose this isn’t a flaw per se and something that Janko may need to consider. However, it is the case that in an application with rodauth-rails, the “interface” between the app and Rodauth is one where those keys are present and so rodauth-rails may need to provide a way to set them even for internal request cases. If you want to push back on this, I can work around this by always setting them myself, but I think it could be cumbersome and I doubt I am likely to be the only one to hit it.
However, given that internal requests don’t trigger a request on the Roda app, that code doesn’t get executed. Thanks for raising this issue though, I think setting it in #_around_rodauth as Jeremy said would make sense, so I’ll try that
If I understand correctly from Janko’s message, then perhaps this is the answer to what I am raising above. If so, that is great news. Glad I ran into it in that case!
Just curious, are you already using the inheritance provided by rodauth-rails? This is the recommended way in rodauth-rails that should make sharing configuration comfortable.
I spiked it but ran into some issues. IIRC, it was something like only the configure
block in the parent class was running, or only the one in the child… my memory is a bit foggy. TBH, it was a quick spike and I had tabled it to come back to. Probably PEBCAK, but I am aware of this feature and it is the main reason I haven’t raised the shared config issue on the mailing list yet—I want to have another go at this with more time and attention to the details.
FWIW my “shared” config is pretty gross and looks something like:
common_config = ->(config_name) do
enable(:common_features, ...)
domain { ... }
base_url { ... }
remember_cookie_key "_remember_#{config_name}"
# etc
auth_clas_eval do
# mailer_send overrides
end
end
configure(:admin) do
enable(:admin_only_features)
instance_exec(:admin, &common_config)
# admin-specific config and overrides
end
configure(:user) do
enable(:user_only_features)
instance_exec(:user, &common_config)
# user-specific config and overrides
end
Regarding the table config specifically, is it possible to perform it on the class level, as I suggested in a recent discussion thread? That would avoid the issue of
#post_configure
being re-executed for the internal rodauth class.
I wrote this post_configure
early on in my integration when the various levels of metaclasses and other indirection was a bit overwhelming. I may have a fiddle as part of trying to switch to using inheritance.
I’ll manually keep setting rodauth.*
keys for my continued testing until Janko has something to test or further thoughts. My goal for today is to convert as many of our “programmatic” usages of Rodauth into some version using internal request feature and then come back with any further thoughts/reflections or gaps (of which I anticipate two: changing email without confirmation & generating a PW reset link without emailing it — but I think I could fix both by including an internal request-only feature which allows me to conditionally disable all emails).
Cheers,
Bo
Whoops, copy paste error there. The first “ack” was in response to:
It’s expected that the post_configure runs twice if you are using internal_request.
For anyone else reading, the support for the internal_request feature in rodauth-rails is being tracked here. With the current changes in the PR, I was able to successfully make internal requests inside the demo app.I wanted to discuss one more thing regarding the internal_request feature in Rodauth itself. Currently, the feature overrides #set_error_reason to raise an exception for regular request methods, or set return value to false for predicate methods. One limitation is that the exception object doesn't contain any field errors, which could be useful for displaying to validation errors to the end user, and also for better understanding why an internal request failed (the error reason might not always be enough). Because #set_error_reason is typically called before field errors are set, it's not currently possible to add it directly.So I wanted to propose the following solution. Instead of immediately raising in #set_error_reason, we could save the reason to an instance variable, and only raise when the flash error is being set. At the time the exception is raised, we'll have field errors available in the instance variable, which we could then set as an attribute on the exception object (e.g. #field_errors attribute). This would also allow for potentially including the flash error message in the exception as well, as that could also be shown to the admin user managing accounts (though the flash message might not always sound right when a person is managing the account for someone else).What are your thoughts on this?
--
You received this message because you are subscribed to a topic in the Google Groups "Rodauth" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/rodauth/Ugod2tnbUXQ/unsubscribe.
To unsubscribe from this group and all its topics, send an email to rodauth+u...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/rodauth/CADGZSScRSiF7Bs602u%2BYDUsKzkwSd6v81HtmuF%2BxzMjqn5e-TA%40mail.gmail.com.
The proposed attributes sound good to me, I opened a pull request.I was thinking whether we should still include the error reason in the exception message (maybe in parentheses), so that the caller immediately knows why the internal request failed, without having to retrieve the reason from the exception object, which can speed up debugging. In that case we would probably want an additional #flash attribute for retrieving the pure flash message. Let me know what you think, and I could add it to the pull request.
Hi Jeremy,
I was hoping to be able to use internal_request_configuration
to enable features. The use case is for creating admin accounts. We do not have create_account
feature enable for admins and instead programmatically create them. This is to eliminate the risk of a Rodauth sign up form being accessed.
I was hoping that to port this programmatic user creation, I could do something like:
# In Rodauth app
config(:admin) do
# ... elided ...
internal_request_configuration do
enable :create_account
create_account_set_password? false
end
end
# in usage
RodauthApp.rodauth(:admin).create_account(login: 'ad...@domain.com')
RodauthApp.rodauth(:admin).reset_password_request(account_login: 'ad...@domain.com')
However, I merely get a NoMethodError
here.
The way I do this today is, like all my existing programmatic usage, opening up a fresh instance (created with Rodauth::Rails.rodauth
and my own equivalent prior to that) and:
rodauth = self.rodauth(:admin)
admin = Admin.new(email: email)
return false unless admin.valid?
# Important to use Sequel's transaction management. The underlying connection is shared so it will cover
# ActiveRecord save, but the email send happens in a Sequel-owned `after_commit` hook.
rodauth.db.transaction do
id = rodauth.db[rodauth.accounts_table].insert({
email: email,
# We don't let admins change email address and we email them their initial password, so we can skip an
# account verification email and just create the account in a verified state:
status: 'verified'
})
admin.id = id
admin.save!
# Load the internal @account var
rodauth.account_from_login(email)
# Generate and store the password reset token
rodauth.send(:generate_reset_password_key_value)
rodauth.create_reset_password_key
# Email the new admin a password reset link
rodauth.send_reset_password_email
end
admin
What would you advise here?
Cheers,
Bo
Thanks Jeremy,
Indeed that worked!
I’m slowly whittling down my custom “API” usage and adapting it to use the internal request feature. It’s a bit slow-going because I’m having to move a bit of application code into the Rodauth app to match existing behaviour.
Mostly, this is going well though. For instance, my Admin creation is now covered by:
configure(:admin) do # ....
internal_request_configuration do enable :create_account create_account_set_password? false
account_initial_status_value :verified before_create_account do super() @admin = Admin.new(email: param(login_param)) end save_account do @admin.valid? && super() end after_create_account do @admin.id = account_id @admin.save! _set_internal_request_return_value(@admin) self.class.reset_password_request(account_id: @admin.id) end end end
One area where I’m using
Rodauth::Rails.rodauth
extensively (or my equivalent, before it existed) is for getting access to the*_path
and*_url
values.Rodauth::Rails.rodauth
returns an instance which gave access to it. It’s used a lot in tests to drive Capybara or assert current pages without duplicating the config. Other than that, it’s used for building nav and links outside of rodauth-owned pages (layouts, admin backend pages showing user URLs etc).I don’t see a nice way to do that with the internal request mechanism out of the box. Arguably, this is made worse by not being a pure Roda app, so perhaps this could be resolved by Janko having rodauth-rails publish “routes” in a more Rails like way, but I think this need may arise any time multiple configs exist and cross-linking auth pages is performed.
I could imagine writing a Rodauth feature which, for obvious security reasons, is only loaded inside
internal_request_configuration
, publishing a “route” that returned all other routes as data. Alternatively, perhaps you’d consider simply exposing any*_path
/*_url
method automatically.
Hi Jeremy,> I think continuing to use the `Rodauth::Auth` instances to get `*_path` and `*_url` should be fine. We could add a feature to expose them at the class level, but all that will do internally is create an instance and call the methods. However, to me that seems like unnecessary complexity. However, I'm interested to hear what others think.Yeah it does seem a bit complex to do that. However, I have been under the impression throughout this that deprecating the instance-level monkeying around might not be endorsed. Janko put a note on the `Rodauth::Rails.rodauth` helper that it might be removed, for instance, so I think he may have the same impression.
What could solve this most nicely is to have a singular class-level method that returned a prepared instance, much like the internal request method already does. That is, if you do indeed want to sanction working with these. So `RodauthApp.rodauth(:config_name).instance` or similar. This would avoid the drift between `Rodauth::Rails.rodauth`'s setup of `env`, `session`, etc.
For now, I've created this `Authentication::API` class which allows me some conveniences and coalesces the "instance" and "class" level methods I need to work with. It's long, so it's in a Gist this time: https://gist.github.com/bjeanes/85eabe7fb03e73cb9b1e6bc7cc81a0f8.As you can see from that gist, I also still am using some internal methods of some features to perform actions. These so far have resisted porting to internal_request and are detailed below:* mark an account as verifiedfor reasons beyond our control and which I can't get into in this forum, we have deliver-ability issues with certain domains, in particular verizon, AOL, and yahoo. As such, we have need to manually mark accounts as verified after we have manually verified ownership of the email address out of band (our support system _is_ able to deliver emails to them). Currently, `verify_account` is exposed as an internal request method, but it requires knowledge of the key in order to succeed.
* confirm an email changepretty much same as above except after an email has changed on an already confirmed account
* generating password reset URLsame underlying issue -- our system can't deliver these so users are locked out and we have to manually generate a reset URL and send it via support system.
* _re-open_ an accounttypically this is when an account is closed erroneously or the user has changed their mind. I could probably just write a Rodauth feature for this which exposes its own internal route but it's kind of counter-intuitive as it could never be used as a real route (the user would have no way to log in)
I do not plan to remove Roda.rodauth (returning Rodauth::Auth class) or Roda#rodauth (returning Rodauth::Auth instance)
I hadn't given a significant amount of thought to exposing smaller things such as *_path or *_url, but doing so is fairly easy, just a few lines in post_configure. What are peoples thoughts on this?
I've been against doing that. The reason being is that it opens up the entire API to call on the returned instance, and due to Rodauth's stateful design and use of throw to return results, I think it would result in people misusing the API and potentially introducing security issues or bugs into their application. That's part of the reason the internal_request feature is designed the way it is, where instance of the internal request class is never exposed to the user.
However, I do realize it's possible to make the same mistakes by calling methods on the instance returned by Roda#rodauth, or inside any of blocks passed to configuration methods.
Maybe we can get by with an API like:
app.rodauth.instance(:account_login=>'f...@example.com') do
_path
end
This would instance_exec the block in the context of the internal request class instance, with the return value being the return value of the block. This avoids leaking the internal request class instance outside the block, unless you purposely have the block return self.
Jeremy --
You received this message because you are subscribed to a topic in the Google Groups "Rodauth" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/rodauth/Ugod2tnbUXQ/unsubscribe.
To unsubscribe from this group and all its topics, send an email to rodauth+u...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/rodauth/CADGZSSfPT-ZRdd6oin1oG8gArNZ6k7Yum_2ckiDZde0h5qP8Ew%40mail.gmail.com.
Hi,
Responses to both previous methods in-line. Hopefully the narrative makes sense…
Bo was referring to
Rodauth::Rails.rodauth
, a method thatrodauth-rails
defines. It creates a Rodauth instance in pretty much the same way that internal_request feature does now, so I would ideally like remove it in favor of the internal_request feature at some point in the future.
That’s right. It should either be pushed upstream (something like the instance
call I proposed and that Jeremy counter-proposed) or removed entirely with alternative provisions for the role it plays today (for me, at least).
I was also imagining that the internal_request feature could allow retrieving the Rodauth instance, but I understand the concern.
Yes, same. I am definitely also cautious of the statefulness here but it doesn’t get around the need to access instance-level data that I have.
However, I do realize it’s possible to make the same mistakes by calling methods on the instance returned by
Roda#rodauth
, or inside any of blocks passed to configuration methods.
That’s true. I actually liked from Rodauth’s design that some methods are public, allowing one to access most of the configuration. Some of the public methods are request-specific, though, such as
#login
and#login_required
, and these ones shouldn’t really be called outside of a request.
Agreed. Ideally there would be a way to access the “reader”-like methods only without incidentally making the mutative methods part of that API. I haven’t yet tested Jeremy’s suggestions for the remaining cases where I am using these internal methods but, trusting they work and there are no others, it would be ideal to have distinction between safe instance methods and unsafe.
Personally, now that internal_request feature provides an official API for performing full authentication operations, I don’t see much problem in allowing people to retrieve the Rodauth instance to use at their own responsibility. They know that the way to verify accounts is through the
.verify_account
class method, but if they choose to use the more fine-grained public methods thatverify_account
feature exposes, that’s up to them.
I’m inclined to agree. I wouldn’t mind having the escape hatch of digging into the internals as it has served me very well up until now, but making that a rarity by directly accommodating the primary needs could be sufficient.
So maybe I’m just being overly paranoid. Maybe we can get by with an API like:
app.rodauth.instance(:account_login=>'f...@example.com') do _path end
This seems like an appropriate escape hatch and learning experience to make safer and ordained methods for common usages of such an escape hatch. The gist I created is essentially my shim for doing this by combining internal_request
with Rodauth::Rails.rodauth
in lieu of an upstream Rodauth::Auth.instance
.
Another thought that came to mind was to have
Rodauth::Auth.instance
return a frozen rodauth instance, to prevent calling methods that change the state. I don’t know how much sense that would make.
This is a very interesting idea. It could get hairy, but I am intrigued…
The internal_request version of
verify_account
can work directly with the account without theverify_account
key. Similar for other features that deal with keys, such asverify_login_change
andreset_password
. Check the specs, you just need to provide:account_login
or:account_id
when calling the internal request method.
Huh. I read through the code in these features and couldn’t see how this was possible. In fact, I still don’t see how it works, as I don’t see anything which overrides or short-circuits account_from_verify_account_key
in the internal_request
feature. I’m embarrassed to say, I was so confident in my read of the code that I hadn’t tried to use it. I will do so today and come back if they don’t work as expected. Out of curiosity - how does this work?
Maybe try:
internal_request_configuration do reset_password_email_link do _return_from_internal_request(super()) end end
Something like this occurred to me after I sent that last email. Thanks for the confirmation.
How would this work for reopening accounts, though? When you close an account, almost all data about the account is deleted. I suppose we could just switch from the closed status back to the open status. However, that would only allow the user to login if there was some way for them to setup new authentication, such as using the
reset_password
oremail_auth
features. I guess I’m open to the idea, even if there are definitely configurations where it wouldn’t be useful. What are your thoughts?
Yeah, this is the tricky thing. My version of this does a reset_password
right after re-opening. I also have it consult other artifacts I have to determine if they were verified or not (though in the absence of this I think re-requiring verification would be fine).
The code is currently something like this:
def self.reopen_account(user:, **rest)
return false unless user.discarded?
raise "user has already been purged" if user.reaped? # we purge app data 30 days after account closed
api(user, **rest) do
transaction do
status =
if user.confirmed_at.present?
account_open_status_value
else
account_unverified_status_value
end
update_account(account_status_column => status)
add_audit_log(account_id, "reopen_account")
user.undiscard!
return user
end
end
end
I’m going to try to convert all of the verify features I thought wouldn’t work today.
Cheers,
Bo
I hadn't given a significant amount of thought to exposing smaller things such as *_path or *_url, but doing so is fairly easy, just a few lines in post_configure. What are peoples thoughts on this?
It would definitely be useful to be able to access the route helpers, especially in tests. It was already requested in rodauth-rails, though in the form of integrating rodauth routes into Rails, which would at least require Rodauth exposing the *_path or *_url helpers (currently they're accessible via `Rodauth::Rails.rodauth`).
I've been against doing that. The reason being is that it opens up the entire API to call on the returned instance, and due to Rodauth's stateful design and use of throw to return results, I think it would result in people misusing the API and potentially introducing security issues or bugs into their application. That's part of the reason the internal_request feature is designed the way it is, where instance of the internal request class is never exposed to the user.
I was also imagining that the internal_request feature could allow retrieving the Rodauth instance, but I understand the concern.However, I do realize it's possible to make the same mistakes by calling methods on the instance returned by Roda#rodauth, or inside any of blocks passed to configuration methods.
That's true. I actually liked from Rodauth's design that some methods are public, allowing one to access most of the configuration. Some of the public methods are request-specific, though, such as `#login` and `#login_required`, and these ones shouldn't really be called outside of a request.
Maybe we can get by with an API like:
app.rodauth.instance(:account_login=>'f...@example.com') do
_path
end
This would instance_exec the block in the context of the internal request class instance, with the return value being the return value of the block. This avoids leaking the internal request class instance outside the block, unless you purposely have the block return self.
Personally, now that internal_request feature provides an official API for performing full authentication operations, I don't see much problem in allowing people to retrieve the Rodauth instance to use at their own responsibility. They know that the way to verify accounts is through the `.verify_account` class method, but if they choose to use the more fine-grained public methods that verify_account feature exposes, that's up to them.
I'm not sure how I feel about `app.rodauth.instance(&:login_path)` for accessing rodauth methods. But if we had class-level *_path and *_url helpers, maybe it would be sufficient. I understand the motivation to keep the rodauth instance short-lived, and this API would definitely encourage that.
Another thought that came to mind was to have `Rodauth::Auth.instance` return a frozen rodauth instance, to prevent calling methods that change the state. I don't know how much sense that would make.
However, I do realize it’s possible to make the same mistakes by calling methods on the instance returned by
Roda#rodauth
, or inside any of blocks passed to configuration methods.That’s true. I actually liked from Rodauth’s design that some methods are public, allowing one to access most of the configuration. Some of the public methods are request-specific, though, such as
#login
and#login_required
, and these ones shouldn’t really be called outside of a request.Agreed. Ideally there would be a way to access the “reader”-like methods only without incidentally making the mutative methods part of that API. I haven’t yet tested Jeremy’s suggestions for the remaining cases where I am using these internal methods but, trusting they work and there are no others, it would be ideal to have distinction between safe instance methods and unsafe.
So maybe I’m just being overly paranoid. Maybe we can get by with an API like:
app.rodauth.instance(:account_login=>'f...@example.com') do _path end
This seems like an appropriate escape hatch and learning experience to make safer and ordained methods for common usages of such an escape hatch. The gist I created is essentially my shim for doing this by combining
internal_request
withRodauth::Rails.rodauth
in lieu of an upstreamRodauth::Auth.instance
.
The internal_request version of
verify_account
can work directly with the account without theverify_account
key. Similar for other features that deal with keys, such asverify_login_change
andreset_password
. Check the specs, you just need to provide:account_login
or:account_id
when calling the internal request method.Huh. I read through the code in these features and couldn’t see how this was possible. In fact, I still don’t see how it works, as I don’t see anything which overrides or short-circuits
account_from_verify_account_key
in theinternal_request
feature. I’m embarrassed to say, I was so confident in my read of the code that I hadn’t tried to use it. I will do so today and come back if they don’t work as expected. Out of curiosity - how does this work?
How would this work for reopening accounts, though? When you close an account, almost all data about the account is deleted. I suppose we could just switch from the closed status back to the open status. However, that would only allow the user to login if there was some way for them to setup new authentication, such as using the
reset_password
oremail_auth
features. I guess I’m open to the idea, even if there are definitely configurations where it wouldn’t be useful. What are your thoughts?Yeah, this is the tricky thing. My version of this does a
reset_password
right after re-opening. I also have it consult other artifacts I have to determine if they were verified or not (though in the absence of this I think re-requiring verification would be fine).The code is currently something like this:
def self.reopen_account(user:, **rest) return false unless user.discarded? raise "user has already been purged" if user.reaped? # we purge app data 30 days after account closed api(user, **rest) do transaction do status = if user.confirmed_at.present? account_open_status_value else account_unverified_status_value end update_account(account_status_column => status) add_audit_log(account_id, "reopen_account") user.undiscard! return user end end end
I’m going to try to convert all of the verify features I thought wouldn’t work today.
On Thu, Jul 15, 2021 at 7:39 PM Bo Jeanes <m...@bjeanes.com> wrote:So maybe I’m just being overly paranoid. Maybe we can get by with an API like:
app.rodauth.instance(:account_login=>'f...@example.com') do _path end
This seems like an appropriate escape hatch and learning experience to make safer and ordained methods for common usages of such an escape hatch. The gist I created is essentially my shim for doing this by combining
internal_request
withRodauth::Rails.rodauth
in lieu of an upstreamRodauth::Auth.instance
.I think I will implement this, but I'm planning on changing the name to internal_request_eval, because it will operate basically as instance_eval on the internal request.
Jeremy,
Maybe try:>
internal_request_configuration do reset_password_email_link do _return_from_internal_request(super()) end end
It took me a while to figure out why some tests were failing after this change, but I realise now it’s because _return_from_internal_request
does a throw(:halt)
. Unfortunately, that appears to have some show-stopping implications:
after_*
hooks for the relevant features don’t get called, even though the transaction commits.For the two locations that _return_from_internal_request
is used out-of-the-box (add_recovery_codes_view
and otp_setup_view
) these calls are already in the tail position of their handler, so doesn’t matter too much, though perhaps mean that the throw
is redundant anyway. WDYT?
I wonder if you meant to suggest the following, which does appear to work for these purposes.
internal_request_configuration do
reset_password_email_link do
_set_internal_request_return_value(super())
end
end
Cheers,
Bo
Jeremy,
Maybe try:>
internal_request_configuration do reset_password_email_link do _return_from_internal_request(super()) end end
It took me a while to figure out why some tests were failing after this change, but I realise now it’s because
_return_from_internal_request
does athrow(:halt)
. Unfortunately, that appears to have some show-stopping implications:
- This method is called inside the mailer, which means the mail doesn’t get sent
- Any
after_*
hooks for the relevant features don’t get called, even though the transaction commits.For the two locations that
_return_from_internal_request
is used out-of-the-box (add_recovery_codes_view
andotp_setup_view
) these calls are already in the tail position of their handler, so doesn’t matter too much, though perhaps mean that thethrow
is redundant anyway. WDYT?I wonder if you meant to suggest the following, which does appear to work for these purposes.
internal_request_configuration do reset_password_email_link do _set_internal_request_return_value(super()) end end
On 21 Jul 2021, at 01:08, Jeremy Evans <jeremy...@gmail.com> wrote:
--
You received this message because you are subscribed to a topic in the Google Groups "Rodauth" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/rodauth/Ugod2tnbUXQ/unsubscribe.
To unsubscribe from this group and all its topics, send an email to rodauth+u...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/rodauth/CADGZSScBpONthGAG31y%3D08CqtZ8jHo3Og5i%3D_1mOyGQBTKuh3w%40mail.gmail.com.
Ah I see. You’re right that I don’t want the email sent but I’ve handled that another way, as I do need the after_* to run. All good.Closer to green on CI I think. internal_request_eval has cleaned things up nicely, though Janko’s branch needs to be updated to pass the block along (I just woke up and haven’t checked yet, so Janko may already have done so).
Hi Jeremy,
I saw your message last week about wanting to release so I’ve been trying to fully integrate this branch to get you as much feedback as possible. Unfortunately I’m still not at green (it’s a long slog of working around things like my already_logged_in { super(); redirect(login_redirect) }
short-circuiting some internal requests etc).
That being said, I think if you wanted to release I’m pretty happy with where it’s at.
The biggest pain points remaining is how much I’m having to rely on internal_request_eval
for things like displaying whether a user has MFA enabled in admin dashboards, displaying warnings to users when they are low on recovery codes, etc., because all of those methods are not exposed in the “API” (and in fact are sometimes private). But, I think thanks to the eval escape hatch, I’ve wrapped in my own API
class to make it a bit nicer, and we can probably improve these things down the line in a backwards-compatible way anyway.
Thinking what it would take to expose that, I think my only hesitation is passing account_id
/account_login
with each method call, vs something like App.rodauth_api(:config_name, account_id: "...").some_internal_request
. If there is a case for exposing things like .otp_enabled?
, .recovery_codes
, .webauthn_usage
, etc., passing the account as arguments to these would seem cumbersome vs closing over this detail when instantiating the API. The call to some_internal_request
could still create a fresh rodauth instance to avoid pollution, of course.
Incidentally, this is the chief thing my own API
wrapper class works around. If you disagree on this ergonomic point, I think it’s fine for this to be solved in “user space” as it were. My API
class is here (along with a stripped down version removing the things specific to my custom features & app): https://gist.github.com/bjeanes/a48be85c35614fea4c31a53ae66e287d
I am continuing to try to get to green today with this branch and my fork of rodauth-rails (which preserves the &block
parameter for internal requests). If you can wait a few more days for release, I’ll have more confidence. But my degree of confidence is already pretty high.
Cheers,
Bo
Hi Jeremy,
I saw your message last week about wanting to release so I’ve been trying to fully integrate this branch to get you as much feedback as possible. Unfortunately I’m still not at green (it’s a long slog of working around things like my
already_logged_in { super(); redirect(login_redirect) }
short-circuiting some internal requests etc).That being said, I think if you wanted to release I’m pretty happy with where it’s at.
The biggest pain points remaining is how much I’m having to rely on
internal_request_eval
for things like displaying whether a user has MFA enabled in admin dashboards, displaying warnings to users when they are low on recovery codes, etc., because all of those methods are not exposed in the “API” (and in fact are sometimes private). But, I think thanks to the eval escape hatch, I’ve wrapped in my ownAPI
class to make it a bit nicer, and we can probably improve these things down the line in a backwards-compatible way anyway.Thinking what it would take to expose that, I think my only hesitation is passing
account_id
/account_login
with each method call, vs something likeApp.rodauth_api(:config_name, account_id: "...").some_internal_request
. If there is a case for exposing things like.otp_enabled?
,.recovery_codes
,.webauthn_usage
, etc., passing the account as arguments to these would seem cumbersome vs closing over this detail when instantiating the API. The call tosome_internal_request
could still create a fresh rodauth instance to avoid pollution, of course.Incidentally, this is the chief thing my own
API
wrapper class works around. If you disagree on this ergonomic point, I think it’s fine for this to be solved in “user space” as it were. MyAPI
class is here (along with a stripped down version removing the things specific to my custom features & app): https://gist.github.com/bjeanes/a48be85c35614fea4c31a53ae66e287dI am continuing to try to get to green today with this branch and my fork of rodauth-rails (which preserves the
&block
parameter for internal requests). If you can wait a few more days for release, I’ll have more confidence. But my degree of confidence is already pretty high.
Yup no worries. As I said, I can achieve the ergonomics I desire either way. :)
I’ll come back with anything else I hit. So far most of my time is sinking into the fact that a lot of the methods will return nil
on success but they also return nil
if they early-exit and it takes me a while to figure out why this is happening. I wonder if there is utility in explicitly returning true
when it reaches the end of the success branch and nil
for non-failure early exits (such as when a redirect occurs).
Cheers,
Bo
--
You received this message because you are subscribed to a topic in the Google Groups "Rodauth" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/rodauth/Ugod2tnbUXQ/unsubscribe.
To unsubscribe from this group and all its topics, send an email to rodauth+u...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/rodauth/CADGZSSctO8%3D_WAjnMGanJYfs9mLYj%3DPtD3oCh-MV04q%3DL9xQ%3Dg%40mail.gmail.com.
Jeremy,
Should env.merge!
in internal request feature perhaps ensure some of its local values wins? I know it’s a bit of a shot-gun approach to pass my whole parent request.env
in, as I do for auditing context, but clearly its essential that 'REQUEST_METHOD'
key remains as 'POST'
, for instance. Obviously, I can splice out the problematic keys if you think the caller should be able to do this.
One of the early nil
returns I just got was because the route actually didn’t match. So alternatively, perhaps an error path if the request didn’t match the intended route?
Cheers,
Bo
Jeremy,
Should
env.merge!
in internal request feature perhaps ensure some of its local values wins? I know it’s a bit of a shot-gun approach to pass my whole parentrequest.env
in, as I do for auditing context, but clearly its essential that'REQUEST_METHOD'
key remains as'POST'
, for instance. Obviously, I can splice out the problematic keys if you think the caller should be able to do this.One of the early
nil
returns I just got was because the route actually didn’t match. So alternatively, perhaps an error path if the request didn’t match the intended route?
--
You received this message because you are subscribed to a topic in the Google Groups "Rodauth" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/rodauth/Ugod2tnbUXQ/unsubscribe.
To unsubscribe from this group and all its topics, send an email to rodauth+u...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/rodauth/CADGZSSd4F3s1ba1bTJ2QWEik9SCUZ%2BtpMHxUrBpMKLhTzLQs6Q%40mail.gmail.com.
Yup no worries. As I said, I can achieve the ergonomics I desire either way. :)
I’ll come back with anything else I hit. So far most of my time is sinking into the fact that a lot of the methods will return
nil
on success but they also returnnil
if they early-exit and it takes me a while to figure out why this is happening. I wonder if there is utility in explicitly returningtrue
when it reaches the end of the success branch andnil
for non-failure early exits (such as when a redirect occurs).
The only one I recall currently is my custom already_logged_in
handler which redirect
ed. I don’t think overriding either of those out-of-the-box makes sense, though.
The other instances were also redirects but more complicated to explain. I have some ugly parts of my config, such as:
# Do our best to mask whether or not an account exists
# https://groups.google.com/g/rodauth/c/U-xxqes-ur4/m/1yzuLzdUGgAJ
reset_password_request_error_flash do
set_notice_flash reset_password_email_sent_notice_flash
redirect reset_password_email_sent_redirect
end
…which redirect in strange places. So that’s on me. Overriding redirect
to not :halt
for internal requests seems more likely to cause issues than solve them.
So far, I don’t think any of the instances I hit are anything which Rodauth should pre-empt, but anything which would make discerning between an early :halt
and a request reaching the success branch would make it easier to diagnose and harder for bugs to creep in with its usage, IMO
Cheers,
Bo
--
You received this message because you are subscribed to a topic in the Google Groups "Rodauth" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/rodauth/Ugod2tnbUXQ/unsubscribe.
To unsubscribe from this group and all its topics, send an email to rodauth+u...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/rodauth/CADGZSSdSrqBL3SjGnZnT8NfG4PrBZvZ7mvRZiJh7VHSTdHOTww%40mail.gmail.com.
The only one I recall currently is my custom
already_logged_in
handler whichredirect
ed. I don’t think overriding either of those out-of-the-box makes sense, though.The other instances were also redirects but more complicated to explain. I have some ugly parts of my config, such as:
# Do our best to mask whether or not an account exists # https://groups.google.com/g/rodauth/c/U-xxqes-ur4/m/1yzuLzdUGgAJ reset_password_request_error_flash do set_notice_flash reset_password_email_sent_notice_flash redirect reset_password_email_sent_redirect end
…which redirect in strange places. So that’s on me. Overriding
redirect
to not:halt
for internal requests seems more likely to cause issues than solve them.So far, I don’t think any of the instances I hit are anything which Rodauth should pre-empt, but anything which would make discerning between an early
:halt
and a request reaching the success branch would make it easier to diagnose and harder for bugs to creep in with its usage, IMO
--
You received this message because you are subscribed to a topic in the Google Groups "Rodauth" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/rodauth/Ugod2tnbUXQ/unsubscribe.
To unsubscribe from this group and all its topics, send an email to rodauth+u...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/rodauth/CADGZSScg3HdrtFkYi_O2%3DBbcHa_G2nva%3DO-J8Z%2Be59d6j0H4hA%40mail.gmail.com.
Jeremy --
You received this message because you are subscribed to a topic in the Google Groups "Rodauth" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/rodauth/Ugod2tnbUXQ/unsubscribe.
To unsubscribe from this group and all its topics, send an email to rodauth+u...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/rodauth/CADGZSSeuD27_jJuuZPzpUc4pCikRYTnb41UPWG-5jQcnBtoS7w%40mail.gmail.com.