Skip to content

fix(usermanager): Don't throw when checking if a too long user id is an existing user #52224

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Apr 17, 2025
Merged

fix(usermanager): Don't throw when checking if a too long user id is an existing user #52224

merged 2 commits into from
Apr 17, 2025

Conversation

@nickvergessen
Copy link
Member

@nickvergessen nickvergessen commented Apr 16, 2025 •
edited
Loading

Steps

  • Write a comment or chat message with @ followed by 65+ a-z0-9 characters and enable LDAP
  • When LDAP checks if the user exists, it checks preferences
  • But preferences throws InvalidArgumentException in UserConfig::assertParams although it's not documented
  • Regression from IUserConfig #47658 @ArtificialOwl

Checklist

@nickvergessen nickvergessen added this to the Nextcloud 32 milestone Apr 16, 2025
@nickvergessen nickvergessen self-assigned this Apr 16, 2025
@nickvergessen nickvergessen requested a review from a team as a code owner April 16, 2025 15:41
@nickvergessen nickvergessen requested review from Altahrim, ArtificialOwl, icewind1991 and skjnldsv and removed request for a team April 16, 2025 15:41
blizzz
blizzz reviewed Apr 16, 2025
View reviewed changes
Copy link
Member

@blizzz blizzz left a comment

Choose a reason for hiding this comment

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

looks fine, one suggestion

@nickvergessen
fix(usermanager): Don't throw when checking if a too long user id is …
775ca88 
…an existing user

Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen nickvergessen force-pushed the bugfix/noid/dont-break-when-checking-if-too-long-user-exists branch from 8429eab to 775ca88 Compare April 16, 2025 15:47
@nickvergessen
Copy link
Member Author

/backport 775ca88 to stable31

@nickvergessen nickvergessen requested a review from blizzz April 17, 2025 07:39
@nickvergessen
fix(user): Introduce a public constant for max length of user id
1ada991 
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen nickvergessen force-pushed the bugfix/noid/dont-break-when-checking-if-too-long-user-exists branch from f464541 to 1ada991 Compare April 17, 2025 07:50
@blizzz blizzz merged commit 8410d67 into master Apr 17, 2025
213 of 233 checks passed
@blizzz blizzz deleted the bugfix/noid/dont-break-when-checking-if-too-long-user-exists branch April 17, 2025 14:55
@backportbot backportbot bot mentioned this pull request Apr 17, 2025
Merged
@nextcloud-bot nextcloud-bot mentioned this pull request Aug 19, 2025
Merged
@skjnldsv skjnldsv modified the milestones: Nextcloud 32, Nextcloud 33 Sep 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@miaulalala miaulalala miaulalala approved these changes

@provokateurin provokateurin provokateurin approved these changes

@come-nc come-nc come-nc approved these changes

@SystemKeeper SystemKeeper Awaiting requested review from SystemKeeper

@icewind1991 icewind1991 Awaiting requested review from icewind1991 icewind1991 is a code owner automatically assigned from nextcloud/server-backend

@Altahrim Altahrim Awaiting requested review from Altahrim Altahrim is a code owner automatically assigned from nextcloud/server-backend

@skjnldsv skjnldsv Awaiting requested review from skjnldsv skjnldsv is a code owner automatically assigned from nextcloud/server-backend

@ArtificialOwl ArtificialOwl Awaiting requested review from ArtificialOwl

@blizzz blizzz Awaiting requested review from blizzz

Assignees

@nickvergessen nickvergessen

Labels

3. to review Waiting for reviews bug feature: users and groups regression

Projects

None yet

Milestone

Nextcloud 32

Development

Successfully merging this pull request may close these issues.

7 participants

@nickvergessen @blizzz @miaulalala @provokateurin @come-nc @skjnldsv