Extracting complete actions into separate methods

[Janko Marohnić]

Bo Jeanes and I have been discussing how best to improve Rodauth's programmatic interface, mainly to make it easier to perform actions on accounts that are not logged in. In addition to some improvements in the builder API in rodauth-rails, we both thought it would make sense to extract complete Rodauth actions together with hooks into separate methods.

I started experimenting with this, and so far it seems like it should be fairly straightforward, and shouldn't affect backwards compatibility. For example, verifying login change for an account programmatically would then look something like this:

  rodauth.account_from_verify_login_change_key(key)

  rodauth.complete_login_change

While password reset could look like this:

  rodauth.request_password_reset

  # ...

  rodauth.account_from_reset_password_key(key)

  rodauth.reset_password(password)

Would you accept pull request(s) which extracts some of the more common operations using this approach? As you can see from the linked commit, I'm still deciding on the best naming convention, let me know if you have any preferences.

Kind regards,

Janko

[Jeremy Evans]

I don't want to go that direction.  One issue with the implementation you are proposing is it skips the checks that Rodauth has.  For example, your proposed reset_password method doesn't check that the password meets the password requirements, and allows resetting passwords for unverified accounts. Your proposed perform_login_change doesn't check that the login meets the login requirements.

I think programmatic access to the Rodauth API should come in the form of a class method that implements an internal request.  We could support something like:

   RodaApp.rodauth.change_login(account_id, new_login)

   RodaApp.rodauth.reset_password(account_id, new_password)

These class methods would do the equivalent of creating a Rack environment hash and Roda scope/request/response/session, setting the account id in the session (if the route requires authentication), then calling the Rodauth route (possibly extending the Rodauth::Auth instance with a module).  If there is an error in the Rodauth route, the methods would raise an exception.  Otherwise, they would return an appropriate value (nil in the above cases, potentially the created account id for a new account).

The reason behind this approach is that none of the logic interior to the Rodauth routes should need to change.

What are your thoughts on this approach?

Thanks,

Jeremy

[Jeremy Evans]

On Fri, Jun 18, 2021 at 5:24 PM Bo Jeanes <m...@bjeanes.com> wrote:

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).

I don't think there is currently support for adding class methods in Rodauth features.

 

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.

I'll keep that in mind.  In addition to setting a custom rack env, we also need support for a custom session.

 

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 metadata

      self.actor = current_admin

      self.audit_metadata = { reason: "https://support.domain.com/ticket/1234" }

      account_from_login(account.email)

      before_change_login

      unless change_login(new_login)

        return ActiveRecord::RecordInvalid.new(account)

      end

      after_change_login

    end

As 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 env

session :: Sets default session

account_id :: Updates session key to set logged in account for internal request

login :: 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.

Thanks,

Jeremy

[Jeremy Evans]

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 env

session :: Sets default session

account_id :: Updates session key to set logged in account for internal request

login :: 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/dabf95f367109428686efa1c06b59071 

It 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

[Bo Jeanes]

On Wednesday, June 23, 2021 at 9:19:32 AM UTC+10 jeremy...@gmail.com wrote:

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 env

session :: Sets default session

account_id :: Updates session key to set logged in account for internal request

login :: 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.

I wasn't imagining that `.api` would return a rodauth auth instance, just some object which had the API surface area on it. I think that would mitigate the misuse you're concerned about. But, what you propose would work for my purposes as well, I think. I'll probably need to chew on it a bit to be sure.

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.

Ah very good point. I hadn't considered that aspect in a world where we are firing off internal requests to the existing code paths. I could imagine that getting hairy but not insurmountable.

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.

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 

It 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:

I'm only working a few hours today (and not at all for the rest of the week) but I'll try to give it a once over today and let you know any thoughts I have.

  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!

Thanks,

Jeremy

[Jeremy Evans]

On Tue, Jun 22, 2021 at 5:31 PM Bo Jeanes <m...@bjeanes.com> wrote:

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.

The way this currently works in the patch is that the Rodauth::Auth instance is extended with a module for the behavior that allows internal requests to work easily. Due to the use of extend, this would take precedence over configuration methods. I have plans to add a configuration method that will allow for overriding this behavior, so the user can always have control over the internal requests.

 

  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.

So far in the patch case, I think nil-or-exception will work fine for all cases the patch handles.  The way it handles failure cases is that the error reason is used as the exception message.  The error reason is fairly fine-grained.  So the exceptions will not be generic.

I've since extended this so that valid_login? can work and return true or false (though I decided that valid_login_and_password? is a better name).  The way this has been added will allow for things like being able to get an array of recovery codes via an internal request method.

 

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!

Great!  I look forward to your feedback.  I'll keep working on this tomorrow and post an updated version of the patch after I get the remaining methods implemented.

Thanks,

Jeremy 

[adam.me...@gmail.com]

Interesting discussion so far. This has been a pain-point in a few of my Rodauth integrations. I ended up doing something similar, where I have a Rodauth plugin that provides class methods that simulate a request, but also overrides a few methods such as `two_factor_password_match?`.

I audited the routes I use specifically with my plugin (there are some operations that pre-date the plugin and poke through Rodauth internals more explicitly), and they are the following:

/otp-setup

/recover-codes

For /recovery-codes, it's actually the return value and not any side-effect I'm interested in. This is not something I saw in the patch that Jeremy shared. I wonder if it's something we could handle as well.

[Jeremy Evans]

On Tue, Jun 22, 2021 at 4:19 PM Jeremy Evans <jeremy...@gmail.com> wrote:

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 

I've updated this patch.  The implementation is mostly complete, only documentation work is left.  I would still appreciate feedback, but after I finish with the documentation, I'm planning on merging the changes into master tomorrow.  That will make it easier to test, which will hopefully make it easier get any significant issues fixed before release.

Thanks,

Jeremy 

[Bo Jeanes]

I’m on a train for next two hours, so I’ll take a gander now

Sent from my iPhone

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.

[Bo Jeanes]

Jeremy,

Thoughts as I'm reading through your patch:

• I really like the choice to take stable params and use those as a basis to lookup the real param name based on configuration (e.g. pass `new_password: "foo"` translates to `{new_password_param => 'foo')`
• The introduction of `@configuration_name` means I can remove some custom code as I've also needed this for some of my (quite awkward -- but more on that another time) shared config/features between admin and users
• `_return_from_internal_request` with response data is much cleaner than I feared for handling this. Nice.
• Being able to test if we are in an internal request will be very useful for things like my extended auditing metadata feature
  

I've read through the whole patch and I think it looks very practical and clean.

I'm on slow internet here and on a laptop I haven't used in about 6 months, so the project I have Rodauth in is still trying to build Docker images needed to boot the app. If it succeeds before I run out of battery or arrive at my destination, I'll try to replace some of my custom admin actions with the ones provided by this feature and report back again. If not today and you already merge, I'll try to give feedback by early next week if not sooner from such an experiment.

Thanks for whipping this up. It really looks like it hits all the important concerns I had in mind.

Cheers,

Bo

[Bo Jeanes]

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.

Cheers,

Bo

[Jeremy Evans]

On Wed, Jun 23, 2021 at 6:38 PM Bo Jeanes <m...@bjeanes.com> wrote:

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.

Bo,

Thank you very much for your review and your testing.

The internal_request feature overrides modifications_require_password? to set it to false, and most of the other methods that require password are set to default to that.  So the only way a password would be required is if you explicitly overrode change_password_requires_password? instead of modifications_require_password?.  I guess there are reasons to do that. I don't have a problem with overriding the *_requires_password? methods to also default to false, so I'll make that change.

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.

Thanks,

Jeremy

[Jeremy Evans]

I think I found the bigger issue with this, which is that we cannot use a copy of the class for the internal request class, we need to use a subclass, otherwise configuration methods specified in the normal configuration override the default internal request configuration. So I made that change.

 

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.

I did some more refactoring on this and got the changes to the core fairly minimal.  Just configuration and configuration_name added to Rodauth::Auth, and internal_request_method added to Rodauth::Feature.  All tests are still passing and I've finished the documentation work, so I've pushed the changes to the master branch: https://github.com/jeremyevans/rodauth/commit/d21277ddaae0b74ba70c86a8f08f467344085f76

Still plenty of time to make changes and tweak things if necessary before next month's release.

Thanks,

Jeremy

[Janko Marohnić]

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).

Kind regards,

Janko

[Jeremy Evans]

On Thu, Jul 1, 2021 at 1:49 PM Janko Marohnić <janko.m...@gmail.com> wrote:

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" })

Sure, I think that's a good idea, and meshes nicely with the support for :env and :session.  Can you submit a pull request for it?

 

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).

I think that makes sense.  Could you send in a pull request for that?

Thank you very much for your review!  If you don't have time for the pull requests, please let me know and I can work on it before the next release.

Jeremy

[Bo Jeanes]

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#L12

I 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-L240

Though 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 either

Others 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']`

Cheers,

Bo

[Bo Jeanes]

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

[Jeremy Evans]

It's expected that the post_configure runs twice if you are using internal_request.  It runs once for the normal configuration, and once for the internal request class configuration (the internal request class is a subclass of the normal class). This was added to fix the case where features were added to the internal request configuration that were not present in the normal  configuration.

Note that post configure is not necessarily called only once anyway.  It's called every time you load the rodauth plugin.  However, in most cases you only load the rodauth plugin once per configuration.

To work around this in your environment, you may be able to do:

  self.class.define_method(table_conf){...} unless internal_request?

Thanks,

Jeremy

[Jeremy Evans]

On Mon, Jul 5, 2021 at 7:48 PM Bo Jeanes <m...@bjeanes.com> wrote:

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#L12

I 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-L240

Though 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 either

Others 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']`

Rodauth the library doesn't set any rodauth keys in the rack environment as far as I know.  I know the README shows the setting of the rodauth key in env, though.  However, that's optional configuration. 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. I made changes to internal request to call the around_rodauth and before_rodauth hooks for internal requests, so possibly you could use those for any internal request specific environment modification.  I wouldn't be opposed to extracting methods to make this easier, assuming using the existing hooks doesn't work well.

Thanks,

Jeremy

[Janko Marohnić]

Bo, regarding the rodauth instances stored in rack env keys, rodauth-rails sets those before a request, so that they can be accessed in the controller. 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. In general I plan to add better compatibility with internal_request feature to rodauth-rails, such as applying Action Mailer default URL options and disabling controller instrumentation.

> 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).

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. 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.

[Bo Jeanes]

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_configures 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

[Bo Jeanes]

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.

​

[Janko Marohnić]

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?

[Jeremy Evans]

On Thu, Jul 8, 2021 at 10:41 PM Janko Marohnić <janko.m...@gmail.com> wrote:

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?

I like this idea. Maybe it would be best to add a InternalRequestError #reason, #field, and #field_error accessors, and have the message be the flash error message?  That way we can populate the InternalRequestError with as much data as possible before raising.

Thanks,

Jeremy

[Bo Jeanes]

I like this idea too.

--

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.

[Janko Marohnić]

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.

Kind regards,

Janko

[Jeremy Evans]

On Fri, Jul 9, 2021 at 11:59 PM Janko Marohnić <janko.m...@gmail.com> wrote:

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.

That makes sense to me.  I wonder if it would also make sense to add the field errors to the message as well?

Thanks,

Jeremy

[Janko Marohnić]

I was thinking about the exact same thing. I've just updated the PR to have the exception message contain flash, reason, and field errors. I've also merged #field and #field_error into a single #field_errors attribute, because it's theoretically possible that multiple field errors accumulate before an error flash is created. It probably doesn't happen at the moment, but I wanted to leave that door open in the future, since it's generally good UX for the user to see all validation errors.

Kind regards,

Janko

[Bo Jeanes]

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

​

[Jeremy Evans]

This should be a simple change of `klass.features` to `internal_class.features` in post_configure.  Assuming that works, we can make that change and add a test for it.

Thanks,

Jeremy 

[Jeremy Evans]

On Tue, Jul 13, 2021 at 7:44 AM Bo Jeanes <m...@bjeanes.com> wrote:

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.

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.

Thanks,

Jeremy

[Bo Jeanes]

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 verified

   for 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 change

   pretty much same as above except after an email has changed on an already confirmed account

* generating password reset URL

   same 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 account

   typically 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)

Do you have any thoughts on these final (I hope) items?

Cheers,

Bo

[Jeremy Evans]

On Tue, Jul 13, 2021 at 5:23 PM Bo Jeanes <m...@bjeanes.com> wrote:

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.

I do not plan to remove Roda.rodauth (returning Rodauth::Auth class) or Roda#rodauth (returning Rodauth::Auth instance).  Mostly because Roda#rodauth is the most public API next to r.rodauth (Roda::RodaRequest#rodauth), and it uses Roda.rodauth to get the class.

The internal_request feature is designed to expose entire actions.  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?

 

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.

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.  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 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.

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 verified

   for 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.

The internal_request version of verify_account can work directly with the account without the verify_account key.  Similar for other features that deal with keys, such as verify_login_change and reset_password.  Check the specs, you just need to provide :account_login or :account_id when calling the internal request method. Example:

  app.rodauth.verify_account(:account_login=>'f...@example.com')

* confirm an email change

   pretty much same as above except after an email has changed on an already confirmed account

Same as verify_account:

  app.rodauth.verify_login_change(:account_login=>'f...@example.com') # the login is the old login, not the new one

 

* generating password reset URL

   same 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.

Maybe try:

  internal_request_configuration do

    reset_password_email_link do

      _return_from_internal_request(super())

    end

  end

 

You could make the above conditional if it's only needed in certain cases and not others.

* _re-open_ an account

   typically 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)

There is a precedent already for something like this, in that the lock_account internal request method is supported, even though the lockout feature doesn't support it for non-internal requests (because nobody should want to forcibly lock their own account).

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 or email_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?

Thanks,

Jeremy

[Janko Marohnić]

Hi Jeremy,

I do not plan to remove Roda.rodauth (returning Rodauth::Auth class) or Roda#rodauth (returning Rodauth::Auth instance)

Bo was referring to `Rodauth::Rails.rodauth`, a method that rodauth-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.

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.

Kind regards,
Janko

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.

[Bo Jeanes]

Hi,

Responses to both previous methods in-line. Hopefully the narrative makes sense…

Bo was referring to Rodauth::Rails.rodauth, a method that rodauth-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 that verify_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 the verify_account key. Similar for other features that deal with keys, such as verify_login_change and reset_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 or email_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

​

[Jeremy Evans]

On Thu, Jul 15, 2021 at 9:52 AM 'Janko Marohnić' via Rodauth <rod...@googlegroups.com> wrote:

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 think it may be useful to add a separate feature for this, that exposes the *_path and *_url helpers as class methods.

 

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.

Yes.  Unfortunately, which methods are safe and which methods are not isn't really documented.

 

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 guess I agree.  By limiting access to the block, I think we can prevent the worst of the issues.  The block can return self, but any user doing that can be assumed to take responsibility for it.

 

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.

As mentioned above, I'm thinking of a separate feature for the *_path and *_url helpers.

 

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.

I don't think that is worth doing that.  Some Rodauth methods change state simply to make it possible to call later methods that use the state, even though they don't modify the database (stored state).  And some probably modify the database without modifying the instance.  Plus, it's definitely possible to have valid usage that modifies state and is safe.

Thanks,

Jeremy

[Jeremy Evans]

On Thu, Jul 15, 2021 at 7:39 PM Bo Jeanes <m...@bjeanes.com> wrote:

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.

Documenting that would be quite challenging, considering the scope of Rodauth's API.  I agree it is a good ideal, though.

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.

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. 

The internal_request version of verify_account can work directly with the account without the verify_account key. Similar for other features that deal with keys, such as verify_login_change and reset_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?

The internal_request feature overrides account_from_key, which the other account_*_key methods call.  If the internal request already has an account set, instead of checking if key matches, it just checks that the account has the matching status.  If no account is set, then the key has to be provided (similar to how regular requests are handled).  All of this behavior is documented in doc/internal_request.rdoc.

 

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 or email_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.

This is interesting, but I don't see a way to generalize it to handle all applications.  As it shows, you cannot even figure out what status you should use without knowledge recorded outside of Rodauth.  So I don't think I'll add support for reopening accounts.  You'll be able to take care of this using internal_request_eval.

Thanks,

Jeremy

[Jeremy Evans]

On Fri, Jul 16, 2021 at 8:30 AM Jeremy Evans <jeremy...@gmail.com> wrote:

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 with Rodauth::Rails.rodauth in lieu of an upstream Rodauth::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. 

I implemented internal_request_eval at https://github.com/jeremyevans/rodauth/commit/6d482203af6e2bf662d094f1c8546793ab910738.

I fixed the issue with calling internal request methods for features only loaded into the internal request configuration at https://github.com/jeremyevans/rodauth/commit/2b21c899734bec05c366aafe7db4d837d8827292

I added the path_class_methods feature at https://github.com/jeremyevans/rodauth/commit/eb38166427943bdfd44a80ec6de45f5726585122

Please give these a shot and let me know what you think.  I was planning for a release next week, so if you think any other changes should be made related to this, please let me know.

Thanks,

Jeremy

[Bo Jeanes]

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:

1. This method is called inside the mailer, which means the mail doesn’t get sent
2. 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 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 Evans]

On Mon, Jul 19, 2021 at 9:48 PM Bo Jeanes <m...@bjeanes.com> wrote:

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:

1. This method is called inside the mailer, which means the mail doesn’t get sent
2. 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 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

My understanding was you wanted a method that returned the link without sending the email, presumably so you could use the link in some other manner. If you do want to send the email and also wanted the link returned, then using _set_internal_request_return_value makes sense.

Thanks,

Jeremy

[Bo Jeanes]

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). 

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.

[Jeremy Evans]

On Tue, Jul 20, 2021 at 2:31 PM Bo Jeanes <m...@bjeanes.com> wrote:

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). 

Bo and Janko,

Are you happy with the current internal_request design, or do you want to make any additional changes before release?  After release, backwards compatibility will become a concern, so I want to make sure this meets both of your needs.  I'd like to get the release out this week, but it's not a big deal if it slips to next week.

Thanks,

Jeremy

[Bo Jeanes]

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

​

[Jeremy Evans]

On Wed, Jul 21, 2021 at 5:02 PM Bo Jeanes <m...@bjeanes.com> wrote:

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.

I think waiting a few more days is fine.

We can consider adding additional internal request methods later.  otp_enabled? and webauthn_usage both sound reasonable.  recovery_codes already exists.  I don't want to move away from providing the account_id/account_login to the internal request methods, though. 

Thanks,

Jeremy

[Bo Jeanes]

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.

[Bo Jeanes]

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 Evans]

On Wed, Jul 21, 2021 at 5:32 PM Bo Jeanes <m...@bjeanes.com> wrote:

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?

I think the passed in values should take precedence.  Passing in an existing request env is a bad idea, you should be careful to only pass the keys that you need.  I don't want to add defensive programming to try to handle cases where the caller is passing in values that don't make much sense.  I'm open to changing the documentation to make this more clear.

If you want to pass the existing request env in, pass it as a key of the env instead of as the env itself: env: {original_request: request.env}

Thanks,

Jeremy

[Bo Jeanes]

I understand that position. I'll exclude the problematic keys. I don't want to pass it in as a sub-request because then the auditing code gets even more complex by having to check two places and it isn't checking the env directly in all instances (e.g. headers).

--

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.

[Jeremy Evans]

On Wed, Jul 21, 2021 at 5:13 PM Bo Jeanes <m...@bjeanes.com> wrote:

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).

 Can you go let me know what cases are early exiting that are not errors but that you need to differentiate from the non-early exit case? I'll have to consider whether we want to do something differently for those cases.

Thanks,

Jeremy

[Bo Jeanes]

FWIW. I now have a green test suite! I am not using the new feature to expose paths yet, so that's next up on the agenda.

[Bo Jeanes]

The only one I recall currently is my custom already_logged_in handler which redirected. 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.

[Jeremy Evans]

On Wed, Jul 21, 2021 at 6:50 PM Bo Jeanes <m...@bjeanes.com> wrote:

The only one I recall currently is my custom already_logged_in handler which redirected. 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

I think the issue here is there isn't really a "success branch". In most cases, Rodauth will use a non-error redirect at the end of handling an action, which isn't really different from a non-error redirect used earlier.

If you want to turn error redirects into non-error redirects, I guess the easiest way to handle what you want would be to use _set_internal_request_return_value(true) inside the appropriate after hook for the action so you can tell how it was handled.  I don't think it's something Rodauth should do by default, though.

Thanks,

Jeremy

[Bo Jeanes]

I think I'm happy with where we are now and it's a definite improvement over poking inside the internals as much as I was. I've opted to tweak my `API` class a bit towards the following:

```rb

class API

  @@delegated_instance_methods = [
    :otp_interval,
    :otp_exists?,
    :account_webauthn_usage,
    /_path$/,
    /_url$/,
    /_redirect$/, # redirect locations
    /_subject$/,  # email subjects
    /_?$/,        # any predicate is unlikely to cause harm
  ].freeze

  # ...

  def delegated_instance_method?(meth)
    case meth.to_sym
    when *@@delegated_instance_methods
      true
    else
      false
    end
  end

end

```

Then delegated methods are called with `internal_request_eval { send(meth) }`.

This way, I have a clear documentation of where I'm relying on unexposed (or yet-to-be exposed) internals.

I'll away merging this branch until you and Janko both do respective releases, likely.

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/CADGZSScg3HdrtFkYi_O2%3DBbcHa_G2nva%3DO-J8Z%2Be59d6j0H4hA%40mail.gmail.com.

[Jeremy Evans]

OK.  I'll plan for a release tomorrow then.  Hopefully Janko is also happy with the current implementation, I haven't heard from him recently regarding it.

Thanks,

Jeremy

[Janko Marohnić]

Hi Jeremy and Bo,

Sorry, I've been pretty busy at work for the past two weeks, so I didn't get a chance to follow up.

I'm definitely happy with the current implementation, I think it will be a great improvement to Rodauth. I already have the rodauth-rails integration feature ready to be released as well.

Kind regards,
Janko

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.