Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tobiberlin’s picture

I would appreciate such a possibility as well

cYu’s picture

Status: Active » Needs review
FileSize
2.19 KB

Attached is a patch which will theme user name before output in the various autocompletes provided by masquerade module.

cYu’s picture

Status: Needs review » Needs work

Actually, this themes the username as a clickable link...which is undesirable. With realname overriding theme_username you'd be able to do

theme('username', $user, array('plain' => TRUE));

but without making realname a dependency you'd need to striptags or check_plain or something of that sort to get rid of the name being a link to the user profile page.

deekayen’s picture

how about wrapping theme() in the same check_plain that you replaced?

cYu’s picture

Status: Needs work » Needs review
FileSize
2.25 KB

check_plain would encode the markup but still look bad in the autocomplete field. filter_xss looks like what I want (with an empty array for allowed tags) for stripping out the markup. Was hoping there was a nicer way in D6, but it looks like that is the standard protocol. If this goes in I'll also roll a D7 patch which should be cleaner. http://drupal.org/update/modules/6/7#format_username

deekayen’s picture

what about just straight strip_tags()?

cYu’s picture

Yeah, that will be more efficient and give the same output. Attached patch with strip_tags.

j4’s picture

Hi,

Can you provide a similar patch for version 7 also?

Thanks
Jaya

deekayen’s picture

Status: Needs review » Patch (to be ported)

#8 is a good point. It'd be good not to commit to only 6.x and leave it out of 7.

andypost’s picture

Version: 6.x-1.6 » 6.x-1.x-dev
Status: Patch (to be ported) » Needs review
FileSize
1.58 KB

@deekayen suppose we could not commit 6.x and change this for 7.x for format_username() because this require to change a logic for _masquerade_user_load($username) but I have no idea how to implement a search having a some piece of render

liquidcms’s picture

Version: 6.x-1.x-dev » 7.x-1.x-dev
Category: support » feature
Status: Needs review » Needs work

no progress on this? i'll take a look at it tonight.

and bumping to D7

liquidcms’s picture

hmm.. i'm at a loss; do either the d6 or d7 patches here work at all?

i see the query in the patch is changed simply to this:

$result = db_query_range("SELECT u.uid, u.name FROM {users} u WHERE LOWER(u.name) LIKE LOWER('%s%%')", $string, 0, 10);

so still searching in user table on username for the string that gets entered. that will never work. possibly what this patch is doing is simply returning the realname to show in the field prior to the user hitting GO; but it isn't searching on realname.. which i think is kind of the real issue here isn't it?

liquidcms’s picture

doesnt seem that elegant but not sure there is a much better way.

so this patch checks if realname module exist and if it does; it does the "right" thing; which is search on realname and display realname in the autocomplete results.

i actually display the result as [realname] ([username]) since realname is not unique and this would possibly allow someone to pick the correct result; plus it makes it easier at the end to set to the correct user.

liquidcms’s picture

Status: Needs work » Needs review

patch is not in GiT format so auto test will fail; sorry, GiT sucks

liquidcms’s picture

i also fixed search method in both the realname method and pre-existing username method of doing search. for some reason the original code uses "starts with" rather than "contains"; which seems silly.

andypost’s picture

+++ masquerade.module	(working copy)
@@ -691,11 +696,29 @@
+  if (module_exists('realname')) {
+    $result = db_query_range("

Not sure that escaped realname could be found this way

+++ masquerade.module	(working copy)
@@ -691,11 +696,29 @@
+      SELECT u.name, rn.realname ¶
...
+      LEFT JOIN {realname} rn ON rn.uid = u.uid ¶
...
+  ¶

trailing whitespaces

+++ masquerade.module	(working copy)
@@ -691,11 +696,29 @@
+      $name = $user->realname . ' (' . $user->name . ')';
...
+      $matches[$user->name] = check_plain($user->name);

format_username()

+++ masquerade.module	(working copy)
@@ -691,11 +696,29 @@
+      $matches[$name] = $name;

unescaped?

liquidcms’s picture

sorry, not sure what your comment is referring to.

yes, my code does work; i am using it on a site.

perhaps you are saying i need a check_plain() as some sort of security measure? but i don't see how that is necessary.

deggertsen’s picture

Thanks for the patch. It works perfectly. I've rerolled the patch using git and removed the whitespace errors.

deekayen’s picture

I'm confused. The reason I use realname at work is for security. It helps keep the login username masked so it's harder to know how to login as another user since Realname isn't valid for logins. If you expose $user->name in the autocomplete, aren't you circumventing that?

liquidcms’s picture

it's the autocomplete for the realname switch form. why would your untrusted users have access to switching users? would seem if they had that; they would already be bypassing logging in.

deggertsen’s picture

@deekayen. Are you using masquerade for something other than admin development? If so then what you say sort of makes sense, but usually you would not want people to be able to masquerade as someone else using their account unless they are an administrator anyways right? I think in most cases, this patch serves in a way people would want it to work.

deekayen’s picture

We have non-admin users (customer service reps) who masquerade as a user for helping users place orders by phone or for doing level one helpdesk.

liquidcms’s picture

@deekayen

1. if you can become that person; surely you can edit their profile and see their username, correct? or is the issue that your reps are limited to who can masquerade as; but with this patch you can see everyone's username? (is that a setting/perms with this module - to limit who you can masquerade as?)

2. knowing someone's username is a long ways from knowing their password and being able to log in.

andypost’s picture

Title: Realname in masquerade switch block instead of username » Use [uid] for masquerade autocomplete to provide Realname and other modules integrations
Status: Needs review » Needs work
Issue tags: +Needs tests

SO let's re-title and solve the issue forever.

@liquidcms The real names could not be unique so you add a $user->name here but introduces security issue.
I'd suggest to output [uid] as all autocompletes does for nodes, it's a common practice.

BTW it introduces nice API change which I'd like to support - always use [uid] for autocomplete so it would allow us to use format_username() for autocomplete or maybe theme(username) and finally solve the issue with all modules that alters user name.

PS: Most of sites I'm working on use email_registration module so names of users are auto-generated and not readable

+++ b/masquerade.module
@@ -691,11 +696,29 @@ function masquerade_autocomplete($string) {
-    $matches[$user->name] = check_plain($user->name);
...
+      $name = $user->realname . ' (' . $user->name . ')';
...
+      $matches[$user->name] = check_plain($user->name);

the scariest thing here the we output names unescaped

deekayen’s picture

iirc, theme_username returns the realname, so why not just skip the query change, and do check_plain(theme_username(array('account' => $user)))?

andypost’s picture

@deekayen yes, you get it right - once we will use [uid] from autocomplete then we can prepend it with any output that produced by format_username() that need sanitization with check_plain()

The last submitted patch, 2: masquerade-realnameautocomplete-1295818-2.patch, failed testing.

The last submitted patch, 5: masquerade-realnameautocomplete-1295818-5.patch, failed testing.

The last submitted patch, 7: masquerade-realnameautocomplete-1295818-7.patch, failed testing.

The last submitted patch, 13: masquerade-use_realname.patch, failed testing.

The last submitted patch, 18: masquerade_use_realname-1295818-18.patch, failed testing.

deggertsen’s picture

Updated patch to apply against most recent dev. I would like to implement what is being discussed in #24-26, but I don't totally follow. It would be great if one of you could update this patch with those changes.

Thanks

deggertsen’s picture

Hiding patch from #2

scottsawyer’s picture

I am testing the patch in #32 ( masquerade_use_realname-1295818-32.patch ) and it is pulling the Real Name in the autocomplete, it is passing masquerade_block_1_validate, but it never seems to hit masquerade_block_1_submit.

Using 7.x-1.0-rc7.

I have put some debug code into _validate to make sure it was setting $form_state['values']['masquerade_user_field'] to the actual user name and it does. But it won't submit. The page reloads and it doesn't start the masquerade session.

The links work. And I put debug code in the top of masquerade_block_1_submit and it doesn't even react.

I have an older clone on 7.x-1.0-rc5 with masquerade_use_realname-1295818-18.patch and it is working perfectly.

Any thoughts?

PascalAnimateur’s picture

Here's a simple patch to integrate realname superficially (autocomplete works, changed a couple of messages to use format_username(), but not all of them).

izmeez’s picture

The patch in #35 passes test, is it suitable to add to #3010095: Masquerade 7.x-1.0 stable release plan

izmeez’s picture

The patch in #35 applies to 7.x-1.x-dev but has difficulty applying when in conjunction with the patches in #1171500: Add "masquerade as @role" permissions/settings for each role and #2201055: Do not show inactive / blocked users in autocomplete and block.

A re-roll somewhere along the way may be needed.

The patch still needs to be RTBC.