username must be at least 3 characters long by peterprochaska · Pull Request #29198 · owncloud/core

peterprochaska · 2017-10-10T12:15:43Z

Description

Username must be at least 3 characters long

Related Issue

HackerOne and https://github.com/owncloud/security-tracker/issues/227

Motivation and Context

How Has This Been Tested?

You can't add a user with 'a' or 'ab'

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

DeepDiver1975 · 2017-10-10T12:21:23Z

@Peter-Prochaska did you test this? jenkins is red - please have a look

phil-davis · 2017-10-10T12:33:01Z

This will "fix" the user named "0" issue, by banning any 1 (or 2) char string as a username: #28406

phil-davis · 2017-10-10T12:34:30Z

I guess this only applies to "local" ownCloud-created usernames. Short ones could still come from LDAP?

PVince81 · 2017-10-10T12:36:11Z

Short ones could still come from LDAP?

Yes

peterprochaska · 2017-10-10T12:57:00Z

PVince81 · 2017-10-10T13:24:58Z

mmattel · 2017-10-10T14:24:25Z

 		$l = \OC::$server->getL10N('lib');
+
+		// Username must at least 3 characters long
+		if(strlen($uid) <= 3) {


This ends up in min 4

Additionally I would place it after the whitespace check...

No and no, <=3 ist min 3. And after the whitespace check we have the problem with the "."

The message says The username must be at least 3 characters long which means it can be 3, 4, 5... characters long. But if I put in a 3-character username "abc" it will not let me do it.
So the "if" test here and the message have different things. One of them needs to be changed, depending on what is the requirement.

just replace <= with <

PVince81

see @mmattel's comments

ghost · 2017-10-10T15:24:55Z

  ModPagespeed Off
-</IfModule>
-
+</IfModule>


I guess this will break the .htaccess again? AFAIK the newline is expected.

What now? Expected or not?

In the first place a change to .htaccess should never have been added to the commit. Sometimes git is seeing that it was modified during a local dev install, and then "just adds it" to the commit (it depends what git tool you are using and what is in the .gitignore file etc.)

So you need to press "Enter" at the end of the last line, commit and squash. Hopefully .htaccess will then become identical to the current version in master, and so it will disappear from this PR.

git checkout master .htaccess to revert the changes.

htaccess is not relevant to this PR / commit, so if we want something different there, needs to be fixed separately

phil-davis

Just little comments.

phil-davis · 2017-10-11T07:33:32Z

Username must be at least...

phil-davis · 2017-10-11T07:35:18Z

I like to have space between keywords an the next bracket, like in the next ``if (` below.
It is part of some coding standards - don't know if it is the rule in oC.

Is there a reason why you put that quey at the start - just before all other checks like whitespace and valid characters?
Should´nt the result be minimum three valid characters ?

Quite valid question - otherwise we end up with ' .' as user name

the answer is likely: need more ☕️ 😉

fixed error reverted .htaccess reverted .htaccess reverted .htaccess to master version and adjust if statement moved len check after other checks

PVince81

👍

PVince81 · 2017-10-13T07:52:09Z

restarted jenkins

lock · 2019-08-02T08:01:34Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

peterprochaska requested review from DeepDiver1975 and PVince81 October 10, 2017 12:16

peterprochaska added the 3 - To Review label Oct 10, 2017

peterprochaska added this to the 10.1 milestone Oct 10, 2017

DeepDiver1975 modified the milestones: 10.1, development Oct 10, 2017

DeepDiver1975 approved these changes Oct 10, 2017

View reviewed changes

DeepDiver1975 added the backport-request label Oct 10, 2017

phil-davis mentioned this pull request Oct 10, 2017

User Id "0" issues #28406

Closed

peterprochaska force-pushed the username-must-at-least-3-characters-long branch from 5c806a6 to 94ee206 Compare October 10, 2017 13:06

PVince81 reviewed Oct 10, 2017

View reviewed changes

peterprochaska force-pushed the username-must-at-least-3-characters-long branch from eb80f56 to e723c18 Compare October 10, 2017 13:34

mmattel reviewed Oct 10, 2017

View reviewed changes

PVince81 suggested changes Oct 10, 2017

View reviewed changes

ghost reviewed Oct 10, 2017

View reviewed changes

peterprochaska force-pushed the username-must-at-least-3-characters-long branch from e4bccad to 23e7eb0 Compare October 11, 2017 07:32

phil-davis reviewed Oct 11, 2017

View reviewed changes

username must be at least 3 characters long

e682412

fixed error reverted .htaccess reverted .htaccess reverted .htaccess to master version and adjust if statement moved len check after other checks

peterprochaska force-pushed the username-must-at-least-3-characters-long branch from 3484c4e to e682412 Compare October 11, 2017 08:00

PVince81 approved these changes Oct 11, 2017

View reviewed changes

PVince81 added 4 - To release and removed 3 - To Review labels Oct 11, 2017

DeepDiver1975 merged commit 67c8e05 into master Oct 13, 2017

DeepDiver1975 deleted the username-must-at-least-3-characters-long branch October 13, 2017 10:45

This was referenced Oct 13, 2017

[stable10] username must be at least 3 characters long #29237

Merged

[stable9.1] username must be at least 3 characters long #29239

Merged

DeepDiver1975 removed the backport-request label Oct 13, 2017

cdamken mentioned this pull request Dec 18, 2017

API controller is not catching exceptions from user manager nextcloud/registration#110

Closed

ownclouders added the status/STALE label Dec 20, 2017

sharidas mentioned this pull request Jan 28, 2019

Usernames with 2 characters are not allowed and throw error #33770

Closed

lock Bot locked as resolved and limited conversation to collaborators Aug 2, 2019

Uh oh!

Conversation

peterprochaska commented Oct 10, 2017

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Uh oh!

DeepDiver1975 commented Oct 10, 2017

Uh oh!

phil-davis commented Oct 10, 2017

Uh oh!

phil-davis commented Oct 10, 2017

Uh oh!

PVince81 commented Oct 10, 2017

Uh oh!

peterprochaska commented Oct 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phil-davis Oct 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PVince81 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phil-davis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DeepDiver1975 Oct 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PVince81 left a comment

Choose a reason for hiding this comment

Uh oh!

PVince81 commented Oct 13, 2017

Uh oh!

lock Bot commented Aug 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

peterprochaska commented Oct 10, 2017 •

edited

Loading

phil-davis Oct 11, 2017 •

edited

Loading

DeepDiver1975 Oct 11, 2017 •

edited

Loading