Skip to content

Do not allow whitespace characters in user names.#4888

Merged
driusan merged 2 commits intoaces:21.0-releasefrom
nicolasbrossard:no_whitespaces_in_unames
Jun 18, 2019
Merged

Do not allow whitespace characters in user names.#4888
driusan merged 2 commits intoaces:21.0-releasefrom
nicolasbrossard:no_whitespaces_in_unames

Conversation

@nicolasbrossard
Copy link
Copy Markdown
Contributor

@nicolasbrossard nicolasbrossard commented Jun 17, 2019 •
edited
Loading

Brief summary of changes

Spaces are not allowed anymore in user names. This PR implements this new restriction.

This resolves issue...

To test this change...

  • Try creating a new user with a username that contains a whitespace character and ensure you cannot.
  • Create a new user and set the user name to be the user's email address. Enter an email address that contains a whitespace (e.g an email with an embedded comment that contains a space, as in crusty(the clown)@simsons.com) and try clicking 'Save'. Ensure that you get an error message.

This replaces #4744, which is now closed.

@nicolasbrossard nicolasbrossard changed the title Do not allow whitespace characters un user names. Do not allow whitespace characters in user names. Jun 17, 2019
@nicolasbrossard nicolasbrossard requested a review from ridz1208 June 17, 2019 19:19
@nicolasbrossard nicolasbrossard added 21.0.0 Testing Category: Bug PR or issue that aims to report or fix a bug labels Jun 17, 2019
@nicolasbrossard nicolasbrossard mentioned this pull request Jun 17, 2019
Closed
3 tasks
driusan
driusan reviewed Jun 17, 2019
View reviewed changes
modules/user_accounts/php/edit_user.class.inc Outdated
// Check that user name does not contain a whitespace character
if (preg_match('/\s/', $effectiveUID)) {
// Note: email adresses can contain comments which themselves can
// contain spaces
Copy link
Copy Markdown
Collaborator

@driusan driusan Jun 17, 2019

Choose a reason for hiding this comment

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

That's true for mail clients software, but is it true for LORIS?

The name part of "John Smith" <john.smith@example.com> is constructed, not user supplied. (Assuming that's what you're referring to..)

Copy link
Copy Markdown
Contributor Author

@nicolasbrossard nicolasbrossard Jun 17, 2019

Choose a reason for hiding this comment

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

You can enter an email address like me(yeah really me)@test.com on the create new user page in LORIS, and that is a valid email address with a user-supplied comment. My PR prevents the user name from being set to the email address in such a (rare/extreme) case.

Copy link
Copy Markdown
Collaborator

@driusan driusan Jun 17, 2019

Choose a reason for hiding this comment

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

TIL. (That's an insane thing for the spec to allow.)

johnsaigle
johnsaigle reviewed Jun 18, 2019
View reviewed changes
modules/user_accounts/php/edit_user.class.inc Outdated
// contain spaces
if ($values['NA_UserID'] == 'on') {
$errors['UserID_Group']
= 'You cannot have the user name match an email adress'
Copy link
Copy Markdown
Contributor

@johnsaigle johnsaigle Jun 18, 2019

Choose a reason for hiding this comment

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

Suggested change
= 'You cannot have the user name match an email adress'
= 'You cannot have the user name match an email address'

Small typo.

Also I'm a little bit confused by this part of the code. What is this checking for?

Copy link
Copy Markdown
Contributor Author

@nicolasbrossard nicolasbrossard Jun 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@johnsaigle I am trying to display an error message that is coherent with the way that the user name is set (i.e whether it was typed in the user name text field or if the 'Make user name match email address' checkbox was used).

@driusan
Copy link
Copy Markdown
Collaborator

driusan commented Jun 18, 2019

@nicolasbrossard this is failing Travis

@nicolasbrossard
Copy link
Copy Markdown
Contributor Author

nicolasbrossard commented Jun 18, 2019

@driusan It fails for PHP 7.2 but passes for PHP 7.3. Travis "bug"?

@driusan
Copy link
Copy Markdown
Collaborator

driusan commented Jun 18, 2019

you're right, looks transient.. I just restarted it

kongtiaowang
kongtiaowang approved these changes Jun 18, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@kongtiaowang kongtiaowang left a comment

Choose a reason for hiding this comment

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

LGTM

@kongtiaowang kongtiaowang added the Passed manual tests PR has been successfully tested by at least one peer label Jun 18, 2019
@driusan driusan merged commit 0cddda0 into aces:21.0-release Jun 18, 2019
@ridz1208 ridz1208 added this to the 21.0.0 milestone Jun 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@driusan driusan driusan left review comments

@kongtiaowang kongtiaowang kongtiaowang approved these changes

@ridz1208 ridz1208 ridz1208 approved these changes

+1 more reviewer

@johnsaigle johnsaigle johnsaigle approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@nicolasbrossard nicolasbrossard

@ridz1208 ridz1208

Labels

Category: Bug PR or issue that aims to report or fix a bug Passed manual tests PR has been successfully tested by at least one peer

Projects

None yet

Milestone

21.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@nicolasbrossard @driusan @johnsaigle @kongtiaowang @ridz1208