search

plone / Products.CMFPlone

The core of the Plone content management system
https://plone.org
GNU General Public License v2.0
240 stars 183 forks source link

Deleted users keep the roles they had #3937

Open wesleybl opened 2 months ago

wesleybl commented 2 months ago

BUG/PROBLEM REPORT (OR OTHER COMMON ISSUE)

When we delete a user and create him again with the same username, he keeps the roles he had before.

What I did:

What I expect to happen:

New user has no roles.

What actually happened:

The new user has the role of Editor.

What version of Plone/ Addons I am using:

Plone 6.0.10 (6021) CMF 3.3 Zope 5.9 Python 3.11.8 (main, Feb 13 2024, 10:25:57) [GCC 10.2.1 20210110] PIL 9.5.0 (Pillow) WSGI: On Server: waitress 2.1.2

davisagli commented 2 months ago

I strongly recommend to always use uuids as user ids, so they are unique. It is too difficult to find and remove a user's local roles everywhere in a large site.

yurj commented 2 months ago

Or never delete a user. Plone misses a user "disable" checkbox. It could be an useful feature, for example to deny user to log in without changing the user password or deleting the user.

stevepiercy commented 2 months ago

Or never delete a user.

This is what I would do in systems where the user was an employee and history of their activity must be retained.

In some countries for certain websites, if a user requests permanent deletion of their data, the presiding law requires deletion. The GDPR is one example: https://gdpr-info.eu/art-17-gdpr/

davisagli commented 2 months ago

@stevepiercy Good point, but in that case I think it would be best to delete all personally identifiable information but still keep the userid so it cannot be reused. Another good reason for uuid-based userids.

davisagli commented 2 months ago

@yurj I agree, that would be a useful feature and I have wanted it from time to time.

jensens commented 3 weeks ago

This is a feature, not a bug. Anyway, I always configure my sites to use UUID based userids to avoid this.

In my opinion we should switch to use UUID based user Ids by default, together with Use email address as login name, to provide sane defaults. This settings are in the security control panel.

https://github.com/plone/plone.base/blob/2ef2838c7bc564252718dcd6a7836bc866f5f781/src/plone/base/interfaces/controlpanel.py#L1111-L1137

wesleybl commented 3 weeks ago

This is a feature, not a bug.

When I exclude a user and registering another with the same login name, I don't expect him to have the roles he had when first registering. I think this is a bug.

In my opinion we should switch to use UUID based user Ids by default

So is there another user identifier other than login? When this option is enabled, is a different identifier generated, even if the login is the same? Are the roles associated with this id, not the login?

davisagli commented 3 weeks ago

In my opinion we should switch to use UUID based user Ids by default, together with Use email address as login name, to provide sane defaults.

I totally agree.

So is there another user identifier other than login?

@wesleybl The user id has always been a different variable than the user's login name. With the default settings they are often set to the same value which is a username entered when the user is created.

When this option is enabled, is a different identifier generated, even if the login is the same?

The same as what? With the settings Jens mentioned, the user id is a randomly generated UUID and the login name is an email address entered when the user is created.

Are the roles associated with this id, not the login?

Yes

wesleybl commented 3 weeks ago

The same as what? With the settings Jens mentioned, the user id is a randomly generated UUID and the login name is an email address entered when the user is created.

I think they are two different options. One to use the UUID as an identifier and the other to use the email as a login. My question is, if I check the UUID option, create a user whit login name carlos, delete it, and create a new user whit login name carlos, will the UUID be different from the first carlos? I understand that yes.

In my opinion we should switch to use UUID based user Ids by default, together with Use email address as login name, to provide sane defaults.

I think it would be good to use the UUID option but I don't know if the email option would be good. Plone has been using login name for many years

wesleybl commented 3 weeks ago

When the UUID option is enabled, does it affect users created via acl_users?

davisagli commented 3 weeks ago

My question is, if I check the UUID option, create a user whit login name carlos, delete it, and create a new user whit login name carlos, will the UUID be different from the first carlos? I understand that yes.

Yes

I think it would be good to use the UUID option but I don't know if the email option would be good. Plone has been using login name for many years

I think using email as login is slightly preferable from a UX perspective because it's one less thing you have to pick when creating a new user. We would only make the change for new sites (not in an upgrade) and it would still be possible to turn it off.

When the UUID option is enabled, does it affect users created via acl_users?

You mean in the PAS source_users plugin? Probably not.

jensens commented 3 weeks ago

I prepared a PR for Plone 6.1 to set more secure defaults https://github.com/plone/plone.base/pull/67

wesleybl commented 3 weeks ago

You mean in the PAS source_users plugin? Probably not.

Yes. Ideally, it would affect users created there as well.

wesleybl commented 3 weeks ago

This probably doesn't affect users created with plone.restapi either (Volto)

jensens commented 3 weeks ago

This probably doesn't affect users created with plone.restapi either (Volto)

That would be a serious bug.

davisagli commented 3 weeks ago

It doesn't make sense to me to for PAS source_users to be aware of Plone-level settings. If you create a user there, you're intentionally bypassing the extra functionality that Plone adds for user management.

On the other hand, it's intended for plone.restapi to handle these settings well, and if there are ways it's not doing that, those are bugs we should fix.

jensens commented 3 weeks ago

Wow, that whole usage of these settings are a PITA.

restapi just ignores those. https://github.com/plone/plone.restapi/blob/main/src/plone/restapi/services/users/add.py#L207 and I would say restapi just should use plone.api to do it right.... but:

I think plone.api ignores it too . https://6.docs.plone.org/_modules/plone/api/user.html But as you see in the code there, terms username, userid and email are mixed up slightly, so not helpful.

And this is the correct way and all above should use this way https://github.com/plone/plone.app.users/blob/47483e65bea6cef6ed9456c36f727745a66c696f/plone/app/users/browser/register.py#L127