Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Dont allow reusing usernames #27105

Closed
wants to merge 11 commits into from

Conversation

LukasReschke
Copy link
Member

@LukasReschke LukasReschke commented May 25, 2021

We have cases in which app developers forget to delete user data when a user is being deleted. This could be problematic as future users with the same username could then access this data.

This mitigates this risk a bit by not allowing reusing of usernames. That said, we still need to fix occurrences of these issues by deleting missed data.

Test plan

  • Create user in the UI
  • Delete user
  • Check table for inserted hashed user ID
  • Try to create user with same UID. See it fail.
  • Other users can still be created.

There is currently no feedback in the UI but that is due to another bug, which also affects other errors such as the ones by an enforced password policy: #27115

TODOs

  • Adjust DI for Manager
  • Fix up code
  • Listen to user deletion event and mark user name as used
  • Fixup commits
  • Fix tests that reuse the same UID

.htaccess Outdated Show resolved Hide resolved
lib/public/DB/QueryBuilder/IQueryBuilder.php Outdated Show resolved Hide resolved
@LukasReschke LukasReschke force-pushed the dont-allow-reusing-usernames branch 13 times, most recently from 48d982b to f3c6d2f Compare May 26, 2021 17:55
@LukasReschke LukasReschke force-pushed the dont-allow-reusing-usernames branch 4 times, most recently from 99a2b22 to 8355ea5 Compare May 26, 2021 19:21
Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, little bit of cleanup and good to go

We have cases in which app developers forget to delete user data when a user is being deleted. This could be problematic as future users with the same username could then access this data.

This mitigates this risk a bit by not allowing reusing of usernames. That said, we still need to fix occurrences of these issues by deleting missed data.

Signed-off-by: Lukas Reschke <lukas@statuscode.ch>
Signed-off-by: Lukas Reschke <lukas@statuscode.ch>
Signed-off-by: Lukas Reschke <lukas@statuscode.ch>
Signed-off-by: Lukas Reschke <lukas@statuscode.ch>
Signed-off-by: Lukas Reschke <lukas@statuscode.ch>
Signed-off-by: Lukas Reschke <lukas@statuscode.ch>
Signed-off-by: Lukas Reschke <lukas@statuscode.ch>
Signed-off-by: Lukas Reschke <lukas@statuscode.ch>
Signed-off-by: Lukas Reschke <lukas@statuscode.ch>
Signed-off-by: Lukas Reschke <lukas@statuscode.ch>
Signed-off-by: Lukas Reschke <lukas@statuscode.ch>
@skjnldsv skjnldsv added the 2. developing Work in progress label Jun 2, 2021
@nickvergessen
Copy link
Member

Should put into #26407 as I guess this will break the tests of a lot of apps :-X

@nickvergessen
Copy link
Member

There was 1 error:

  1. OCA\Files_Sharing\Tests\External\ManagerTest::testAddShare
    TypeError: Argument 1 passed to OCA\Files_Sharing\External\MountProvider::getMountsForUser() must implement interface OCP\IUser, null given, called in /home/runner/work/server/server/apps/files_sharing/tests/External/ManagerTest.php on line 139

/home/runner/work/server/server/apps/files_sharing/lib/External/MountProvider.php:75
/home/runner/work/server/server/apps/files_sharing/tests/External/ManagerTest.php:139
/home/runner/work/server/server/apps/files_sharing/tests/External/ManagerTest.php:174

@susnux susnux added this to the Nextcloud 30 milestone Apr 18, 2024
@skjnldsv skjnldsv closed this Aug 14, 2024
@skjnldsv skjnldsv removed this from the Nextcloud 30 milestone Aug 14, 2024
@nickvergessen nickvergessen deleted the dont-allow-reusing-usernames branch August 15, 2024 08:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants