Exclude users never logged in from the search results

I just noticed this problem today as well- and just confrimed that the bug is present in HEAD/5.0 as well.

A user without "administer users" permission gets "Page not found" when trying to view the profile of a user who has never logged in. I cannot see in the user module code why this would be the case, though i'll keep looking.

hhhm, playing with the database, it seems the access must be non-zero to allow the viewing of a profile. I could then find this code, which is producing that behavior:

So, this seems to be a deliberate part of the code, but doesn't make sense to me, especially for admin-created user accounts.

Also, this is a real usability problem since the usernames will show up using search, but then clicking on the link will give "page not found".

Here is the issue that lead up to that line getting how it is currently:
http://drupal.org/node/64893

Thanks for that link- it certainly clarifies the reasoning behind the code. It's pretty trivial to go an set access to a non-zero value, but it would be better to have it be automatic, and also to retain the "never" for last login in the admin/user list. A (somewhat hackish) approach could be to set access to -1 for admin-created accounts, and define never logged in as access <= 0.

Nice detective work pwolanin :-) I think its a shame, if efforts agains spamming, spoils usability of drupal. The "access user profiles" permission is clear - given to anonymous users, these use should be able to "access user profiles".

If a user needs to be "activated" before being shown to the public, I suggest

a) that admin created profiles are automatically activated

b) un-activated profiles are NOT listed in search results and by custom profiles properties.

c) if drupal MUST throw an error when requesting un-activated profiles, the error should be more descriptive than just "page-not-found"

Bjarne

This patch prevents "blocked" users from appearing in search results (except for user UID==1, the admin account). For all other users (anon or otherwise) blocked users are suppressed from teh search list. Is this teh sort of thing people are looking for? Feedback welcome.

This patch is part of what is needed, but there are really two additional pieces (I'll try to work on code tonight)

1) admin-created accounts should get access set to a non-zero value, but it needs to be a special value that is still registered as "never logged in" for the user list. I'd suggest either 1 or -1.

2) the search code also needs to check for access == 0 as well as the improvement in your patch of not showing blocked users.

here's a patch against 4.7 to set access == -1 for admin-created users, and to still list these as access = 'never'

The patch against 5.0/HEAD should be very similar.

Subscribing... this is filling up my log and irritating my users!

This issue is related to http://drupal.org/node/73804, where the users are being listed in the profile listings and yet coming up as "not found". There may be similar problems with any other forms of user listing in other modules, so it's probably a good idea to replace the 404 with either an error message (to indicate that code needs fixing) or a more helpful user message,

My issue is slightly different: profiles can only be seen with administer user permission no matter if the users have logged in or not. Additionally, user lists are empty, although all profiles can be searched, found and read. I have posted a forum topic with a full description.

Is this a duplicate of that issue: http://drupal.org/node/87540 ?

@pwolanin : you offered to merge the two patches. What are you trying to do here that is fundamentally different from the other issue?

To show a 404 error for a user that has never logged in is the proper thing to do. In my experience, it has proved to be a very effective deterrent for spammers: they stopped trying registering fake accounts in my site after the patch referenced above was committed. If the user has never logged in, then they are not part of any community, and shouldn't be listed anywhere, but on the admin page.

As far as accounts set up by the admin, what's the point of setting up accounts for users who never use it? If they never use it, it's because they forgot the password, the email is wrong, or they are simply not interested. If they are not interested, why list their accounts to other users? If there is a mistake (email, password), as soon as it is corrected, people can login and they are then listed on the userlist.

The title of this issue is:
Only profiles of users logged in at least once can be viewed, unless having "administer users" permission.
for this reason I set this issue as "by design"! It is as it should be.

The problem is that such accounts should not be listed on the userlist, which confused other users and what several people really complained about here -> this is fixed in the patch here: http://drupal.org/node/87540. This patch should be committed soon to HEAD and 4.7 and will solve your problems.

If there is any other problem that I missed, maybe the best is to start another issue and offer a proper description.

There's still the patch to hide the users from search results. That can be reviewed and committed.

http://drupal.org/node/73804 has been committed now. Does this take care of this patch too? :)

Nope, we still need to remove inactive users from search results in user.module. Beginner's arguments in comment #13 have convinced me that the other proposed changes are best left out. The patch in the other issues was for profile.module, and hid users from profile listings.

pwolanin's path in comment #7 does this, and looks like a good start. I don't have CVS set up here so can't roll a proper patch, but I've edited that one to
* hide never active users as well as blocked ones
* check for the 'administer users' permission to find the hidden users (instead of uid==1)
* remove string interpolation

I think this does what we need.

I'll try to review/revise this

The patch in #16 misses the critical step of of setting access to -1 for admin-created users.

Attahced patch combines my patch in #9 + #16 for HEAD/5.0.

Is this title more descriptive of what you are trying to do?

Ok I think that overly long title covers it better. There are several small goals for this patch that add to the work done at http://drupal.org/node/87540:

1. distinguish in the database admin-created users from self registered uses. Right now Drupal doesn't show the profile of any user who has never logged in. As an example of why this is a problem: I've created accounts for all my committee memebrs, and I want their profiles to show even if they never get around to logging in.
2. show the profiles of these admin-created users, but also continue to accurately list their last login as "never" in the admin interface.
3. prevent self-registered users who never logged in from showing up in the search results. Right now, they show up but the link gives an error (I think "access denied") which isn't very user friendly.

I think it is perfectly OK to show admin created users that never logged on. What is wrong with showing these? Sorry but I don't get it yet.

Currently user.module 404's the profiles of user who have created accounts, but have never logged in. I understand that the purpose of this was to prevent spamming.

Admin-created accounts aren't spam, so the patch assigns an 'access' value of -1 to those (instead of the 0 that new accounts usually get). This means that the profiles will not be hidden by checks on 'access !=0'. The patch also hides users whose profiles are 404'd from the search results.

In actual reply to your question, you are right, and that is what the patch does.

* It hides the never-logged-in non-admin-created users from search results, as the profiles are already 404'd.
* It prevents never-logged-in admin-created users from being hidden, whether search results, profiles, or profile listings.

Patch nitpicks:

• The queries should probably be enclosed in double quotes to avoid escaping the single quotes.
• "access !=0 "should be "access != 0".
• There is a double space after the = in the last patch chunk: $form = array(.

I fail to see the need for the -1 logic though, seems like uncessary complication for no real benefit. What is the benefit from showing admin created users? The part with removing the unaccessed users from search results is fine seeing as they would just result in a 404.

As above, the advantage of showing admin created users is that I might want them to show up immediately as site users- for example I'm adding the members of a certain committee for an organization. This is important for me and, I assume, the others who are interested in this patch.

This patch is, in effect, partially counteracting an earlier patch which blocks viewing of never-logged-in users to prevent profile spam. Obviously, profile spam should not be an issue for admin-created users.

Attached patch addresses code style issues you mentioned.

-1 will have to be documented in code.

updated patch attached- applies cleanly against HEAD, additional code comments incorporated as requested.

Since drumm's only reservation seemed to be the code documentation, I think this is RTBC.

This needs to be consistent with the rules on the user view page. The search results should never present a link that is a 404. This is a bug that should be fixed in Drupal 5.

The different access time usage, I'm not sure if that is a behavior which should be changed for Drupal 5.0.

Queries with single quotes in them should be double quoted to avoid backslashes, like the original code.

I'll fix the quotes. I think this is already fine with the account page. Did you get a 404? Here's the relevant code:

Note that with the patch $user->access can be -1 and still shown in the search results, and -1 != 0.

Ok, there's something I don't understand (obviously) about the menu system, since the following behavior can be observed:

If the is no nid #999, but i go to /node/999 I just see the page /node

if there is no user #999, but i go to /user/999 I get "access denied".

To make matters worse, currently for a user with $user->access == 0, drupal returns "not found".

Thus, there is an information leak/inconsistency (though somewhat trivial), since Drupal behaves differently for uid values for never-logged-in users versus uid values for non-existent or blocked users.

Attached patch fixes the quotes problem and also this inconsistent behavior by changing "not found" to "access denied" in user_view.

Good catch on inconsistency on the node/999 where 999 does not exist.
It is a separate issue. I will prepare a patch for that and create a new issue, unless one already exists (I think one does).

For the user/999 bit however: I tested and user/999 gives me page not found, and the same should happen to users who have never logged in (excepting admin created accounts): this is by design as explained above.

The third hunk in your patch should therefore be removed.

@beginner- I think you may be mistaken- a non-existant user gives an "access denied"- see:

http://drupal.org/user/999999

Well, on my test platform updated to latest HEAD, I have a page not found which it should be.

If there is a bug there, then it is a separate issue. In any case, the check in your third hunk should return 'page not found': I keep repeating that this is by design, I said it in this issue a few months ago, and I repeat it.

d.o should also give page not found for http://drupal.org/user/999999 but I don't know the difference between d.o and my test site.

Ok, I'm really confused, since I get "Access denied" when I follow the link above.

However, playing a little more, I see that I get "page not found" if I'm logged in to my own site with "administer users" permission. Maybe you have more permissions on d.o than I do.

Anyhow, make this consistent should indeed be a separate issue. I'll re-roll the patch without that bit.

for node/nnn, there is this issue: http://drupal.org/node/82764. I knew there was an issue already, but I forgot it was me who filed it. I attached a patch there.

For user/nnn, I checked and at the top of the function user_view() you have:

I don't understand why drupal.org returns "access denied".

The code is right, and my test site behaves as expected. drupal.org doesn't.
And the patch still needs work.

look in user_menu(): there is a user_load() step there, which mean for non-existent accounts, user_view() is never called. This logic in user_menu() is somehow leading to the access_denied().

Ack!!

Some bugs are really stupid and obvious... but we are so conditioned to naming some variables with a specific name, that we do not see the mistake at all, never mind how many times we read the code!!!
Anyway, this is a separate issue: details and quick patch here: http://drupal.org/node/108579

It still doesn't explain why d.o behaves differently, though...

Patch attached without the 3rd chunk. Should be RTBC- but another set of eyes, please.

I wanted to review, but you forgot to attach the patch...

yes, sorry!

That array needs to be indented 2 spaces, not 1.

'#type' => 'value',
'#value' => -1,

Ah, the problem occurred because I copied and used as a template the form item above: $form['account']['notify']

Both whitespace issues fixed in attached patch.

I applied the patch, and tested it. I checked that everyone's concerns mentioned above where taken into account.
I think this is ready to go.

patch attached for HEAD

bump - probably needs to be re-rolled

If the patch needs to be re-rolled, please set the status to 'code needs work'. Thanks.

@dries- sorry for not changing the status before- I had ambitions to re-roll the patch last night.

Here is the re-rolled patch. I just re-tested and it works as expected.

It would be great, if the comment about the exclusion could be a bit more verbose about why is this done. The essence of the discussions above would be great.

patch didn't apply to HEAD due to user module changes. Patch is re-rolled with more extensive code comment. Tested it and all seems in order.

RTBC

Meh, I'm not really convinced by the motivations of this patch. It seems like it makes more sense to just remove the '$account->access == 0' check in user_view().

I'd like to discuss this some more -- it feels like we're solving this problem in an awkward way. What's wrong with showing users that never logged on? There are not much different from users that only logged in once for 2 minutes.

@Dries - the change to check the access time was done in this patch from about a year ago: "fix to deter from registration SPAM"

The motivation stated there is to prevent user profile spam from being visible. So, this patch is countering the effect of that patch for the subset of user accounts created by an admin. Obviously, rolling back that other patch is an option, though not ideal either. This patch also fixes a bug on the user search page - blocked users are currently shown in the search results.

This is exactly the same patch as #52, but rerolled to take into account the split into a different .inc file for the admin section.

Pwolanin has already replied to Dries's concern: we want to keep the registration spam determent but this does not apply to admin created accounts. The patch itself has been reviewed and tested numerous times before. It is ready to go.

I also feel a bit uneasy about using -1 for admin created users. I'd rather use the current timestamp as that would not break the existing API (other modules are possibly dependent on access being a true timestamp, a positive integer). Also we need to point to that previous issue, because the documentation was not there to epxlain this. The documentation added in this patch has quite some grammar issues, like "Exclude from the search blocked users and self-registered users" should be "Exclude blocked users and self-registered users from the search" I think and "to to prevent" does not look right either.

#171117 has an alternative approach for admin created users using time() so covers Gabor's concerns.

The search issue isn't dealt with over there, so why not split that out of the patch and deal with it on it's own here assuming 171117 gets in.

Retitling.

Here is the current usability of admin-created accounts, as reported by one of my clients:

1. Admin creates user
2. Admin goes to /devel/switch/username/ in order to update the 'access' timestamp field in the users table
3. Admin logs out, and then logs back in as Admin (unless using Masquerade)

It would be sooo much easier if the admin just did step #1 without having to do steps #2 and #3.

Thanks all.

raspberryman: update to Drupal 5.5! Problem solved.

Score! Thanks Gábor!

I don't see that the real issue here has been solved, so upping to D7.