As administrator, I created a batch of users on my site.
I gave the "anonymous user" the node module permission "access user profiles".
The anonymous user, was able to view the profile of some users, but not others - in the latter case, a "page not found" was displayed.
The problem could be fixed, by giving the anonymous user "administer users" permission - not a resonable fix.
I found out, that the anonymous user, was able to view the profile, of all users that had been logged in at least once. So, after logging on some of my newly created users, and logging out again - the anonymous user was able to view their profile as well.
Best regards
Bjarne
Comment | File | Size | Author |
---|---|---|---|
#65 | drupal_user_search_access-84490-65.patch | 758 bytes | hefox |
#65 | drupal_user_search_access-84490-65-status.patch | 775 bytes | hefox |
#65 | drupal_user_search_access-84490-65-d7-status-do-not-test.patch | 408 bytes | hefox |
#65 | drupal_user_search_access-84490-65-d8-status-do-not-test.patch | 428 bytes | hefox |
#56 | view_admin_created60_52.patch | 3.3 KB | beginner |
Comments
Comment #1
pwolanin CreditAttribution: pwolanin commentedI 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.
Comment #2
pwolanin CreditAttribution: pwolanin commentedhhhm, 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.
Comment #3
pwolanin CreditAttribution: pwolanin commentedAlso, 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".
Comment #4
bdragon CreditAttribution: bdragon commentedHere is the issue that lead up to that line getting how it is currently:
http://drupal.org/node/64893
Comment #5
pwolanin CreditAttribution: pwolanin commentedThanks 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.
Comment #6
netbjarne CreditAttribution: netbjarne commentedNice 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
Comment #7
AjK CreditAttribution: AjK commentedThis 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.
Comment #8
pwolanin CreditAttribution: pwolanin commentedThis 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.
Comment #9
pwolanin CreditAttribution: pwolanin commentedhere'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.
Comment #10
pwolanin CreditAttribution: pwolanin commentedComment #11
mhutch CreditAttribution: mhutch commentedSubscribing... 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,
Comment #12
ledelboy CreditAttribution: ledelboy commentedMy 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.
Comment #13
beginner CreditAttribution: beginner commentedIs 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.
Comment #14
mhutch CreditAttribution: mhutch commentedThere's still the patch to hide the users from search results. That can be reviewed and committed.
Comment #15
Dries CreditAttribution: Dries commentedhttp://drupal.org/node/73804 has been committed now. Does this take care of this patch too? :)
Comment #16
mhutch CreditAttribution: mhutch commentedNope, 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.
Comment #17
pwolanin CreditAttribution: pwolanin commentedI'll try to review/revise this
Comment #18
pwolanin CreditAttribution: pwolanin commentedThe 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.
Comment #19
beginner CreditAttribution: beginner commentedIs this title more descriptive of what you are trying to do?
Comment #20
pwolanin CreditAttribution: pwolanin commentedOk 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:
Comment #21
Dries CreditAttribution: Dries commentedI 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.
Comment #22
mhutch CreditAttribution: mhutch commentedCurrently 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.
Comment #23
mhutch CreditAttribution: mhutch commentedIn 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.
Comment #24
Kjartan CreditAttribution: Kjartan commentedPatch nitpicks:
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.
Comment #25
pwolanin CreditAttribution: pwolanin commentedAs 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.
Comment #26
drumm-1 will have to be documented in code.
Comment #27
pwolanin CreditAttribution: pwolanin commentedupdated patch attached- applies cleanly against HEAD, additional code comments incorporated as requested.
Comment #28
pwolanin CreditAttribution: pwolanin commentedSince drumm's only reservation seemed to be the code documentation, I think this is RTBC.
Comment #29
drummThis 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.
Comment #30
pwolanin CreditAttribution: pwolanin commentedI'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.
Comment #31
pwolanin CreditAttribution: pwolanin commentedOk, 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.
Comment #32
beginner CreditAttribution: beginner commentedGood 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.
Comment #33
pwolanin CreditAttribution: pwolanin commented@beginner- I think you may be mistaken- a non-existant user gives an "access denied"- see:
http://drupal.org/user/999999
Comment #34
beginner CreditAttribution: beginner commentedWell, 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.
Comment #35
pwolanin CreditAttribution: pwolanin commentedOk, 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.
Comment #36
beginner CreditAttribution: beginner commentedfor 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:
This is the proper handling.
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.
Comment #37
pwolanin CreditAttribution: pwolanin commentedlook 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().
Comment #38
beginner CreditAttribution: beginner commentedAck!!
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
Comment #39
beginner CreditAttribution: beginner commentedIt still doesn't explain why d.o behaves differently, though...
Comment #40
pwolanin CreditAttribution: pwolanin commentedPatch attached without the 3rd chunk. Should be RTBC- but another set of eyes, please.
Comment #41
beginner CreditAttribution: beginner commentedI wanted to review, but you forgot to attach the patch...
Comment #42
pwolanin CreditAttribution: pwolanin commentedyes, sorry!
Comment #43
RobRoy CreditAttribution: RobRoy commentedThat array needs to be indented 2 spaces, not 1.
'#type' => 'value',
'#value' => -1,
Comment #44
pwolanin CreditAttribution: pwolanin commentedAh, 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.
Comment #45
beginner CreditAttribution: beginner commentedI 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.
Comment #46
drummComment #47
pwolanin CreditAttribution: pwolanin commentedpatch attached for HEAD
Comment #48
pwolanin CreditAttribution: pwolanin commentedbump - probably needs to be re-rolled
Comment #49
Dries CreditAttribution: Dries commentedIf the patch needs to be re-rolled, please set the status to 'code needs work'. Thanks.
Comment #50
pwolanin CreditAttribution: pwolanin commented@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.
Comment #51
Gábor HojtsyIt 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.
Comment #52
pwolanin CreditAttribution: pwolanin commentedpatch 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.
Comment #53
dmitrig01 CreditAttribution: dmitrig01 commentedRTBC
Comment #54
Dries CreditAttribution: Dries commentedMeh, 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.
Comment #55
pwolanin CreditAttribution: pwolanin commented@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.
Comment #56
beginner CreditAttribution: beginner commentedThis 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.
Comment #57
Gábor HojtsyI 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.
Comment #58
catch#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.
Comment #59
Gábor HojtsyRetitling.
Comment #60
raspberryman CreditAttribution: raspberryman commentedHere is the current usability of admin-created accounts, as reported by one of my clients:
It would be sooo much easier if the admin just did step #1 without having to do steps #2 and #3.
Thanks all.
Comment #61
Gábor Hojtsyraspberryman: update to Drupal 5.5! Problem solved.
Comment #62
raspberryman CreditAttribution: raspberryman commentedScore! Thanks Gábor!
Comment #63
robertDouglass CreditAttribution: robertDouglass commentedI don't see that the real issue here has been solved, so upping to D7.
Comment #64
casey CreditAttribution: casey commentedStill an issue?
Comment #65
hefox CreditAttribution: hefox commentedAs far as I can tell, 7.x on has removed the 'access' check, so here's a simple, non-tested patch with the access <> 0 test for d6. It does not contain any admin form changes as that also seemed like a separate issue as it's an existing issue at viewing user profiles.
Since the subject doesn't contain about blocked users/is focused on access check, that seems like a separate issue (tried finding one, but wondering if it's a security issue and thus on security.drupal.org instead; However, it's already been talked about here several times). Any some further untested patches for that issue are also attached.
Comment #66
thedavidmeister CreditAttribution: thedavidmeister commentedAs far as I can see, it looks like #64893: fix to deter from registration SPAM was rolled back at some point, and fair enough too, that patch obviously caused more trouble and confusion that it was worth. I fail to see how it ever would have provided an effective measure against the creation of spam registrations as it is based on two dubious assumptions:
1. Spambots cannot log into your site after creating an account
2. Spambots crawl your site looking for visible signs of content they posted before attempting to post again
As Dries said in #54, we might as well just fix the bug (by removing the weird access check) than try to sweep it under the UI rug.