Maybe I'm missing something, but there is no way for me to set the password for a new account created with fboauth. The register/login process is fine, but you need the current password in order to change/set it from you profile.

CommentFileSizeAuthor
#35 fboauth-no-way-to-set-your-password-1398782-35.patch1.78 KBmike.davis
#34 fboauth-no-way-to-set-your-password-1398782-34.patch1.74 KBmike.davis
#30 fboauth_password-d7.patch1003 bytesquicksketch
#21 fboauth-no-way-to-set-your-password-1398782.patch1.15 KBmeSte
#20 fboauth-current-password-1398782-20.patch571 bytesfotuzlab
#16 d7.patch571 bytesfotuzlab
#9 d7.patch341 bytesfotuzlab
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Stefan Vaduva’s picture

Stefan Vaduva CreditAttribution: Stefan Vaduva commented
Component: User interface » Code

I've updated the "component"

quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch commented

The register/login process is fine, but you need the current password in order to change/set it from you profile.

Hm, good point. This wasn't the case in D6 and I didn't think about it when I ported the module to D7. Of course you don't really need a password if you're logging in via Facebook every time, but we should build-in some kind of skipping functionality so you don't need a password if you haven't set one yet. Because FBOAuth saves an empty string in the database, it's impossible to enter a "correct" password since anything entered (even an empty string) will be hashed first before checked against the database.

In the mean time, if a user is unable to login, they can use the Reset Password tab to set a "new" password.

Stefan Vaduva’s picture

Stefan Vaduva CreditAttribution: Stefan Vaduva commented

Looking for a solution to this I've found something in the user.module. The user_account_form() function decides if the current password field should be displayed or not (http://api.drupal.org/api/drupal/modules--user--user.module/function/use...):

...
// To skip the current password field, the user must have logged in via a
    // one-time link and have the token in the URL.
    $pass_reset = isset($_SESSION['pass_reset_' . $account->uid]) && isset($_GET['pass-reset-token']) && ($_GET['pass-reset-token'] == $_SESSION['pass_reset_' . $account->uid]);
    $protected_values = array();
    $current_pass_description = '';
    // The user may only change their own password without their current
    // password if they logged in via a one-time login link.
    if (!$pass_reset) {
      $protected_values['mail'] = $form['account']['mail']['#title'];
      $protected_values['pass'] = t('Password');
      $request_new = l(t('Request new password'), 'user/password', array('attributes' => array('title' => t('Request new password via e-mail.'))));
      $current_pass_description = t('Enter your current password to change the %mail or %pass. !request_new.', array('%mail' => $protected_values['mail'], '%pass' => $protected_values['pass'], '!request_new' => $request_new));
    }
...

So, I guess it will be easy to generate the password reset token with user_pass_rehash($account->pass, $timestamp, $account->login) (this is the same way the password reset hash is generated) and then set the session variables. I'll give it a try and let you know if it works.

Stefan Vaduva’s picture

Stefan Vaduva CreditAttribution: Stefan Vaduva commented

Hmm, an easier solution is to redirect the newly created account to the password reset page with:

drupal_goto(user_pass_reset_url($account));

This is what I've ended up using (it's just a quick hack to make it work with my particular settings - no account approval needed). In fboauth.fboauth.inc:

if ($account->status == 0) {
        _user_mail_notify('register_pending_approval', $account);
        drupal_set_message(t('An account has been created for you on @sitename but an administrator needs to approve your account. In the meantime, a welcome message with further instructions has been sent to your e-mail address.', array('@sitename' => variable_get('site_name', ''))));
      }
      // Log in the user if no approval is required.
      // elseif (fboauth_login_user($account)) {  <-- no need to login the user anymore because the password reset function will take care of this
      else {
        // drupal_set_message(t('Welcome to @sitename. Basic information has been imported from Facebook into your account. You may want to <a href="!edit">edit your account</a> to confirm the details and set a password.', array('@sitename' => variable_get('site_name', ''), '!edit' => url('user/' . $account->uid . '/edit'))));
        drupal_set_message(t('Welcome to @sitename. Basic information has been imported from Facebook into your account. After clicking "Log in" you will be able to set a password.', array('@sitename' => variable_get('site_name', ''), '!edit' => url('user/' . $account->uid . '/edit'))));

        drupal_goto(user_pass_reset_url($account)); // <-- redirect to password reset form
      }
quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch commented

Hm, setting a password would be a good thing, but I don't think this would be something all sites would want. There's really no need to set a password at all, users that use Facebook to log in have a tendency of using it all the time, rather than using a password.

The code you dug up looks like it could be a work-around, though attempting to trigger the no-password behavior by setting SESSION variables seems unreliable.

fotuzlab’s picture

fotuzlab CreditAttribution: fotuzlab commented

subscribe.

matrlx’s picture

matrlx CreditAttribution: matrlx commented

I have the same issue here..
As quicksketch said, I don`t think people would like to get a generated password. The best solution is to skip the "current password" field at the first time new users edit their profile.

It would be nice to get this working as soon as possible because if someone Deauthorize the Facebook login feature and did not setup new password, they will loose account access.

quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch commented
Priority: Major » Normal

It would be nice to get this working as soon as possible because if someone Deauthorize the Facebook login feature and did not setup new password, they will loose account access.

If you have an existing account on the Drupal site and you (re-)authenticate with Facebook, if the e-mail addresses line up between Drupal and Facebook, your account is automatically linked (rather than making a new account), because e-mail addresses have to be unique anyway. So they're still not actually locked out of their account. And as I said above, the user can use the password reset option to gain access to their account too.

fotuzlab’s picture

fotuzlab CreditAttribution: fotuzlab commented
FileSize
d7.patch341 bytes

Here is a patch for fboauth module.
If a user has authenticated his facebook account, he won't be presented with "current password" field, though he can set his password and other details.
When he de-authenticates his facebook account, a warning to reset password is already displayed.

quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch commented

Thanks @fotuzlab, I'll give this a shot. Though it looks like we should only hide the password field if they haven't specifically set one? Removing this field also has security repercussions in that it makes the end-user more vulnerable to XSS attacks to hijack their account. Though D6 didn't have similar protections... so we're no worse off than any D6 site out there.

fotuzlab’s picture

fotuzlab CreditAttribution: fotuzlab commented

I agree with you on that. It is definitely like back porting to D6.

The other option I found feasible was to ask the user to set a password at time of or after authentication. But users are not used to such kind of workflow. I mean most of the websites use facebook connect and users are habitual of instant registration.
Not sure if auto generating and feeding a password would be a good option on every profile edit attempt.

May be we can add a warning to set a password asap to secure user's details. Once the password is set, "current password" field would show again for future edits.
Thoughts?

quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch commented

May be we can add a warning to set a password asap to secure user's details. Once the password is set, "current password" field would show again for future edits.

I think that would make sense. FBOAuth already asks users to set a password when they first log in, though ironically you can't actually do that in D7 version. FBOAuth sets the password column in the database to an empty string so it's already easy enough to know that the user hasn't set a password at all. While an empty column may sound dangerous (as if you might be able to log in with a blank password), this isn't the case because all passwords (even blank) are hashed before they're checked against the database. A blank value therefor will never match anything.

So in short I think we could check if the password is blank, and if so, show another message on the user profile page requesting that the user set a password. We can hide the existing "Current password" field using your code in #9.

fotuzlab’s picture

fotuzlab CreditAttribution: fotuzlab commented

"show another message on the user profile page"

Did you mean persistent/status message at the time of login?

matrlx’s picture

matrlx CreditAttribution: matrlx commented

I have another idea that might not have a lot of security problems. What do you think if we do this, when the new user connects to facebook our app get some information from him (email, name, bio, etc), right? We can define a default "current password" with birthday date, for example. So, the user will connect using facebook and to change the password, the current one will be his birthday date (information we got from facebook).
We continue showing the alerts (as said before) asking the user to change their current password.

quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch commented

We can define a default "current password" with birthday date, for example. So, the user will connect using facebook and to change the password, the current one will be his birthday date (information we got from facebook).

That sounds even more likely to cause problems. When the user doesn't have a password at all, it's impossible to guess. Imagine a site that had thousands of users and if their passwords were all set to their birthdays. It'd be much more likely that a hacker would figure that out a lot faster than the end-users would.

Did you mean persistent/status message at the time of login?

I meant just put a message at the top of the user/x/edit page.

fotuzlab’s picture

fotuzlab CreditAttribution: fotuzlab commented
FileSize
d7.patch571 bytes

Was busy travelling...
Here is the revised patch against 7.x-1.4

meSte’s picture

meSte
Primary language Italian
Location Rimini
CreditAttribution: meSte commented

Patch #16 works fine on version 1.4

fotuzlab’s picture

fotuzlab CreditAttribution: fotuzlab commented
Version: 7.x-1.3 » 7.x-1.4
Priority: Normal » Critical
quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch commented
Priority: Critical » Normal
Status: Active » Needs work

Putting these patches together into a single file (and using the right patch format) would make reviewing a bit easier. This isn't a critical bug (or even major bug), as I said in #5. Users aren't locked out, they *can* actually set a password by using the password reset, and most users won't notice because they'll use the Facebook login button anyway.

fotuzlab’s picture

fotuzlab CreditAttribution: fotuzlab commented
Status: Needs work » Needs review
FileSize
fboauth-current-password-1398782-20.patch571 bytes

@quicksketch: I have renamed the patch.
IMO it should be marked critical for its usability issue. Of course you can reset your password, but then you need to logout first. Most users do not think so hard. This bug was reported to me by a user and to her it was a blocker, without any work around!

meSte’s picture

meSte
Primary language Italian
Location Rimini
CreditAttribution: meSte commented
FileSize
fboauth-no-way-to-set-your-password-1398782.patch1.15 KB

Just adapted the patch to the drupal standard.

@fotuzlab it's not just a matter of file name, maybe this page can be helpful in the future. It was for me.

giorgio79’s picture

giorgio79 CreditAttribution: giorgio79 commented

Drupal for FB has the same issue, but it auto generates a password during account creation. I added a patch there that notifies the user of this pass here #1326430: Sending account creation notice with password to new user after using FB Connect. Maybe it can be considered as an alternative way here, as it is one less thing for the user to do or click.

fotuzlab’s picture

fotuzlab CreditAttribution: fotuzlab commented

Thanx Davide for the link. I'll surely follow these guidelines to submit a patch in future :)

@giorgio79: Interesting! I was thinking on the lines of how users are acclimatized to using facebook connect...click and over, no mails, no notifications, no pass generation. But surely your solution solves the security issues one may have. Lets have others' opinions.

quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch commented

But surely your solution solves the security issues one may have. Lets have others' opinions.

I don't know... e-mailing a user a raw-text password is about the least secure thing you can possibly do. IMO that's much worse than disabling the existing password field. Sending passwords over unencrypted traffic like e-mail is highly susceptible to interception.

meSte’s picture

meSte
Primary language Italian
Location Rimini
CreditAttribution: meSte commented

I agree. Sending passwords via email is really a bad habit.
Surely more exploitable than having no "old password" field in the user edit form for those users who haven't set their password yet.
(imho)

giorgio79’s picture

giorgio79 CreditAttribution: giorgio79 commented

If it's included in Drupal core I guess it passed security review http://api.drupal.org/api/drupal/modules%21user%21user.module/function/_......

meSte’s picture

meSte
Primary language Italian
Location Rimini
CreditAttribution: meSte commented

It's included as an option, not as the only way.
What about doing the same and add an option to let the site admin choose?

quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch commented

If it's included in Drupal core I guess it passed security review http://api.drupal.org/api/drupal/modules%21user%21user.module/function/_......

Drupal core doesn't have a token any more for password, you can only send a password reset link (essentially the same thing as users using the reset password form). The default e-mails that come out of the box just say literally Password: Your Password.

In any case, I think the current approach is taking the right path. I need to find time to actually sit down and apply it and test it out.

quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch commented
Status: Needs review » Fixed

The approach in #21 worked great for me. I only found one problem where the message is displayed immediately after the user submits the form, which is rather confusing. This patch works the same way but only displays the message when the form is initially viewed and not on submit. Thanks for the patch, committed to the D7 branch.

quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch commented
FileSize
fboauth_password-d7.patch1003 bytes

Here's the patch of the committed changes.

giorgio79’s picture

giorgio79 CreditAttribution: giorgio79 commented

Thanks quicksketch could you create a new release please? This is a great new feature. :)

quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch commented

I'd like to get #1441758: Support deauthorization requests from Facebook solved first too.

Status: Fixed » Closed (fixed)

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

mike.davis’s picture

mike.davis CreditAttribution: mike.davis commented
Status: Closed (fixed) » Needs review
FileSize
fboauth-no-way-to-set-your-password-1398782-34.patch1.74 KB

I have updated this slightly to use the hook_user_login() to highlight to the user that they should set an account password every time a user logs in.

This helps to make the user accounts more secure even though it is linked to a Facebook account.

mike.davis’s picture

mike.davis CreditAttribution: mike.davis commented
FileSize
fboauth-no-way-to-set-your-password-1398782-35.patch1.78 KB

Updated user_login() hook to work with the popup window from #1364698: Popup / modal dialog?

  • 83086f5 committed on 7.x-2.x 
    Issue #1398782 by fotuzlab and meSte: No way to set your password in...