As the title says when anonymous uses are given access they cannot view user profiles error message saying page can not to found

Im running a XAMMP box running on local host pretty must check to see if the error appears on a virgin installation

any ideas

CommentFileSizeAuthor
#114 drupal.user-remove-that-half-baked-spam-protection.114.patch3.15 KBsun
#111 drupal.user-remove-that-half-baked-spam-protection.111.patch2.26 KBsun
#88 user_access.patch3.76 KBAnonymous (not verified)
#79 user_access.patch3.68 KBAnonymous (not verified)
#78 user_access.patch3.67 KBAnonymous (not verified)
#72 user_access-reverse.patch1.46 KBAnonymous (not verified)
#72 user_access-reverse-D6.patch1.48 KBAnonymous (not verified)
#72 user_access-reverse-D5.patch1.51 KBAnonymous (not verified)
#53 user_access.patch3.67 KBAnonymous (not verified)
#39 user_access.patch3.55 KBAnonymous (not verified)
#38 user_access.patch3.55 KBAnonymous (not verified)
#35 user_access.patch3.54 KBAnonymous (not verified)
#25 user-admin-edits-D5.patch1.55 KBbeginner
#22 user-admin-edits.patch1.47 KBcatch
#17 user-admin-edits.patch1.52 KBJirkaRybka
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

StevenPatz’s picture

Priority: Critical » Normal
Status: Active » Postponed (maintainer needs more info)
somes’s picture

Title: anonymous users cant not view user profiles when given access » anonymous users can not view user profiles when given access

Just tried this on a virgin installation, same thing when an anonymous user is given access to view an user account the permission isnt being given and returns a page not found error

can this be fixed

ShutterFreak’s picture

Same or similar issue here: regular (non-admin) users can not access user profiles of users that never logged in, as they get to see a "page not found" error. For users with admin privileges all user profiles are visible though.

Is it possible that an access check was implemented in a too restrictive manner, or is this by design?

Is there a way to make the profiles of never logged on users accessible to regular users?

somes’s picture

could this be looked at as there may be problems with viewing attachment when users that aren't logged in also - I havent got the skills to start debugging this but cant check test any patches - I need this feature ASAP

tks
M

Gábor Hojtsy’s picture

Hm, I looked into this with Drupal 5, because it was reported to be also an issue with Drupal 6.

- Used a fresh (as of today :) D6-dev install
- Went to /user/1 (in a browser where I am not logged in) and got access denied as expected.
- Gone to the permissions page and provided the anonymous user with access user profiles permission.
- Again went to /user/1 (in a browser where I am not logged in), and was able to view the profile.

Can you reproduce this issue with D6? (You opened an issue specifically for this).

Gábor Hojtsy’s picture

Erm, I tested with Drupal 6-dev, not Drupal 5.

somes’s picture

Yeh I tried that and its works fine for user/1

but when you add other users ie user/2 things begin to come unstuck
and the page not found comes back

can you see if you can replicate this

tks
M

ShutterFreak’s picture

For your information, I think the problem lies in user.module in the code for user_view(). More precisely in the 2nd line of the following code snippet:

  $account = user_load(array('uid' => $uid));
  if ($account === FALSE || ($account->access == 0 && !user_access('administer users'))) {
    return drupal_not_found();
  }
 

This code issues a drupal_not_found() if no corresponding account is found OR you are allowed to administer users and the user whose profile you want to view never logged in ($account->access == 0).

I suppose the idea was to disallow access to blocked users ($account->status == 0), not to users that never logged on.

Anyhow, I still prefer to see a different outcome for an invalid account ("page not found" error) or for access restrictions ("forbidden" error).

somes’s picture

Excellent

tks Olivier

freephile’s picture

This issue is described also in this thread http://drupal.org/node/66653

It does indeed revolve around the code that checks if the user->access time is zero.

Scenario:
Site administrator creates users, and detailed profiles including speaker bios for a seminar website.
Admin sets perms so that anonymous users do have priviliege to see user profiles.
The actual (registered) users have mostly NOT logged into the site for the first time since they are invited conference speakers
Marketing people heavily promote the site, but discover that anonymous users get a 404 response when requesting profiles that DO exist.

It seems like the behavior is a bug, and could be fixed by perhaps setting the access date to creation date (even when created through various methods like import?), or some old date (epoch) or removing the check at all?

Set that last accessed time to their created time, for example, and it will be fixed.

You can do this with the following SQL query in the Drupal database as a fix:

UPDATE users SET access = created WHERE access = 0;
TRUNCATE cache;TRUNCATE cache_content;TRUNCATE cache_filter;TRUNCATE cache_menu;TRUNCATE cache_page;TRUNCATE cache_views;

(The truncate caches are thrown in for good measure)

JirkaRybka’s picture

Another (real!) scenario:

The webmaster (admin) of community site receives a great article from someone, but that person didn't register to the site yet, being more of a VIP / outside-info-source than regular reader, and so sent the article via e-mail to the webmaster. The webmaster wants to provide valid author's profile, avoid accusation of stealing others' texts under own username, raise morale about the site being wide-community contributed, and allow the original author to make edits later on, so he create a new user profile for the author, submit the mailed text, and set it to be 'authored by' that user. But however, the author is satisfied by mere fact of his text being published, and never actually log-in to the site. Now, we have a great article on the front page, authored by never-logged-in user, so the author's details (correctly filled) are not accessible.

I didn't encounter the bug, because the site in question allows user-profile access only to authenticated users, but I confirm that it's entirely possible to have valid user profiles, which technically never logged in.

Gábor Hojtsy’s picture

If you look up why that login requirement was added, you will see that unless this is in, Drupal sites are user registration spam targets. Sites having profile fields for homepage or whatever which gets to be a link attract spammers. By registering lots of users, never logging in to your site, they can plant arbitrary number of links on your site, hijacking your pagerank for their own needs. That was the reason this restriction was added.

fractile81’s picture

So, a simple work-around would be to set the "access" field for a user you've created to 1 or the current time, or whatever is greater than 0. I ran into this problem migrating users to Drupal from a different database-type, and this solution worked for me. Of course, my site doesn't support registration without a live person first verifying then adding them, so this solution worked just fine for us.

JirkaRybka’s picture

Gábor is right, but then, definitely, it would be nice to have an option for admin to manually activate accounts if necessary (mainly the ones he created himself) without requiring the user to log-in. Or even better, having the admin-created accounts active by default (as fractile81 is saying above, if I got the point right).

ShutterFreak’s picture

Thank you for clarifying this issue, Gábor. I now understand why it is implemented the way it is. This approach makes sense in 'open' communities where users can register without requiring moderator approval.

In certain cases, e.g. where user registration is 'closed', this approach is less relevant. a number of solutions can be proposed, like:

  1. Provide a 'toggle' (new config parameter) enabling 'view user profile for users that never logged on' (default value: off). This could be a Drupal configuration parameter or a per group configuration parameter.
  2. Display a restricted user profile for users that never logged on (e.g., only user name).

For the time being I have edited the line that tests whether the user logged in, and it does the job for me. Of course all the groups I manage on this particular site are closed, and the admin has to manually add the new user to the group.

Best regards,

Olivier

JirkaRybka’s picture

http://drupal.org/node/182455 contains a patch (making the behavior configurable), but otherwise is duplicate (so I marked it as such).

JirkaRybka’s picture

Title: anonymous users can not view user profiles when given access » anonymous users can not view admin-created user profiles
Version: 5.2 » 6.x-dev
Status: Postponed (maintainer needs more info) » Needs review
FileSize
1.52 KB

OK, I think that I have a simple non-intrusive solution finally. The attached patch is for 6.x-dev (I know this was initially asked for D5 - it may get backported later). My approach is like this:

- There's absolutely no change to the viewing code, or to the anti-spam scenario. Newly created accounts are still blocked from anonymous viewing.
- But, if there's some administrator's activity on account which didn't log in yet (i.e. user saved under session with 'administer users' permission), it's now considered as an access to the account and time saved. Note that all new behavior only occurs with the 'administer users' permission.

So this means:
- Anonymous user registers -> access 'never', profile not visible (no change).
- User log in -> access time set, profile visible (no change).
- User edits his profile -> access unchanged (no change).
- Administrator created a new account (via "Add user" tab) -> The profile is considered as (non-spam) approved by the admin's manual activity, so access set to time created, and profile visible. This fixes the above described scenarios.
- Administrator edits (and saves) a profile, which was created by normal anonymous registration and never logged in yet (is currently invisible) -> Again, the admin's manual activity is understood as the profile being approved (either blocked or published, possibly with extra information added), so access time set to the time of edit, and profile visible. This fixes the scenario of admin needing to "publish" a previously existing profile.
- Administrator edits a profile, which already logged in (have access time set) -> access unchanged (no change).

ShutterFreak’s picture

Thank you for providing this patch!

Does your patch also address the following case, where logged in users can now access user profiles from new members added by an administrator and that naver logged in? I'm fine with disallowing anonymous users to access user profiles altogether as that's an open invitation for spammers.

Best regards,

Olivier

JirkaRybka’s picture

Logged in users can access the same (or more) than anonymous (not sure now)... Basically what this patch does, is that Drupal consider an admin-manually-edited/added account as already logged in once, so EVERYONE with the permission can access it.

Now we need some review, to be able of getting this into Drupal.

JirkaRybka’s picture

Re: #18 - Checked this, and yes, also registered users have the same behavior (i.e. new accounts invisible until first log-in, but with this patch, admin-created/updated profiles are visible). (BTW - If you want do disable access to profiles for all anonymous users, we have a permission for that.)

Another comment was said in the duplicate thread http://drupal.org/node/182455 - asking if there might be a case where admin creates accounts for people, but still wants to require log-in.

I think that is not a problem, the 'admin created = spam free' logic applies to this case too. User profiles are not really contents (no published/promoted/sticky and such options), the viewing restriction is just an anti-spam measure. So IMO there's no problem if admin-created user account is visible. If it was made for a person to really use, there would most probably be no more visible (yet) than the username anyway. The person in question might even find it confusing, being notified of a new account, but unable to visit it before login-attempt. (Also note that personal contact form is disabled by default, so no problem with that.)

This patch still needs review, to be able to proceed towards core. Applies cleanly, still. If going to test, please keep in mind that the change only applies to accounts NEWLY added/edited by an administrator.

JirkaRybka’s picture

Sorry for posting third time in a row, but apart from a notice that this still applies cleanly, I might also explain another use case (my own one):

Once I decided to prune the user registrations, because some stupid person registered a lot of common Czech names without really using the profiles. These profiles were never accessed, so after some waiting, I just deleted all never-accessed user profiles older than 6 months. Also good for performance, to reduce the users table to a half. But OOPS! There were also few profiles hand-created by another admin of the site, never logged in, but still 'owning' some nodes. :-/
This scenario is fixed by the patch, too - such profiles will not appear as 'never' accessed now (unless the other admin assigned nodes to a profile he didn't touch, which is much less likely).

catch’s picture

FileSize
1.47 KB

I tested this and it works great, this approach was also suggested by Gabor on a duplicate issue. I've made some grammar adjustments to the code comments, with which this should be RTBC hopefully.

catch’s picture

Status: Needs review » Reviewed & tested by the community

Since I only changed code comments slightly, marking RTBC.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Looks good, thanks, committed.

beginner’s picture

Version: 6.x-dev » 5.x-dev
Status: Fixed » Reviewed & tested by the community
FileSize
1.55 KB

I don't know why this approach was preferred to the other one.
With this patch, we loose information: there is no indication anymore of whether the said user has already logged in, because we set the last access time as if he had actually accessed the site (when in fact he has not).
The other patch (in the other issue) managed to preserve this bit of information.

Anyway, since the hierarchies have already decided, here is a patch for D5.

Gábor Hojtsy’s picture

As I said repeatedly, the access time is expected to be a positive number and we are already past beta2, so we are not changing our APIs here.

drumm’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 5.x.

Anonymous’s picture

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.

salvis’s picture

Status: Closed (fixed) » Active

It seems like we're not getting anywhere in http://drupal.org/node/202909, so I'll reopen this issue.

This patch has destroyed basic functionality and I'm looking for ways to restore it:

There are three timestamps in the users table, created, access, and login. What's the difference between access and login? Is access the time of the latest HTTP request?

The patch is mis-using access by setting it even though there's no access, but at least it's leaving login untouched. This means the access column in admin/user/user will never show "Never" again.

There are different ways to run a site:

  1. Create users and have Drupal notify them of their new account, inviting them to join. In this mode it's essential to be able to tell who has accepted the invitation and who hasn't (and may need some help or further encouragement), and the patch has completely broken this.
  2. Let users register themselves. Here, too, the patch has broken essential functionality, because the admin has lost the ability to easily tell the bogus registrations that never access.
  3. Fake active users when there aren't any. This is the one case where the patch helps, as JirkaRybka wrote in #19:

    Basically what this patch does, is that Drupal consider an admin-manually-edited/added account as already logged in once

    (The patch is actually faking access, not log-in.)
    This behavior is illegitimate -- as a user I'm offended if a site claims that I've accessed it, even though I haven't!

I don't understand why this patch, which favors the exotic case 3 at the expense of 1 and 2, was accepted as THE new behavior for Drupal. Couldn't this have been solved easily in a contrib module that sets access upon creation? If 3 really is core-worthy, then it should be an option that is off by default, IMO.

Fallback: If this quirky behavior is really here to stay, then a login column should be added to admin/user/user and the "Who's new" block should be driven by login rather than by access. Both of these are core functionality that was broken by the patch in D5 and D6!

Gábor Hojtsy’s picture

Status: Active » Closed (fixed)

Well, if this surfaced other bugs, then go and fix those bugs! "Who's new" does not sound like something which should work based on the last access time of users, neither the last login time, but rather on the registration time. If it is based off of the last access time, then it is seriously broken. Feel free to open new issues for bugs you found and post their links here. So far you seem to uncovered other bugs, which are showing up because this change, so I am setting this issue back to closed.

salvis’s picture

Status: Closed (fixed) » Active

I'm sorry to disagree.

If the admin of XYZ site creates an account for me and sends me the invitation, then I don't want my name to show up immediately in the XYZ "Who's new" block and people in the street telling me "Hey, I saw you joined the XYZ site."

It's MY decision whether I accept the invitation and actually authenticate with the site or not!. I haven't joined until I've actually gone there, and XYZ claiming anything else is illegitimate.

So, driving "Who's new" from the registration time (created, right?) would be incorrect. Driving it from access is correct -- not from the last access time, but from whether the user has accessed at all, and that's the vital piece of information that this patch has trashed.

Drupal must not force illegitimate behavior upon unsuspecting admins!

Please reconsider...

danaktivix’s picture

Subscribing to this issue

Anonymous’s picture

I as administrator want the state of access reserved if I update the user administratively. I as administrator want the state of access to be not set when I create the user to track the fact that the user hasn't logged in. Access is for the user accessing the site. It isn't for allowing non users to see who is new.

Yes please reverse this committed patch. As administrators we want to preserve the statistical history. The access column wasn't meant to be used as last updated by 'administer user'.

Michelle’s picture

Wow, I just found this issue due to the thread on the dev list. This "bug" has been plaguing me for a long time. When you use the profiles as nodes modules and have them created on registration, you end up with a node for these users whether they logged in or not. This then shows up in the user view as well as (as pointed out) the who's new block. Users then click on the users only to get a 403 error. It has even weirder effects when used with advanced profile with part of the page showing the 403 error.

Personally, I'd like profiles of people who haven't logged in to be just as accessable as people who haven't. At least can we make this a setting? I use other methods for spam control. I don't need this and think it's a very bad user experience to have profile links be clickable and then end up at a 403 error.

Michelle

Anonymous’s picture

Version: 5.x-dev » 7.x-dev
Status: Active » Needs review
FileSize
3.54 KB

I'm attaching this patch for discussion; it is untested. Does this patch seem reasonable for what needs accomplished? Do we need to separate the create and edit with different variables?

salvis’s picture

I believe users should be spelled as user's below:

If this box is checked, the users "Last Access" column will be set to a value representing the time of add or edit. This will allow the user to be seen by the blocks containing user data such as "Who\'s New".

I've applied the patch #35 to the head of the D6 branch, and it works just fine to disable the clobbering. Re-enabling also works fine for user creation.

Contrary to the text, under D6, editing the user doesn't clobber last access. Only creating a user through the admin interface does.

For D5, OTOH, editing also clobbers last access.

macgirvin’s picture

I'd like to step back to a higher level for a moment. There was a link spam problem. Changes were made to fight the spam problem by altering certain statistical information depending on how somebody got their account into the system.

Now we are finding all kinds of regressive effects because of that change.

Patches are being introduced which further the madness by special casing the hack and how we mangle the data. A maintainer two years from now isn't going to be able to figure out why this contorted logic is in place.

The link spam isn't a problem for some sites (firewalled intranets, approved registration, etc.), however the inability for anonymous folks to view profiles is potentially a problem for more sites than those with link spam problems. It touches more people directly overall. Any creative ideas on alternative ways to fight the link spam problem and revert any changes to the user data so that it isn't all such a dang mess?

If this is the best that can be done given time and resources, fine. But it's turning into a really bad hack.

Anonymous’s picture

FileSize
3.55 KB

@salvis: Thanks for the review and test. You should have changed the status to CNW but I've attached the fix the users vs user's observation. Your observation for D6 edit mode; I wonder if someone "fixed" the edit in CVS with a patch elsewhere? I know it was mentioned on the list that perhaps it should happen that user creation should be the only time the access is set.

Anonymous’s picture

FileSize
3.55 KB

Forgot to quote the quote.

salvis’s picture

Quote addition and quote quote addition verified.

Works great for D6 now.

Could be that someone has provided a partial fix, but admin creation and user access are still two completely different things.

sun’s picture

Status: Needs review » Needs work

@macgirvin: I totally agree. At least, there should have been a

// @see http://drupal.org/node/171117

in the original patch. Without the information in this issue, no one is able to understand this hack.

The real solution for this bug seems to be #64861: META-roles, user registration handling and SPAM registration. However, that's certainly D7 stuff only. So who comes up with an intelligent solution for D6 + 5?

macgirvin’s picture

Thanks @sun - I've subscribed over on #64861: META-roles, user registration handling and SPAM registration. I don't know what it would take to bring that issue back to life, but that's where I would (and others should) focus energy, not here. Looks like there's plenty of progress here on a hack that will probably work for D5,6. I just want to flag that once roles are fixed, we need to come back and undo the check for user->access in the profile access code and also all the mangling of user->access to convey data and perform things which were not access by a user.

Senpai’s picture

I'd love to test something, but I'm really not sure what we're going for here.

macgirvin’s picture

@Senpai - latest patch is #39. I'll bugger off and apologies for getting in the way. I'm interest in seeing the the core problems solved - and that looks to be a different issue.

sun’s picture

To summarize:

Before the original patch of this issue (#22) was committed

  • users without administer users permission can not access user profiles of users that never logged in, they get a "page not found" error
    • #8: This code issues a drupal_not_found() if no corresponding account is found OR you are NOT allowed to administer users and the user whose profile you want to view never logged in ($account->access == 0).
  • This behaviour
    • was intended to not display user profiles of new users until they logged on at least once, specifically: Drupal sites are user registration spam targets. Sites having profile fields for homepage or whatever which gets to be a link attract spammers. By registering lots of users, never logging in to your site, they can plant arbitrary number of links on your site, hijacking your pagerank for their own needs.
    • was a problem for site owners that administratively created user accounts, because no one was able to access them afterwards.

After committing the patch, Drupal's user account creation behaviour changed (see #17 for details), concerns were raised:

  • The Last access information in admin/user/user displays wrong data for administratively created users.
  • Site admins are unable to track whether users (they created) actually logged in.
  • User profile related contrib modules rely on $account->access to decide whether a new profile (node) needs to be created and whether a user should show up in user listings.
salvis’s picture

@sun:

users without administer users permission can not access user profiles of users that never logged in

You seem to accept that this was a problem that needed fixing. I don't — on the contrary, IMO, this is exactly how Drupal is supposed to behave!

You missed one fundamental aspect in your summary:

Before the patch in #22, users who never logged in ("ghost" users), never showed up anywhere on the website, except for in the admin's users list. Consequently, there was no need to make their profile visible to anonymous users, because no one looked for them.

After the patch in #22, every user that the admin creates, immediately shows up on the site as if he were a real, active member of the site, which he obviously isn't.

Why should users without administer users permission even know about ghost users? Are we in the business of faking websites or in the business of actually running them with real users, who do log in occasionally, as protocoled by last access?

Before the patch, the administrator could force the username of a ghost user to appear by assigning him as the author of another user's post. Only in that case would normal users learn that the ghost user existed and might try to look his profile (which caused a 404).

That marginal problem was fixed by the patch in #22 at the cost of trashing basic, mainstream functionality.

If the idea is to keep core clean, then let's simply revert the patch in #22, and those who need ghost users to appear like regular users can write a trivial contrib module that implements that functionality (with our without a checkbox).

OTOH, if we don't mind having weird and unnecessary stuff in core, then let's at least make it optional, as earnie proposes.

Michelle’s picture

"You seem to accept that this was a problem that needed fixing. I don't"

I'll be sure to send all my users' complaints to you, then. :P

This is a real problem for people who use nodes for profiles and likely other places as well. Whether it should be fixed in Drupal or be made the responsibility of every contrib module that has issues with this, I don't know. Personally I'd like to see it fixed in Drupal. At the very least, it should be an option. I don't want my users clicking on names in the new users list and getting an unfriendly access denied page. I, as site owner, should be able to decide what I want to allow my users access to.

Michelle

salvis’s picture

@Michelle: Why do you need publicly visible ghost users (= users that have never accessed your site) on your site?

I'm not saying that multiple modules should do something, but that one tiny module can be written that implements hook_user('insert') and clobbers last access, for those who desire to populate their site with ghost users (and are willing to accept the downsides of that crude solution).

"Don't do in core what can be done just as easily in contrib" seems to be a universally accepted (and enforced!) guideline, and I'm puzzled about why it wasn't applied here.

I don't want my users clicking on names in the new users list and getting an unfriendly access denied page.

I agree completely!

Before patch #22 there were no ghost users in the core New Users list. Are you talking about a different New Users list?

As far as the core New Users list is concerned, patch #22 is solving a problem that wouldn't be there without patch #22.

Michelle’s picture

There are a lot more ways to list users than the list you get under admin. Any module that does lists of users whether it's views of profile nodes or outright views of users in views 2 or the member list module or a custom block or whatever will need to take care to filter out users that haven't logged in, yet. Or, Drupal can simply not go out of its way to deny access to those profiles.

I don't really get the idea of "Don't do in core what can be done just as easily in contrib" as then there's no point in moving CCK to core or views to core or any contrib module that proves itself essential. Even accepting that, though, in this case extra work is being done in core that requires contrib to undo it. The simpler solution, to me, is to take the extra code out of core and move _that_ to contrib if people need to restrict access to certain profiles.

Michelle

salvis’s picture

I was referring to the "New Users" list in the "New Users" block, not "the list you get under admin."

Ok, lists of users in general: do you not have to decide for each of those lists, no matter how they're created, whether you'll include blocked users or not? Likewise, I think you should decide whether to include ghost users or not.

The key to distinguish ghost users from others is the 0 value (= Never) in the last access timestamp. Patch #22 destroys this value and removes my ability to make this distinction (and more).

If pre-patch-#22 Drupal refused to show user profiles of ghost users to non-admins, that was a not-so-subtle hint that user lists for non-admins should not show ghost users either (as implemented in the core "New Users" list). If you showed them anyway, that was a bug in your code.

Maybe I'm overinterpreting a bit — #12 mentions only user registration spam as a motivation — and ghost user removal and the other useful benefits were all just lucky side-effects, but to me they are precious.

The fix to the profile problem is not to auto-magically turn ghost users into real users, but to fix your code, so that it filters not only on active but also on access. Yes, I think this is necessary, because ghost users shouldn't be shown to the public. If it's too great of a burden, then we need something better than patch #22, something without adverse side-effects, but I would really hate having ghost users show up on my lists.

Auto-magically turning ghost users into real users may be what some admins want (not as a fix for the profile problem but to populate their site, see #11), and this is IMO what should go into a separate module, because it serves a very special need (is certainly not "essential"), can be done just as well in contrib, and it partially breaks a core database column.

Speaking of "extra work is being done in core that requires contrib to undo it" — this gives me an idea: clobbering the Never or actual access is the extra work that patch #22 does in core, and if we can't manage to fix this issue, I'll write a contrib module to undo it. But as you imply, it doesn't really make sense to do it that way...

Senpai’s picture

The basic problem here is that we have a field in the {users} table called 'active' for whether or not that particular user has logged-in or not-logged-in, but we're hijacking the intent of the 'active' field so that Drupal can decide if a user's profile should be shown to the unwashed masses. Does this reek of insanity to anyone else?

The 'active' field stores a timestamp of the user's created date, access date, and login date. Now, it seems to me that if Drupal need to know whether to show profiles to a-nony-mouse users, it should be looking at the 'login' timestamp, and not the 'access' timestamp. Observe this scenario:

If a user creates their own profile, the 'access' timestamp will be 0 unless the site allows users to create a profile and immediately login. Thus, most users have an access timestamp of 0 until they've actually logged in. Great. this is exactly how 100% of us feel that Drupal should behave. However, administratively creating or editing a user results in their 'access' timestamp being tampered with. Not good.

The problem isn't with users, nor is it with administration. The root of the issue here is that core should not be staring at a "last accessed" time & date to determine whether anon surfers get to see a user's profile.

Why isn't core looking at the 'created' and 'access' and 'login' stamps to see if they all match up logically, rather than relying on a single field being greater than zero? I suspect that if a user was created, they'd have a 'created' timestamp. If they've never logged in, their 'access' and 'login' should both be 0 also. Thus, the logic might go something like this.

IF (login && !access) { user has never participated in the site. }

IF (created < (access || login) && (access < login) { user has never logged in, but their profile can be shown anyway }

Then, all Drupal has to do is change the login timestamp to time() whenever the user is manually edited or created. Does this make sense to anyone else?

Michelle’s picture

Well, I seem to be outvoted so I'm going to bow out of this discussion. If anyone can tell me how I can make a view of nodes that excludes those where the author has never logged in and therefore there profile will 403, I'd appreciate a note off issue. (Drupal 5.x)

Thanks,

Michelle

Anonymous’s picture

Status: Needs work » Needs review
FileSize
3.67 KB

@michelle:

This is a real problem for people who use nodes for profiles and likely
other places as well.

It is not a problem for Drupal core to fix the misuse of nodes. Storing profiles as nodes isn't that great of an idea. Sure there are some benefits gained from the API but that API was to display content typed by users; not the user data itself. I've seen the arguments, I tried the user profiles as nodes, caused me more grief that than I want to think about at the moment.

@salvis:

users without administer users permission can not access user profiles
of users that never logged in

You seem to accept that this was a problem that needed fixing. I don't
— on the contrary, IMO, this is exactly how Drupal is supposed to
behave!

I agree totally with salvis. This aggravate is nothing more than an aggression to cause the masses who love Drupal to become agitated and angered. Destroying data that obviously means one thing to become something to mean totally different is outright ludicrous. No one who is lucid would even dream of such laughable design. If you look at the comments of the original code you'll see that it was intentional for the users who didn't have a value for access to not be shown.

As salvis mentioned, a contrib module would have taken care of this easily but not now, a contrib module would have to modify and keep modified the Drupal core instructing the user to replace the users module with the contrib module. At least let's have a choice between lunacy and lucidness and let's get this patch applied or reverse the patch at #22 so a contrib module can be written with saneness in mind.

Attached patch contains the suggested comment at #41.

Michelle’s picture

So because you don't like the nodes for profiles solution, you think that I shouldn't get to decide on my own sites whether users can view profiles of people that haven't logged in yet? I don't like treating new users as second class citizens. That should be my choice. If this is the way that's going to be, then I guess I need to figure out how to override this in contrib or hack core. It's silly. Just because a person hasn't logged in, yet, that does not make them a spammer and I don't care to treat them as such.

FWIW, I'm not talking about all the topic changes in the thread and what the access time field is doing and whatnot. I'm talking about the topic of this issue, "anonymous users can not view admin-created user profiles". That is the bug I'm wanting to see fixed. If I'm alone in that, so be it. I'll just have to find my own solution.

Michelle

salvis’s picture

@Michelle: Please don't be offended — this is not against you personally, just my own personal opinion: I think it's dishonest to show users who have never accessed a site as if they were active members. And the patch in #22 makes it difficult to stay honest.

This has nothing to do about first or second class citizens, it's about not showing those as citizens who aren't.

#52: How do your ghost users post content if they never access the site? It occasionally happens that I post content for someone who doesn't have an account, but then I post it under my name with attribution to the author, and I take the responsibility for posting it. I'd never dream of making it look like they were active members of my sites.

Anonymous’s picture

So because you don't like the nodes for profiles solution, you think that I shouldn't get to decide on my own sites whether users can view profiles of people that haven't logged in yet?

No, I don't think that core should be changed so that I don't have a chance to do otherwise. The issue is about the change to core code that munges data for access statistics that there is no method to control. The issue is about the change to core code that could have been accomplished with a contrib module instead. You had a choice; I now don't have a choice other than to munge core code to the state prior the patch.

I don't like treating new users as second class citizens. That should be my choice.

And I don't like to allow a new user to the site without previewing first. With the patch at #22 I no longer maintain the fact that the user never logged in after I set the user to active from blocked. I can no longer remove those users based on never logged in because they appear to have been.

FWIW, I'm not talking about all the topic changes in the thread and what the access time field is doing and whatnot. I'm talking about the topic of this issue, "anonymous users can not view admin-created user profiles". That is the bug I'm wanting to see fixed. If I'm alone in that, so be it. I'll just have to find my own solution.

The patch at #22 is what gave you the ability in 5.7, 6.2 and 7.x-dev to do that and then broke the workflow for others. I'm trying to remedy the workflow for others by giving a choice to the admin in my patch at #53.

Michelle’s picture

@salvis - Adding a node without logging in is easy: they fill it out when they register. Imagine this situation: A new user signs up, fills out their profile, and then for whatever reason doesn't have a chance to log in right then. They come back to the site, see their name on the new member list right on the front page, think that's pretty cool, and click on their name. Bam, access denied! What a lovely first experience. Maybe your sites are so popular you can afford to turn off potential users. I can't.

@earnie - Since you seem to be completely ignoring the fact that I said I'm addressing the problem in the title of this issue and not the topic change that ensued, your arguments have nothing to do with what I'm saying.

If you all have ruled out the possibility of fixing the original issue and are just worried about the setting of the access time, perhaps you should change the title?

Michelle

salvis’s picture

@Michelle:

A new user signs up, fills out their profile, and then for whatever reason doesn't have a chance to log in right then. They come back to the site, see their name on the new member list right on the front page, think that's pretty cool, and click on their name.

This is not supposed to happen, IMO. Don't you send an email with a confirmation link? As long as they haven't proven that they've received the email that the site sent to the address that they registered, they haven't completed registration and their name shouldn't show up anywhere.

Adding a node without logging in is easy: they fill it out when they register.

They can post before having verified their email address??? Can they post without even registering? We seem to run Drupal with very different settings.

If you all have ruled out the possibility of fixing the original issue and are just worried about the setting of the access time, perhaps you should change the title?

No, we're not ruling that out, but we're trying to fix what the patch broke. Earnie proposed an option setting to keep the new functionality as an option. Then macgirvin said it was "turning into a really bad hack" in #37. Sun summarized in #45 and I added to his summary and tried to put the patch into the bigger context in #46. IMO, #22 is a really bad hack already, and if we don't like bad hacks, we ought to revert it and let someone put it into a contrib module.

You've brought forward the advantages that the patch has brought for you, and I'm trying to show you that it's not the right solution for your problem and detrimental to Drupal in general, and that the original issue may not be an issue with core but with the way you create your user lists. That's what we've been discussing for the last few posts, and it directly relates to the original title.

Michelle’s picture

Look, I dont' know anything about this last access time patch you are arguing about. I'm not using 7.x or even 6.x for that matter. What I know is that I've been having people come to me for help because only admins are able to access profiles created by the admin or users that haven't logged in yet and I've been going nuts trying to figure out how advanced profile could possibly be causing that only to find out that this was a deliberate choice made by core to make them inaccessable. What I am saying is this should be set by the site admin, not hardcoded into core. That's it. That's all I've been arguing. Whether this or that patch is a bad hack is irrelevent to this because, AFAIK, there is no patch that actually fixes the issue that started this thread, which is the same issue I am having.

And, FTR, no, I don't make them confirm their email first. I'm all about lowering barriers to using the site. They can fill out their profile at the same time they choose their username and password. To make further posts, they need to actually log in with this new info, which they may not do right away if they don't immediately have something to say. I'm not going to assume they are a spammer because of that and I don't want Drupal to do so either.

Michelle

salvis’s picture

@Michelle: This is about 5.x just as much as 6.x and 7.x, because patch #22 was also applied to 5.7 (#27), and I think it did "fix" your problem with users complaining about advanced profile, but at a much greater cost. You're right in the middle of this. :-)

I'm in favor of enabling you to do it your way, but not at the cost of core trashing valuable data that makes it difficult to have it another way, because the data is gone. We have to revert the data trashing and look for a non-destructive way to satisfy your requirements. Won't you agree to that?

Anonymous’s picture

@Michelle: When did your problem begin? Always or only after the November, 2007 patches? Salvis and I both understand what you are saying. We both are for giving you what you ask for. We want the patch I've supplied applied which will meet the requirements of the patch in #22 as well as give us back what we had before the patch based on an administrative checkbox. If you are using version 5.7 and are still having an issue, then your issue is elsewhere within Drupal core coding; perhaps within the node API instead of the user API. If that is the case you need new issue. If your issue began after installing 5.7 then you may have an issue here or perhaps with a different patch that was added to 5.7.

Michelle’s picture

@salvis & earnie: That patch was comitted last November. Advanced Profile didn't come out until January. Though, I did have some complaints with this with the tutorial that advprofile replaced. At any rate, this is still a problem on 5.7. That's why I came to this issue. I happened on a post on the dev list that mentioned this business of making profiles 403 was actually by design and put in my +1 to change that. Changing the access time for admin touched users is silly and I never was advocating that so I don't know why you keep bringing that up in response to me. What I want is an option to simply not forbid access to any profile whether the user has logged in.

I don't know why you think it's a different issue? It's the same thing. It's just that the patch that you're complaining about only fixed half of it. I'm complaining about all of it. If I give users access to profiles, I want them to have access to all profiles, period.

Michelle

Anonymous’s picture

Changing the access time for admin touched users is silly and I never was advocating that so I don't know why you keep bringing that up in response to me.

Because this is what we're discussing. The issue put a patch in that did just that and we want to either apply the patch I've provided or better reverse the patch entirely and come up with a better patch. I'm glad to hear we are in agreement with regard to user.access being munged on admin operations to the user.

What I want is an option to simply not forbid access
to any profile whether the user has logged in.
I don't know why you think it's a different issue? It's the same thing.

But it is not the same thing. You want to be able to show the profile immediately after registration. That is a totally different issue than the issue of an administrator creating the user and wanting the profile to show. What you need requires a different patch and should be tracked in another issue with a different title.

If we reverse the patch at #22 we could incorporate a change that would fit both since both touch the user. We would need to add a column user.visible column to control the visibility instead of using the access column to do that. Blocked users should always be invisible as well IMO. We might be able to change the user.status column to be a bit valued comparison column for the state of the user instead of adding another column as has been mentioned in the list.

sun’s picture

@earnie: Some of your arguments are not correct. See #1 - #3, and my summary in #45. user.access is misused to decide whether a user may show up. It's hard-coded to prevent Spam on many sites with open user registration.

From my understanding, you and salvis like this Spam protection, but you do not like the misuse of user.access (introduced with patch in #17) for this purpose.

Michelle, OTOH, does not want this Spam protection at all. And, yes, that's the original topic of this issue: users are unable to access user profiles.

IMO, a half-baked Spam protection like this does not belong in Drupal core. What's currently there could easily be provided by a contrib module.

catch’s picture

fwiw although I uploaded the patch that got committed I wasn't that fussed about this issue, just doing patch tidying. I think sun's right here - if anything the solution is to remove user.access from the logic of whether user profiles are displayed or not entirely, and let contrib handle the spam protection issue. If this is really such a problem on Drupal 5 and 6 then I'd support reverting #22 and changing the query to omit that column - then making sure there's a contrib module to take up the slack.

It also looks like #64861 is the more complete solution for D7.

Senpai’s picture

@Michelle in #57:

If you all have ruled out the possibility of fixing the original issue and are just worried about the setting of the access time, perhaps you should change the title?

I'm saying that changing the accessed timestamp in order to allow or deny anonymous users to see profiles is flat out wrong. Drupal shouldn't be thinking that way. It's flawed logic. (BTW, so is my post in #51, so disregard that. It'd never work)

In other words, I'm on your side, but I'm trying to see the real problem here. The issue isn't so much that admins are changing the user's 'last accessed' timestamp, or that link spammers tried to sign up thousands of accounts just so anonymouse spider-bots would help out their link-rank.

No, what I'm saying about this entire thread is that Drupal now seems to be ignoring it's own permissions system in special cases. I feel this is bad behavior, and is causing hacks like that other patch, and now this one, to try and work around the fact that even though 'allow anonymous users to have access to registered user's profiles' is unchecked, anonymous users can still see registered user's profiles under special conditions, and depending on the (possibly innocent) actions of a site admin.

Let's take a step back and rethink the process from the start.

A. Anonymous users need to see user profiles, but only those profiles that an admin wants them to see. Anything other that allows link spammers to flourish.

B. Newly registered users don't want their name showing up in the New Users list until they've actually logged into the site, even if that's 1, 2, or six months later.

C. Site admins need to be able to delete all user accounts that have never logged in without worrying about killing a 'ghost' account that they really needed.

D. Accounts created by an admin on behalf of a prospective user, for whatever reason or purpose, cannot be seen as "Last Accessed on 2008.12.25". To do so is an outright lie.

Ok? To paraphrase Austin Power's Fat Bastart, "Alrighty then. First things first!". We simply must respect the hook_perm. The hook_perm is all. Right now I see a setting for "access user profiles", but I don't see anything for "access profiles created by admins (ghost profiles)". Hmm, why not? Wouldn't this alleviate all of the problems from this thread AND the other one?

sun’s picture

Actually, it's not only #22 that needs to be reverted, but also #64893: fix to deter from registration SPAM - the other patch that introduced the $account->access == 0 && !user_access('administer users') check in 2006.

Senpai’s picture

Status: Needs review » Needs work

I totally agree with Catch in #65. META-roles, user registration handling and SPAM registration is the best way to solve this. We really need to get the commit by goba in #24 reversed, and pursue the new meta roles with vigor.

sun’s picture

Additionally, #84490: Exclude users never logged in from the search results introduced the same access check for user search results and user lists.

Anonymous’s picture

@sun, @catch, @senpai: I agree with you for D7. I think reversing the patch for D5 and D6 needs to happen right away; or at least apply the patch I've supplied. If the patch is reversed a module can be created to allow the admin to clobber the access column on user_save.

The issue is bigger than #22 which came as a result of a suggested work around. I agree that controlling the visibility with user.access isn't correct. A visibility status needs set either as a bit combination of user.status or as a new column user.visible.

I'm not as much concerned with the SPAM control as I am with the user control of those who have never logged in. The request for selecting a user that has never logged in from the advuser module is many. This is the reason for my issue with the patch.

  1. Admin sets user can register but admin must approve.
  2. Admin approves and sets blocked to active (with the patch this action destroys the access data).
  3. Admin wants to select never logged in created 14 days ago (but with the patch can't because "never logged in" is lost).
  4. With the selected users Admin will email or delete selected users (but with the patch can't because "never logged in" is lost).
Michelle’s picture

@earnie - No, that's what you were discussing. I was never discussing it. I was discussing the title of this issue and the original problem. It wasn't the forked discussion I had a quibble with but the root issue.

@sun & senpai - Thank you. :) I feel like I'm finally getting understood. A permission sounds perfect. A simple "access *** profiles" (I can't think of a short way of saying "users who have never logged in") would suit me just fine.

Michelle

Anonymous’s picture

Status: Needs work » Needs review
FileSize
1.51 KB
1.48 KB
1.46 KB

Reversal patch for HEAD, D6 and D5

beginner’s picture

I was opposed to those patches. I supported the much better approach offered by pwolanin. See patch here:
http://drupal.org/node/84490#comment-620999
but Gabor vetoed it, and I don't have enough standing in the community to counter his argument.
I knew it was a bad decision.

But those patches were committed to fix a certain problem. Rerolling is not enough. Maybe now, with enough community support, pwolanin's patch can be considered instead.

Anonymous’s picture

@beginner: The patch you reference addresses a different issue though related. The patches at #22 and #25 in this issue need to be reversed. This workaround can be addressed via a simple module using a hook during the user_save. For me to write a module after the patch in #22 and #25 requires me to replace the user module; just messy and no one would want to maintain such a beast. I.E. A module can easily determine if the user.access is empty; a module can't do the reverse and reset it to empty.

Anonymous’s picture

And to add I do think the issue needs a bigger patch but first we need to reset the state of previous operation for D5 and D6. Then we can discuss the larger picture in a new issue.

Senpai’s picture

Status: Needs review » Closed (won't fix)

It's going to be very hard to get anyone to roll back two head patches that have made their way back to 6 and 5 without offering up an alternative patch that requires the #22 and #25 patches to be reverted in order to work correctly.

We're not in the business of going backwards. The phrase says, "The Drop is always moving", not "The Drop is always rolling, and sometimes downhill" :-)

We need a patch that addresses the real problem and fixes it satisfactorily before we'll be able to get a maintainer to un-commit the bad changes we've observed. How do we want to proceed with a good patch? In comment #68, I mention a Issue#64861, META-roles, user registration handling and SPAM registration issue that sounds very promising. There's also Issue #84490, Exclude users never logged in from the search results. I suspect that the problem in the latter issue would be succinctly handled by the meta roles, and well as the Issue#182023, Lack of a default administrator role within core.

I believe META Roles is the way to handle the lack of an administrator role in core, the spambot plague, the anonymous users who can't see their own profiles, and the admins who create or edit a user's profile only to discover that they've unknowingly created a last-logged-in time for that user even though the user has never site their site before!

Who's with me? Meta Roles, anyone? It does seem rather absurd that we've existed for years already with only a boolean set of Roles that govern our users. Kind of reminds me of a field trip you didn't wanna go on. "Are you in or are you out?" said the teacher. "You can't have it both ways!".

Meta Roles. Let's do it. --> http://drupal.org/node/64861

Senpai’s picture

Status: Closed (won't fix) » Postponed

Woops, I was off by one. Sorry gang, didn't mean to Won't Fix such a hot debate.

And Earnie, once we get a workable solution to the big problem, let's not forget that we need to revisit this issue so your patches in #72 can be applied.

Anonymous’s picture

Status: Postponed » Needs review
FileSize
3.67 KB

Then let's at least have a choice as to whether or not to clobber the access column. It isn't nice to do that when I don't choose to have the access clobbered when I maintain the user.

Anonymous’s picture

Version: 7.x-dev » 6.x-dev
FileSize
3.68 KB

Ok, for 6.x and 5.x anyway. Meta Roles can happen for 7.x.

sun’s picture

Status: Needs review » Needs work

Sorry, but:

a huge -1 for making the present code, functionality, and usability even uglier.

As outlined in #64, #67, and #69, the whole access check needs to be removed from core and turned into a contrib module (sounds like a good add-on for Spam module). This affects D5, D6, and D7.

Afterwards, we should focus on #64861: META-roles, user registration handling and SPAM registration for D7.

Senpai’s picture

Yeah, good call. D5 and D6 need something, even though I don't feel that adding a checkbox to that form is a good thing in terms of usability, it definitely should not be applied to D7, since there are better ideas in the works for D7.

Earnie, the patch in #79 does not apply to D6.2 core, even with fuzz. Can you re-roll?

sun’s picture

Title: anonymous users can not view admin-created user profiles » Regression: anonymous users cant not view user profiles when given access
Anonymous’s picture

Status: Needs work » Needs review

@sun, @senpai: That is fine. But until then let's do either #72 or #79. If we do #72 a contrib module can handle the original patch. If we do #79 then we have a choice. To do what you suggest will take a longer amount of testing time. I'm more concerned to applying either #72 or #79 to versions D5 and D6. I will be more than happy to support #64861: META-roles, user registration handling and SPAM registration for D7. But right now this change has destroyed the workflow for many administrators.

Anonymous’s picture

@Senpai #81: The patch works for me against the DRUPAL-6 branch. Did you update your source?

Senpai’s picture

Earnie, yeah, I have a fresh checkout of D6.2, and a brand new db. Eclipse says the patch can't be applied to the root of core.

Anonymous’s picture

And for me a brand new checkout of DRUPAL-6 module/user gives no exception. I cd to modules/user and patch < user_access.patch.

beginner’s picture

I bet it's a matter of line endings.
As per guidelines, patches are supposed to have unix-like line endings.

Anonymous’s picture

FileSize
3.76 KB

I bet you're wrong. The attached patch will apply from the /path/to/drupal/core/source. The previous patch applied if you cd module/user.

Senpai’s picture

Oh, I wasn't changing directories because I've been told that all core patches were made from the root dir, and thus should be applied from the root dir. I'll test out the revised patch in #88 next.

Anonymous’s picture

Oh, I wasn't changing directories because I've been told that all core patches were made from the root dir, and thus should be applied from the root dir.

The rule makes since. I'm just used to working the contributions modules where I'm in modules/foo already and since modules/users looks like any other module; well, it didn't occur to me until I read and reread "from the root directory".

gaele’s picture

I just got bitten by this issue. Organic Groups allows a "join this group" field on the user registration page. Joined users will show up on the group's members page. Until the user logs in the group manager will see the (clickable) user's name but will not be able to reach his/her contact page.

The current situation has at least three usability issues:

  1. for developers - the current behavior is not documented. It should be documented in user.module and probably elsewhere.
  2. for administrators - ghost users will show up on the users list but are not distinguishable from normal users. They should be marked as such.
  3. for users - after registration users have no idea they should login to activate their account. This should be mentioned in a message shown after registering, and in the welcome e-mail.

I fully agree with Michelle and with Sun in #80. We should get rid of this mess introduced with the original patch. However, I'm not sure what to do as it is currently unclear to me where this issue is heading.

gaele’s picture

Title: Regression: anonymous users cant not view user profiles when given access » Regression: users without administer users permission can not access user profiles of users that never logged in
jtrant’s picture

it would seem to me that for user-created accounts the last time a user was logged in was the time that they requested their account be created [on a system where accounts require approval].

i've used sql to do that, to prevent the 'not found' errors from a site user list, but can't really tell from this thread if there is a fix or not.

thanks.

/jt

salvis’s picture

@jtrant: I disagree.

If the admin reserves the right to accept or deny registrations, then an applicant's (legitimate or usurped) name must not appear on the site until the admin has given his approval.

Even with sites that don't require approval, I wouldn't want (possibly offensive) user names to show up until the users have at least confirmed their email address and their intention to join the site by logging in.

No, there is no patch that does what you're asking for. If names of ghost users appear on your site and you get 'not found' errors, then I'd fix the site user list to not show ghost users, no the other way around.

gforce301’s picture

I just read this entire thread and I am still left with the question: Are there any plans to actually fix this? I find it rather unnerving that the core maintainers would allow a patch to remain active for this long that breaks other core functionality. It was bad enough that it was committed in the first place.

It appears a patch was rolled, examined and committed without any thought to functionality of core behavior that has existed for a long time. No documentation was provided in the interface explaining why all of a sudden administrator created users "appear" to have accessed the site when in fact they have "never" accessed the site. The core code still supports displaying "never" in the last access column on the user administration page. This can be seen by going and changing the access column's value for any user to 0 in the user table.

Either a new patch needs to be rolled and committed removing the code to display "never" as this now "legacy" behavior and code is unnecessary because a user will always have a last access value. No reason to keep old useless code in core now is there? Documentation should be provided in the interface explaining the behavior that is now standard.

--OR--

The current patch needs to be decommissioned thereby bringing the core functionality back to what is expected of the last access data. A new patch/module should then be developed to fix the original issue and/or existing contributed modules should patch their code to avoid the original issue so that they work with the core behavior instead of going around it.

Anonymous’s picture

@gforce301: Until some one picks one of the patches (see #88), tests it and marks this as RTBC the issue will languish forever.

gforce301’s picture

@earnie: How ridiculous is it to have to patch to get previous long standing behavior? I wonder what ever happened to the advice I see given to people all the time, "Don't hack core". Is not patching essentially the same thing if the patch will never be committed?

I understand that I am late to this discussion. I have one other question though. Since when has it ever been acceptable programming practice to solve a logic issue by writing incorrect data to a database table?
As pointed out in #8

<?php
  $account = user_load(array('uid' => $uid));
  if ($account === FALSE || ($account->access == 0 && !user_access('administer users'))) {
    return drupal_not_found();
  }

?>

This is the IF statement that causes the original problem that this thread was started over. Manufacturing fictitious data on the fly because nobody wanted to take the time to rewrite that small piece of code borders on the incompetent.

There has been an 6 month discussion since #29 when salvis wisely reopened this issue. In that 6 months it has virtually been proven beyond a doubt that writing fictitious data was a mistake to solve a logic problem. What is the problem with rolling back a patch that should never have been committed? Is it a pride issue now?

StevenPatz’s picture

gforce301’s picture

@spatz4000: Where did I mention "Who's New". The previous behavior of writing a 0 to the access column for a user who has never logged in was not a bug. It has been by design since day 1 AFAIK. If you can provide documentation that the original design of the users table called for a date to be written to the access field at creation time then I will step down. However I doubt very much that you will be able to find that documentation. The initial 0 has always been there, allowing an administrator to tell when a user had truly never logged in see http://drupal.org/node/171117#comment-896394 2nd paragraph.

Please feel free to post me another link pointing to any design notes, specifications or any other documentation that provides information that the access column in the users table should contain data (by design) other than a 0 for users who have never logged in, or a timestamp of the last time the user logged in accessed the site. Barring that, the post at #30 has no bearing on the issue of arbitrarily writing a timestamp to the access column upon user creation, which is not the design of that particular table.

I'll ask you the same question that I asked in #97. Since when has it ever been acceptable programming practice to solve a logic issue by writing incorrect data to a database table?

Anonymous’s picture

@earnie: How ridiculous is it to have to patch to get previous long standing behavior? I wonder what ever happened to the advice I see given to people all the time, "Don't hack core". Is not patching essentially the same thing if the patch will never be committed?

Uh, hum, check out #72. That patch would need rerolled to create the difference from the drupal root directory though.

gforce301’s picture

@earnie: I was not poking at your patch man. I was merely making a statement about how I have to patch core files in order to revert a patch that should not have been committed. This means that when drupal 5.7.x, 5.8, 6.2.x or 6.3 rolls out, if they don't reverse that abortion of a patch they committed, then I will have to re-patch again. At that time depending on the changes in the new version, the patch I used may no longer be valid and a vicious cycle ensues.

What I was also pointing out is that, that is no better than just plain hacking the core. It amounts to the same thing. The committed patches amount to the same thing. They are at best pretty bad hacks as they follow no good programming practice(s) that I am aware of or have been pointed to.

Some maintainer(s) with the authority to commit a patch made a serious error in committing these particular ones #24 and #27. No big deal really, as long as they can admit it was an error and revert it. However, as long as they continue to stonewall the community and people who have pointed out that the patch(es) in question seriously violate the original design of the user table, then I start to question the actual use of open source practices by the maintainers.

catch’s picture

gforce301: the status of this issue is 'patch (code needs review) - that means it needs reviewing - applying, testing. Perhaps you'd like to do that, then if you decide that it fixes the problem adequately, you could change the status to RTBC, after which it has chance of being committed.

Otherwise it'll stay at needs review, and you can continue to complain that people aren't looking at it. There are no people, only you.

fyi there are only two people who can commit patches to Drupal 6 - they rely on other people testing them in order to do so efficiently.

gforce301’s picture

@catch: The status of this issue may be 'patch (code needs review)' but that does not change anything that I have been talking about. Yes talking with several different people. Not just complaining as you seem to think but a running conversation. I asked some questions, got responses, posted my responses etc..... There are people looking and you are one of them. I understand now as I go back what your animosity is, you were the person who set the status of the patch in #22 to RTBC which got it committed to drupal 6 in #24 by gabor and to drupal 5 in #27 by drumm. I am sorry if you feel that my comments were somehow a personal attack on you, they were not intended to be.

Being new to posting on drupal, but by no means new to drupal. I am still confused why anyone, including yourself, would consider a patch that obviously changes the data in a core table, thereby breaking it's original intent, as RTBC. The fact that the patch solves the original issue is not the point. The fact that the committed patch solves the original issue by breaking core functionality is the point.

Mistakes happen. I am no stranger to making mistakes. I am also not the first person to point out the flaw in the committed patch, I am just the current one. Just because a patch has been reviewed to some degree, marked as RTBC and actually committed does not make it the holy grail of untouchable code now does it? The drupal project does use CVS does it not? Are you implying that it is impossible to rollback a file to before the patch was committed in the CVS system? I mean isn't that what CVS is for? So the patch is no good, it's nothing to get your shorts in a knot over. Mistakes happen.

Just so everyone understands how I arrived in this issue thread to begin with: I recently unpacked and installed a fresh 5.7 core. For many reasons that don't matter here, my place of employment has not yet updated to 5.7. I am unsure exactly which version of the 5.x branch this patch was committed to, I think it was 5.4 and we are on a previous version from that, but it has been carried forward to 5.7. Now as per my design standards one of the first things I do is make a fully privileged user and log out the root user. Imagine my surprise when the administer users view shows that my newly created user has already logged in. I asked myself how is this possible, as I only created him 3 sec ago? Why on earth would core behavior change that has been consistent since drupal 4.7.x at the very least, if not earlier? So I went looking for the answer and wound up here.

I do not see the original issue as an issue/bug. It was by design and has been pointed out by others in this very same thread, that it could have easily been solved by a small contributed module. My issue with all of this is only the undocumented, unprecedented and erroneous behavior caused by the patches committed in #22 and #24 to drupal 6.x and drupal 5.x respectively. If you are telling me that the only possible way to fix what I see as an error is to test the patches in #72, repost them and set the status of this to RTBC (which by the way will roll back the module and recreate the original issue), then that is what I will do. I however, do not think that will be accepted or acceptable by you or many others.

Senpai’s picture

I wish I could delete comment #103, and replace it with, "Hi, I'm gforce301, and I tested this patch tonight. It's RTBC!"

gforce301’s picture

@senpai: You are the same senpai that posted this

We really need to get the commit by goba in #24 reversed, and pursue the new meta roles with vigor.

here http://drupal.org/node/171117#comment-841033 are you not? If you are is there any particular reason you are attacking someone who shares your same views? I find it rather amusing that you would state something of this nature and other things about getting a better patch but I notice you have not tested any of the new ones and posted on them nor have you produced a new one. Do you need me to post the definition of hypocrite for you?

catch’s picture

gforce301, I've RTBCed a more than one bad patch, and equally rolled a few rollback patches similar to this one that were RTBCed by other people. Certainly didn't take anything as a personal attack, more a directionless rant. However I'm still not sure why you prefer to spend many, many minutes writing hundreds of words on the subject rather than the two it would take to test this patch.

BioALIEN’s picture

My issue #295839: Bug in user.module "last access" was marked as a duplicate of this. Subscribing to keep track of this.

Anonymous’s picture

Just to remind others the patch is in #88 and for D6.

salvis’s picture

Version: 6.x-dev » 7.x-dev
Status: Needs review » Active

Beginner's comment in #25 still holds and this is still broken!

salvis’s picture

It has been over two years since we've tried to fix this and some of us spent considerable effort...

After six months I got tired of this and wrote my own module to fix it. This module has gathered a few more fixes in the meantime, and I've just released it as Fix Core for D6. So, rather than patching each core release, just turn on a checkbox and never look back.

sun’s picture

Status: Active » Needs review
FileSize
2.26 KB

Enuff talk. This deeply hidden feature belongs into contrib and nowhere else. It's a constant source of WTFs and cannot reasonably explained to anyone. It breaks perfectly valid use-cases.

sun’s picture

Assigned: Unassigned » sun

Status: Needs review » Needs work

The last submitted patch, drupal.user-remove-that-half-baked-spam-protection.111.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
3.15 KB

RIPITOFF.

sun’s picture

So it finally passed. Let's finally fix this annoying bug for D7!

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

agreed

salvis’s picture

Status: Reviewed & tested by the community » Needs review

+1

salvis’s picture

Status: Needs review » Reviewed & tested by the community

(sorry)

PieterDC’s picture

+1

sun’s picture

sun’s picture

sun’s picture

Status: Reviewed & tested by the community » Closed (won't fix)
Dave Reid’s picture

Assigned: sun » Unassigned
Status: Closed (won't fix) » Reviewed & tested by the community

Just because someone is upset doesn't mean we have to mark something as won't fix. Someone else is more than welcome to pick up work on a ticket.

sun’s picture

Version: 7.x-dev » 8.x-dev

Although badly needed, this sounds like D8 material to me.

sun’s picture

Version: 8.x-dev » 7.x-dev
Assigned: Unassigned » sun

So it seems that bugs are still considered to be fixed for D7.

mstef’s picture

subscribing (looking for d6 fix)

sun’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, drupal.user-remove-that-half-baked-spam-protection.114.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
sun’s picture

Status: Needs review » Reviewed & tested by the community
sun’s picture

sun’s picture

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

deanflory’s picture

Issue summary: View changes

If fixed, update the descriptions on these items at this page (or if those options are no longer necessary remove them):

/admin/config/people/advuser

  • Reset the accessed to never on administrative update
    Because of a bug caused by this issue [external link], watch for administrative updates that change the accessed column from 0 (never) and reset it.
  • Set accessed to never on administrative create
    Because of a bug caused by this issue [external link], watch for administrative creates and set the accessed column to 0 (never).