In default Drupal, we show a checkbox for removing the authenticated user role but we don't let you uncheck the box because then account would not have any roles. thats a useless checkbox! this patch fixes this.
In addition, this patch adjusts the login and access system so that logged in users *always* get the authenticated user role. This is analogous to how we handle anonymous users (they always receive the anon user role). Only harm can happen by not making this assumption. There are situations on table sharing sites where users are created but they are not in any role. This patch makes that problem disappear.
With this patch, the authenticated or anonymous user role is attached to $user->roles during sess_read() or user_load(). This completeness in $user->roles enabled me to simplify the query in node_access() a bit by removing JOIN. Further, some special case code is removed in various places.
I think this adds a bit of UI improvement, and more robustness.
Comment | File | Size | Author |
---|---|---|---|
#22 | role_robust2.diff | 796 bytes | Cvbge |
#21 | role_robust_2.diff | 748 bytes | Cvbge |
#12 | role_robust_1.diff | 12.67 KB | moshe weitzman |
#11 | role_robust_0.diff | 10.82 KB | moshe weitzman |
#6 | auth_role.diff | 9.84 KB | moshe weitzman |
Comments
Comment #1
chx CreditAttribution: chx commentedWith
INSERT INTO role (rid, name) VALUES (2, 'authenticated user');
in database definition, a simpledefine (DRUPAL_AUTHENTICATED_RID, 2);
would suffice instead of the db function. Otherwise, very cool patch.Comment #2
moshe weitzman CreditAttribution: moshe weitzman commentedthat define could cause troubles for existing sites with strange setups. since it has minimal benefit, i propose to go with the patch as is. anyone else care to review this? if you do, please explicitly say whether you tested it or not.
Comment #3
chx CreditAttribution: chx commentedmoshe, my problem is that you add a query to every page served to non-anon users. IIf someone has a real weirdo setup, then (s)he can correct the define.
Comment #4
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedI think I don't like this patch. First, it adds overhead, second, here on drupal.org I on occassion ban users by taking "authenticated" user away and granting the "restricted users" role. We could of course change "authenticated users" to have only a minimal set of permissions but then we'd need to grant additional rights to every new user.
One can of course question the wisdom of having a "restricted users" role and not blocking the users directly.
I think we should instead make sure that at least one checkbox of that checkbox set is checked.
Comment #5
moshe weitzman CreditAttribution: moshe weitzman commentedThanks for the reviews.
I changed to a define() per chx' suggestion. You convinced me.
@killes: i see your point. however, on balance this is a step forward for drupal. the state you describe could only make sense to a developer i think. you have a restricted user role but very few sites will have that. this starts to sound like a "who's on first" comedy routine. imagine a username=joe who is in this state:
q: is joe logged authenticated
a: yes
q: is joe a user?
a: yes
q: so joe is an authenticated user?
a: no
q: so joe is anonymous?
a: no, he's joe
q: what poermissions does joe have?
a: anonymous
Comment #6
moshe weitzman CreditAttribution: moshe weitzman commentednow with a patch
Comment #7
Gerhard Killesreiter CreditAttribution: Gerhard Killesreiter commentedMoshe: the problem is not that I have this setup, but drupal.org has it. :P
I dont know what propted the introduction of the "restricted user" role. if we find out that we don't need it, we can go with this patch.
Comment #8
Dries CreditAttribution: Dries commentedThe 'restricted user' role was introduced to block registered users from posting. We could have blocked these users as well. Still, punishing certain users without having to block them might be a sensible use case?
If we have 'DRUPAL_AUTHENTICATED_RID' we should have 'DRUPAL_ANONYMOUS_RID' as well and use it across the code. Example:
Also, please depricate:
Comment #9
Dries CreditAttribution: Dries commentedSome more observations:
In my
role
table is an anonymous user too. I guess that should be cleaned up as well?In the
users_roles
table are registered users as well:Comment #10
Stefan Nagtegaal CreditAttribution: Stefan Nagtegaal commentedAnd, maybe it's worthwhile at the same time, to fix the role names 'Authenticated user' and 'Anonymous user' which are untranslatable.. :-)
Comment #11
moshe weitzman CreditAttribution: moshe weitzman commentedI implemented all feedback except for Stefan's request to translate these 2 system roles. Thats for another patch.
Comment #12
moshe weitzman CreditAttribution: moshe weitzman commentedmore cleanup. we are no longer dependant on role names, only rids.
@stefan - once this is committed, you may find all the places where we output a role name and wrap in a t(). i see roles, permissions, input filters, user overview, user edit, ... or you could make sure all functions use user_roles() instead of custom query. then do th t() only in user_roles().
-moshe
Comment #13
moshe weitzman CreditAttribution: moshe weitzman commentedmore people are running into this problem - http://drupal.org/node/26700.
would be nice if a CVS reviewer commented here :). i am OK with postponing until 4.8 if thats desired. a bit disappointed, but OK.
Comment #14
Dries CreditAttribution: Dries commentedHaven't test it. Code looks good.
Does $key need to be escaped?
Comment #15
moshe weitzman CreditAttribution: moshe weitzman commentedyes it does. good catch. i will upload again tonight. unless someone says differently, i will change form API to do this automatically for all title elements. those should always be plain text. the function that needs the check_plain() in this case is http://drupaldocs.org/api/head/function/theme_checkbox
Comment #16
chx CreditAttribution: chx commentedJust make sure that we do not double escape some titles. I am OK with the change. But, as you are promising a new patch, I changed the status.
Comment #17
webchickQuestion.. will this patch prevent the logintoboggan module from working? That module allows users to instantly sign up and pick their own passwords without having to futz around in their e-mail, thus increasing usability.
If you use this option, users are added to a "pre-authorized" group of your choosing to give them a set of minimal permissions (better than anon but less than 'authenticated user') until such time that they've validated their e-mail address and then get bumped up to 'fully' authenticated user. But if authenticated user applies for ALL logged in users, it seems we would lose this option for functionality.
Comment #18
Crell CreditAttribution: Crell commented@webchick: I think the better solution would be to do it the other way around. Make authenticated user a minimalist set of permissions, and something like "validated user" the "real" role. Then use hook_user() to automatically put a user into "validated user" on email validation.
Similar solution for Drupal.org and the "restricted but not banned" issue that killes mentioned. It might even be good to generalize "put into role on user creation" functionality into a module, if someone hasn't done it already.
This gets a conceptual +1 from me, although I've not looked at the code yet.
Comment #19
moshe weitzman CreditAttribution: moshe weitzman commentedUpon further review, that escaping is not needed. The content are permission names, and those are module defined, not user defined. And we inherently trust our active modules, because they can execute arbitrary php, delete data, etc.
ready to commit
Comment #20
Dries CreditAttribution: Dries commentedCommitted to HEAD. Thanks.
Comment #21
Cvbge CreditAttribution: Cvbge commentedOriginal patch did not remove INSERTs into user_roles in database.pgsql
Comment #22
Cvbge CreditAttribution: Cvbge commentedRerolled from the top.
Comment #23
Dries CreditAttribution: Dries commentedCommitted to HEAD. Thanks.
Comment #24
markus_petrux CreditAttribution: markus_petrux commentedSorry, shouldn't these statements be removed as well?
Comment #25
moshe weitzman CreditAttribution: moshe weitzman commentedthose two INSERTS are still necessary ... a future patch work on creating get/set functions for roles and assuring that all functions use them. then eliminating those records is easy.
@Cvbge - good catch on the postgres schema. sorry i missed that.
Comment #26
dwwi discovered this thread because a small patch i submitted for user.module (see http://drupal.org/node/47735 for details) applies cleanly in 4.6 but fails to patch against the HEAD . one "side effect" of this whole discussion about 'authenticated user' is that the db_query() inside user_access() was "simplif[ied]... a bit by removing JOIN". well, that broke my nice clean patch from working in HEAD. ;) cvs annotate on the modified db_query() line in HEAD lead me here... (side note: drupal and drupal.org is great... i love the fact that the cvs log messages in your repository include node ids about the thread that lead to the patch -- if only we were so well organized and coordinated where i work).
i can see that removing an unnecessary JOIN in the default case is a good thing, but i'm wondering if any of you user.module experts would comment on my other thread.
thanks much!
-derek
Comment #27
(not verified) CreditAttribution: commentedComment #28
marcoBauli CreditAttribution: marcoBauli commentedumm..still experiencing this on 5.12, new users don't have the 'authenticated user' role attached by default.
Am i the only?
Comment #29
aamin CreditAttribution: aamin commentedI am also facing the same problem, can someone please tell me how can I use this patch. that is where should I copy it.
Thanks.
Comment #30
infohari CreditAttribution: infohari commentedhi all,
i am totally new to the drupal environment, how to set user permission on the user registration. can any one address me please
with regards,
hari