Hello,

While using Phone Number Login Plugin I've noticed that at localhost/user/profile (update user account page) it doesn't check if the phone is already in use.
This kinda makes the Phone number verification feature useless, as it only performs the check at Register. But if a user updates his account, he can set whatever number he likes, regardless if it's in use or not.

I've seen that this function handles the verification at register:
// WHEN NEW USER REGISTER, CHECK IF PHONE DOES NOT EXIST
function phl_check_register() {
  $phone = trim(Params::getParam('s_phone_mobile'));

  if($phone <> '') {
    $user = ModelPHL::newInstance()->findUserByPhone($phone);

    if($user && isset($user['pk_i_id'])) {
      osc_add_flash_error_message(sprintf(__('Phone number %s is already registered to another account, please use different number', 'phone_login'), $phone));
      header('Location:' . osc_register_account_url());
      exit;
    }
  }
}

What other check do we need to add, in order to make it work for the user profile page too?

Thanks!

Hello,

Is there any feedback on this?

This is a big problem, as we're having users with the same phone numbers after they edit their profiles.

Thanks!

@Tango
Based on this:
https://docs.osclasspoint.com/Hooks

It should be this one:
pre_user_post: Run before an user complete the registration, or edit his account (*3.1*)

So basically it should be enough to change this:
osc_add_hook('before_user_register', 'phl_check_register');

into this:
osc_add_hook('pre_user_post', 'phl_check_register');

Also in that function this line:
      header('Location:' . osc_register_account_url());

Into this one:
      if(osc_is_web_user_logged_in()) {
        header('Location:' . osc_user_profile_url());
      } else {
        header('Location:' . osc_register_account_url());
      }
And last one this:
    if($user && isset($user['pk_i_id'])) {

into this:
    if($user && isset($user['pk_i_id']) && $user['pk_i_id'] != osc_logged_user_id()) {


The complete working function:

Plugin functions.php
// WHEN USER REGISTERS/CHANGES THE REGISTERED PHONE NUMBER, CHECK IF PHONE DOES NOT EXIST
function phl_check_register() {
  $phone = trim(Params::getParam('s_phone_mobile'));

  if($phone <> '') {
    $user = ModelPHL::newInstance()->findUserByPhone($phone);

    if($user && isset($user['pk_i_id']) && $user['pk_i_id'] != osc_logged_user_id()) {
      osc_add_flash_error_message(sprintf(__('Phone number %s is already registered to another account, please use different number', 'phone_login'), $phone));
      if(osc_is_web_user_logged_in()) {
        header('Location:' . osc_user_profile_url());
      } else {
        header('Location:' . osc_register_account_url());
      }
      exit;
    }
  }
}

osc_add_hook('before_user_register', 'phl_check_register');
osc_add_hook('pre_user_post', 'phl_check_register');
Waiting for v1.1.2 with this fixed

Thanks a lot for the help!

Plugin updated, but I would say it was partially there already.

Please note that I've found an issue with the phone in use checking function.

Right now, the function checks only s_phone_mobile for duplication, but not s_phone_land.
This becomes a problem (especially for company type users), as they might set 2 numbers in the account.
So the issue goes like this:

• A user fills in both phone fields in his account, with the Mobile Phone and the Land Phone.
• When a new user registers, he can use the Land Phone (s_phone_land) previously set by another user.

So at register and in the user account, the function should check both fields for duplicacy.
Basically if both types of numbers are in use and associated to other users, then you can't use those numbers.

Thanks!

@Tango
This is quite tuff, as each number would have to be checked for uniqueness in fields:

• s_phone_mobile
• s_phone_land

Ok, that is not complicated, but what about this scenario:
Registered user has Land Phone: 123 and Mobile Phone: 456
New user is registering and use Land Phone: 456 and Mobile Phone: 123

Land is unique across land phones, mobile is unique across mobile phones.
What to do now?

@Tango
This is quite tuff, as each number would have to be checked for uniqueness in fields:

• s_phone_mobile
• s_phone_land

Ok, that is not complicated, but what about this scenario:
Registered user has Land Phone: 123 and Mobile Phone: 456
New user is registering and use Land Phone: 456 and Mobile Phone: 123

Land is unique across land phones, mobile is unique across mobile phones.
What to do now?

I was thinking of a single check that verifies both columns at register. It verifies if the actual number already exists in any of the fields.

- So let's say an existing user account has the Land Phone: 123 and Mobile Phone: 456
- A new user is on the register page and enters in the phone field any of the above values (123, 456) - he will get an error that the phone is already in use.
- Another existing (already registered user), is on the /user/profile page, he has no numbers set and wants to update just his Land Phone to 456 (which is the Mobile Phone of the first user) - he will get an error that the number is already in use.

So a check that verifies if the new input already exists in both s_phone_mobile and s_phone_land.

Fixing this will prevent some issues with some possible mismatches between user numbers.

Thanks!

@Tango
You could update model of phone number login, function findUserByPhone

Change:
  $this->dao->where('s_phone_mobile', $phone);

into:
  $this->dao->where('(s_phone_mobile="' . $phone . '" OR s_phone_land="' . $phone . '")');
  $this->dao->where('pk_i_id != ' . osc_logged_user_id());

Not quite sure what could be impact, need testing.

@Tango
You could update model of phone number login, function findUserByPhone

Change:

Code: [Select]

  $this->dao->where('s_phone_mobile', $phone);

into:

Code: [Select]

  $this->dao->where('(s_phone_mobile="' . $phone . '" OR s_phone_land="' . $phone . '")');
  $this->dao->where('pk_i_id != ' . osc_logged_user_id());

Not quite sure what could be impact, need testing.

I just tested it, and it works great for the Register page (it checks if the number is already in use in both tables) and returns an error if true, however it doesn't work for the User Account.
In the User Account, only the Mobile Phone goes through validation, the Land Phone can be updated to whatever number, regardless if it already exists or not.

As for the impact, fully solving this would make sure that the User Accounts have unique numbers. The same phone number shouldn't be used in 2 or more accounts at the same time.
We already have issues with spammy users, and this would add another layer of protection against them.

And let me give you an example from real life.
I worked for a company that was selling some products on a huge marketplace in our country.
One day, we started selling some products that were also sold by another guy, so we were in competition with him.
Do you know what he did? In 4 days, he placed 250 fake orders from our store, all with phone numbers gathered from the actual marketplace and from other places.
So basically, the asshole was creating tons of fake accounts, just to place fake orders and run us out of business.
In that week alone, we've lost over 200 Euros on shipping charges for fake orders, because the marketplace didn't care and forced us to ship some of the orders...

Now in Osclass, we have the Pay Plugin, that provides the same option.
You can't have a product shipped, if you don't provide a phone number. So fixing this plugin, means fixing the malicious recycling of phone numbers. 
If it's already in use, then you can't use it in your account.

Sorry for the long post.
Thanks!

EDIT:
Umm... On a second thought, what happens if an asshole spammer, creates tons of accounts with random numbers, and amongst those numbers are real ones from real people?
Basically that guy would block a real person from using his actual phone number, by already registering it in the system...
Yeah, tough call.

I would say to have the uniqueness check as an option, like this:

Problem is land phone is most probably not checked because it was not added into that function, will need to check that later

Fixed in v1.1.3