Calling guest_user creates guest_user instead of checking if guest_user exists/is set

[asadakbar]

Just wanted to mention that creating a guest user just from calling guest_user is unintuitive and doesn't parallel the devise current_user. With current_user we can rely on it returning nil if the current_user isnt set. However trying the same with guest_user leads to the creation of a guest_user record instead of whether or not a guest record already exists. I encountered a bug in my code where I was checking if guest_user.

Is there an appetite for changing this?

[pacso]

We have a current_or_guest_user helper, which parallels the current_user method from Devise. guest_user is intended to always return the existing guest or create a new one if necessary.

[asadakbar]

In devise, current_user does not create a user record. It returns the user if its signed in or nil https://github.com/heartcombo/devise/blob/main/lib/devise/controllers/helpers.rb#L125. If this isn't something that the team is interested in changing, how would I check if a guest user is created without creating one when calling guest_user?

[pacso]

Can you explain what you're trying to achieve in your code? It might make it easier to suggest an appropriate solution.

[bbugh]

@pacso hello 👋

I ended up here looking for some way to disable automatically creating guest accounts. I was surprised to see how old this gem is and that this functionality didn't already exist!

While I can see a use case for automatically creating the guest user when accessed, in our use case we don't want it to happen automatically, we want a guest account to only be created once the visitor hits one of a few specific buttons (e.g. "try demo"). When a visitor becomes a user, even a guest user, it changes navigation and page behavior in many places, so for us it would preferably be created only when requested.

By adding two additional helpers to the gem's helpers, it allowed us to work around the problem:

diff --git a/lib/devise-guests/controllers/helpers.rb b/lib/devise-guests/controllers/helpers.rb
index 959d7b9..bd78700 100644
--- a/lib/devise-guests/controllers/helpers.rb
+++ b/lib/devise-guests/controllers/helpers.rb
@@ -14,6 +14,14 @@ module DeviseGuests::Controllers
       mapping = mapping.name

       class_eval <<-METHODS, __FILE__, __LINE__ + 1
+        def guest_#{mapping}_signed_in?
+          !!session[:guest_#{mapping}_id]
+        end
+
+        def #{mapping}_or_guest_signed_in?
+          #{mapping}_signed_in? || guest_#{mapping}_signed_in?
+        end
+
         def guest_#{mapping}
           return @guest_#{mapping} if @guest_#{mapping}

@@ -81,7 +89,7 @@ module DeviseGuests::Controllers

       ActiveSupport.on_load(:action_controller) do
         if respond_to?(:helper_method)
-          helper_method "guest_#{mapping}", "current_or_guest_#{mapping}"
+          helper_method "guest_#{mapping}", "current_or_guest_#{mapping}", "guest_#{mapping}_signed_in?", "#{mapping}_or_guest_signed_in?"
         end
         define_callbacks "logging_in_#{mapping}"
         set_callback :"logging_in_#{mapping}", :"transfer_guest_to_#{mapping}"

Then for example this lets us do things like in our visitor/guest banner:

<% unless user_or_guest_signed_in? %> 
  <%# this would actually create the guest only once they visit this link %>
  <%= link_to "Create your own thing", some_path %>
<% end %>

<% if guest_user_signed_in? %>
  You're editing this thing as a guest. Sign up now to keep your stuff synced across all your devices.
<% end %>

I think simply adding these helpers would cover all of the use cases.

Though, imo, it is still too easy to accidentally create a guest user record, and we have to code very defensively.

In my ideal world, it would be possible to disable this automatic user creation, and then if there was going to be a method to automatically create the guest, it would be a bang method, like current_account_or_guest that would return nil, and current_account_or_guest! that would create the guest account, but of course that would be a major version change and probably break a bunch of projects.

Thanks for reading! What do you think?

[pacso]

Hi @bbugh, thanks for your detailed message!

It sounds like you have 3 different types of user:

1. A "full" user - somebody who has signed up with their own email etc.
2. A "guest" user - somebody who has an account with details you've provided (a standard guest user that this gem would create for you)
3. An "unknown" user - somebody using the site, who doesn't have an associated account.

Now, depending on how you build your application, this still translates to 2 user types, but the guest user is capable of having a user profile, or not having one at all.

Rather than creating a 3 user states, and adding the confusion of current_or_guest_user being capable of returning a user, a guest or nil ... I think a better solution is as follows:

• Every session on your site is associated with a user account, either "full" or "guest" as mentioned above
• Full users have a UserProfile record associated with them, which stores their preferences
• Guest users can optionally create a UserProfile to allow them to trial certain features of your site. This isn't created by default though.
• Guest users without a profile will operate in the same fashion as your "unknown" user described above.

This way you remove the complication of a 3rd user type, whilst allowing you to continue offering the guest experience you currently do, and without unnecessary complication to this gem.

[bbugh]

Thanks for the response, definitely appreciate the time that you took to offer help a stranger. That seems like a good plan for the kind of functionality that might be needed in a common scenario. Could be worth a blog post or at least a stack overflow answer. 😄

I had discussed something similar with them, but at this point in the project's lifecycle it would be significantly more time and effort to change how the complex accounts and resources associations work, on code that hasn't been touched in a while, that doesn't have a lot of test coverage, with the original authors long gone, and I'm only doing a short-term contract to help them out with this feature to improve the conversions in their funnel.

Definitely understandable if y'all want to don't want to add features to a decade old gem like this. It's not too much of a lift to add custom guest functionality myself. Thanks again for the message, take care!

[pacso]

I can understand you not wanting to mess with code that isn't broken (and is poorly tested, etc).

It isn't that I'm against making changes to this project. Far from it in fact, but I'm definitely against making changes that don't make sense. In this case, I don't think they make sense.

The right thing here is for your app to separate the concerns of a guest user from a user profile. However, if that isn't feasible for the reasons you've stated, then patching your version of this gem is an OK workaround.

[bbugh]

I really appreciate your willingness to offer assistance! I genuinely value your perspective, I'm sure you've thought through a lot of challenges over the life of this gem. I hear what you're saying, and your idea does seem like a great way to cover common use cases. On the other hand, my experience over the last thirty years has taught me that in most cases there's never a "right thing"; the most we can hope for is a "best thing" (if we're lucky) or a "good enough thing" (most of the time) given the constraints and requirements.

In the interest of time and attention, I shared only a really tiny slice of the scope and requirements of the project. There's years-old BI reports to consider. Streaming ETL churn with constantly making "guests" automatically. User journey tracking through the funnel of visitor -> guest -> trial -> paid member. Functional overlap with this gem automatically creating users and the deeply integrated ahoy Visitor functionality. Other third party integrations, like PII with Sentry, and leads vs users in Intercom and Salesforce. No joke, we'd have to use a microscopic font if we were going to make an iceberg meme out of it.

My intent here was to open up the discussion to explore if y'all were amenable for me to contribute some changes that would allow us to customize the gem's behavior, by disabling or at least work around automatically creating guest users. I don't hold it against you at all if that doesn't make sense for the gem! Massive Rails projects like this are fairly rare. Covering ~80% of use cases with convention over configuration is a perfectly reasonable stance to take.

Open source maintenance is a big, often thankless job, and you shouldn't feel like you owe anyone anything. It's a nicely written gem, and I commend you and the other maintainers for that. Thanks again for the conversation and the work, take care!

[dvodvo]

I have wrestled a lot over this discussion and have arrived at one conclusion. While the discussion points to a situation of '3 different types of user', I believe that stating the problem that way can be misleading; it does not take into account the case where one does not want any user creation.

Illustration.

Premisse: authentication and authorisation are distinct concepts, but often travel hand-in-hand.

Assume a class ProfileUser which belongs_to User - and a related show action (as above). Assuming only users (indistinctly) can visit the page of their data for all sorts of reasons: both a current_user and a pre-existing guest_user according to the session should access the page.

But if a bot/bad_actor/stumbling_visitor comes to the url, one does not want to create a user, but redirect them away.
Otherwise, the consequence are: • creating a lot of unwanted records to the db • adding more actions to clean-up such records

a single assert_guest_user method would eliminate such a situation, leading to the use of something like:

if !current_user || !assert_guest_user
  return 404
end

This would be more efficient relative to implementing logic that also connects to the database more than once.

I realise this mixes authentication and authorisation, but devise does that in a certain sense given the current_user method.

[pacso]

Devise by itself is a 2-state system. You are either logged in and have a current_user, or you are logged out and have no user.

This gem (currently) respects that with a matching 2-state system. You are either logged in and have a current_user, or you are logged out and have a guest_user. All logged-out sessions are guests.

Now, if people want to a 3rd state, where you are logged out (nil user) by default and by request we create a guest or real user ... along with the migration of state from guest to user as already exists ... then I'm not necessarily against it, but it needs to be implemented in such a way that this is opt-in and does not conflict with the existing way people expect this gem to behave.

[dvodvo]

[sorry I updated original post to provide more clarity, I believe in the meantime of your response]

"...does not conflict with the existing way people expect this gem to behave." A most valid consideration.

In my assessment, this impies not altering existing processes and creating a neutral mechanism (not create, update nor destroy). An assert_guest_user method (maybe guest_user_exists is a better name so as to not create confusion in tests) is neutral and does/should not replace, nor affect existing methods.

"by request we create a guest". that is the present situation when current_or_guest_user and guest_user are invoked. asserting the presence of the guest user is independant of that unless someone modifies their code base and includes that somehow at a later point in time.

[dvodvo]

May I suggest that this prase be at the very top of the readMe information, before the installation.

This gem respects Devise's architecture in a matching 2-state system. You are either logged in and have a current_user, or you are logged out and have a guest_user. All logged-out sessions are guests.

It can be deduced, but it ought to be obvious.

[asadakbar]

@pacso Thanks for your responses on this. I stepped away from the project devise-guests was used on and never followed up on your messages, my apologies.

As you teased out, having a three tier user system was our use case with devise-guests. We had "visitors" with no data stored about them transition to a guest user as they went through a wizard which included storing some data. Then when they completed the forms and signed up for a full account, we transitioned them to a full user and utilized "current_user".

I appreciate your suggestion on collapsing the guest and visitor users into simply a guest user to simplify the architecture. However we would end up creating a lot more records that would need to be cleaned up and bake user handling into areas of the site that didnt need it.

Along with that reason, the initial gotcha with the current architecture was that the naming of the method vs its behavior was not intuitive. If current_user returns a user record or nil, then guest_user led me to believe that it would return a guest record or nil. There was nothing about the naming that clued me in to a database insert happening. Going by , this was surprising.

I'm not saying my way is right, or the way the gem is currently architected is wrong. Just bringing up something that I think could clear up some confusion for people using the gem. Happy to help with a pr if you are interested.

My thought would be to add a configuration flag which would be on and default to the current behavior of creating a guest user when guest_user is called. That config flag would be turned off with a major release so people have time to update their apps. And the creation of guest_user? which would return true/false, and the currently private method create_guest_user would become a public. What do you think? Also would appreciate your thoughts on this @dvodvo as your comments brought me back to my issue and had me thinking again about a solution.

[dvodvo]

I had to find a solution, because I really needed the three states: general public, guest_user, user.
In addition, a single visitor could gravitate from one to another, to all three states (potentially).
The following path respects the thinking of both gems, I believe.

The key resides in the fact that devise runs a session.
Thus, the application will generate a session[:guest_user_id] when a guest user is invoked.
That, or the devise user session is a single method applied to the methods that require it.

Thus, current_or_guest_user is invoked only on specific actions (basically when the visitor has to start initialising objects).

Relevant note : with turbo or , I suspect,turbolinks you may find they need to be turned off for some links, otherwise they end up creating unnecessary guest users.

[roco5754]

Thank you @dvodvo - you've saved me from having to manage thousands of pointlessly created guest user records. Putting your code in an application helper method works wonders.

def guest_user_present?
  # The devise-guests gem unfortunately will create a guest user any time one does not exist when 'current_or_guest_user' or 'guest_user' is called.
  # Guest user creation is not always the desired behavior - sometimes we just need to check if a guest user is present without actually wanting to create one. By checking if a guest_user_id exists on the session, we can determine guest_user presence in clean way.
  session[:guest_user_id]
end

[dvodvo]

You may have logic that changes controller behaviour (current_or_guest_user come to mind), based on one of the three states; thus I have this within an application method.