[IMP] base: unify portal accounts

[kmdi-odoo]

This commit adds a validation on the portal user signup. From this commit, new login emails are matched to exisiting logins case-insensitively and by removing the trailing whitespaces.

Task-4247745

[robodoo]

[kmdi-odoo]

Task has been cancelled.

[mrsr-odoo]

Hello @kmdi-odoo ,
Left some comments
Thanks

[mrsr-odoo]

Can't we strip it when preparing qcontext ?

[beledouxdenis]

I am not sure to allow having the same login, once active and once not active is ok regarding the security. At first sight I would say it's ok, but it's been 15+ years that we do not allow this, that the unique constraint is based on the login only and not the active boolean, so there might be a need to think this through correctly

@odony

[beledouxdenis]

Instead of doing a python constraint, shouldn't you modify the existing SQL constraint ?

_sql_constraints = [
        ('login_key', 'UNIQUE (login)', 'You can not have two users with the same login!')
    ]

Instead of a UNIQUE (login) constraint, it should be a unique constraint on the lower login + active

The guideline tells that It's always preferred to do constraint in SQL rather than in python if this is possible. Python constraints are used only when it's not possible to do the constraint in SQL. An example is when you need to have a constraint on multiple tables, but this is not the case here.

Regarding the constraint for website_id, where you use a UNION, shouldn't you use a COALESCE instead ?

GROUP BY lower(login), coalesce(website_id, false)

That would simplify the query rather than doubling it with a union

[kmdi-odoo]

Instead of doing a python constraint, shouldn't you modify the existing SQL constraint ?

Yes, I actually thought of modifying existing SQL constraint, but didn't do it after thinking of both approaches:

1. If we are keeping no duplicate case-insensitive login (including archived):
   The existing dbs having duplicate case-insensitive logins will break the SQL constraint as the upgrade script will only archive the duplicate users therefore can be done using python constraint.

2. Allow only one login for active (case-insensitively unique for unarchived only) (SQL constraint):

   1. I was really unsure to go with this before asking seniors.
   2. If we want to do it, we might need to use models.UniqueIndex as models.Constraint does not allow conditions and functions to be applied on the constraint.
   3. Seems that models.UniqueIndex doesn't show nice error message as we show for models.Constraint (though we can see what we can do). I was also unsure about performance as creating and deleting the user will make postgres index recomputation.

Regarding the constraint for website_id, where you use a UNION, shouldn't you use a COALESCE instead ?

-> Seems that SQL treats NULLs in the GROUP BY as same groupable value. Anyways, we can always make code robust by using COALESCE:)

[xmo-odoo]

If we want to do it, we might need to use models.UniqueIndex as models.Constraint does not allow conditions and functions to be applied on the constraint.

That's correct but not an issue: a unique constraint and a unique index are essentially the same thing, the only difference is some of the more advanced capabilities (e.g. indexes can be partial and created concurrently, but constraints can be deferred), under hood a unique constraints creates and uses a unique index:

Adding a unique constraint will automatically create a unique B-tree index on the column or group of columns listed in the constraint.

which is why you can ADD CONSTRAINT USING INDEX (for a unique or primary constraint).

Seems that models.UniqueIndex doesn't show nice error message as we show for models.Constraint (though we can see what we can do)

It takes a message as second parameter so I'd assume it should:

You can also specify a message to be used when constraint is violated.

that might be something to check with the orm team if it doesn't work correctly.

I was also unsure about performance as creating and deleting the user will make postgres index recomputation.

See above. A unique constraint and a unique index are essentially the same thing, so require the same amount of upkeep.

[kmdi-odoo]

Hello @odoo/rd-security,

As @beledouxdenis is off this week, can anyone give this one a look?

Thanks

[xmo-odoo]

That's going to kill the performances of the searches, on DBs with a large number of records it's going to be really bad.

Also this doesn't make sense, since in website the change to the constraint keeps login as case-sensitive if there is no website_id? But this change means all uses of _get_login_domain are now CI.

[kmdi-odoo]

That's going to kill the performances of the searches, on DBs with a large number of records it's going to be really bad.

Then we should maybe try to query the db directly to search for similar logins?

Also this doesn't make sense, since in website the change to the constraint keeps login as case-sensitive if there is no website_id? But this change means all uses of _get_login_domain are now CI.

If I am not overlooking, in website the first part of the union was kept as it was before, to avoid having duplicates (CS) with website_id as null. (As UNIQUE constraint treats nulls as not equal).

[xmo-odoo]

I am not sure to allow having the same login, once active and once not active is ok regarding the security. At first sight I would say it's ok, but it's been 15+ years that we do not allow this, that the unique constraint is based on the login only and not the active boolean, so there might be a need to think this through correctly

I think it creates all sort of odd inconsistencies e.g. the notion of portal becomes stranger as the (invisible) website_id fields becomes a much bigger factor, and it could be set on internal accounts, thus making them case insensitive and subject to being reused if deactivated.

Because deactivating an account now moves them out of UNIQUE, if you disable a portal user's account they can now recreate the same account immediately, which I'm not sure is great?

[xmo-odoo]

Odo feedback:

• ignoring archived users is both confusing and dangerous e.g. prevents blocking logins, logins can be reused after a user is disabled, and seems completely unnecessary, just rename the duplicates (then archive them)
• this creates confusion between portal-as-group and portal-as-website-user (which is not linked to the portal group?), and thus risk
• as noted previously, indexing is completely broken which is going to be an issue for databases with massive numbers of users (e.g. odoo.com) when looking users up via login

[kmdi-odoo]

Hello @xmo-odoo,

Thanks for your comments.
I've answered the questions.

Also, what should be the final verdict for the unicity of the users (for only active or all)?

[kmdi-odoo]

That's going to kill the performances of the searches, on DBs with a large number of records it's going to be really bad.

Then we should maybe try to query the db directly to search for similar logins?

Also this doesn't make sense, since in website the change to the constraint keeps login as case-sensitive if there is no website_id? But this change means all uses of _get_login_domain are now CI.

If I am not overlooking, in website the first part of the union was kept as it was before, to avoid having duplicates (CS) with website_id as null. (As UNIQUE constraint treats nulls as not equal).

[kmdi-odoo]

Hello @xmo-odoo 👋,
cc @beledouxdenis

I've now updated code to keep unicity across all the users using models.UniqueIndex.

That will create a unique index on lower(login) serving both purposes.

1. Faster lookup on lower(login).
2. Uniqueness across column login.

Can you please have a look if its feasible for large dbs also?

Thanks