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

Lowercase input filter #4906

Draft
wants to merge 3 commits into
base: devel
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions api/src/DTO/ResetPassword.php
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ class ResetPassword {
public ?string $id = null;

#[InputFilter\Trim]
#[InputFilter\Lowercase]
#[ApiProperty(readable: true, writable: true)]
#[Groups(['create', 'read'])]
public ?string $email = null;
Expand Down
1 change: 1 addition & 0 deletions api/src/Entity/CampCollaboration.php
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,7 @@ class CampCollaboration extends BaseEntity implements BelongsToCampInterface {
* a user account. Either this field or the user field should be null.
*/
#[InputFilter\Trim]
#[InputFilter\Lowercase]
#[Assert\Email]
#[Assert\Length(min: 1, max: 128)]
#[AssertEitherIsNull(other: 'user')]
Expand Down
2 changes: 2 additions & 0 deletions api/src/Entity/Profile.php
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ class Profile extends BaseEntity {
* Can only be changed by setting the newEmail field, which triggers an email verification flow.
*/
#[InputFilter\Trim]
#[InputFilter\Lowercase]
#[Assert\NotBlank]
#[Assert\Email]
#[ApiProperty(example: self::EXAMPLE_EMAIL)]
Expand All @@ -65,6 +66,7 @@ class Profile extends BaseEntity {
* If set, a verification email is sent to this email address.
*/
#[InputFilter\Trim]
#[InputFilter\Lowercase]
#[Assert\Email]
#[ApiProperty(example: self::EXAMPLE_EMAIL)]
#[Groups(['write'])]
Expand Down
13 changes: 13 additions & 0 deletions api/src/InputFilter/Lowercase.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<?php

namespace App\InputFilter;

/**
* Lowercases a string using the PHP strtolower() function.
*/
#[\Attribute(\Attribute::TARGET_PROPERTY | \Attribute::IS_REPEATABLE)]
class Lowercase extends FilterAttribute {
public function __construct(array $options = [], int $priority = 10) {
parent::__construct($options, $priority);
}
}
30 changes: 30 additions & 0 deletions api/src/InputFilter/LowercaseFilter.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
<?php

namespace App\InputFilter;

class LowercaseFilter extends InputFilter {
/**
* {@inheritdoc}
*/
public function applyTo(array $data, string $propertyName): array {
if (!array_key_exists($propertyName, $data)) {
return $data;
}

$value = $data[$propertyName];

if (null === $value) {
return $data;
}

if (!is_scalar($value) && !(\is_object($value) && method_exists($value, '__toString'))) {
throw new UnexpectedValueException('Cannot convert value to string for lowercasing.');
}

$value = (string) $value;

$data[$propertyName] = strtolower($value);

return $data;
}
}
3 changes: 2 additions & 1 deletion api/src/Security/OAuth/GoogleAuthenticator.php
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,8 @@ public function authenticate(Request $request): Passport {
}

// do we have a matching user by email?
$profile = $profileRepository->findOneBy(['email' => $email]);
$profiles = $profileRepository->findBy(['email' => strtolower($email)]);
$profile = count($profiles) > 0 ? $profiles[0] : null;
Comment on lines +60 to +61
Copy link
Member

Choose a reason for hiding this comment

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

As long as we don't do the data migration, this will break the login for people who log in via OAuth and have capital letters in their email address on the external identity provider.

We should probably do the data migration, and also make sure that the email address which we get from the OAuth providers (here and here) is lower cased.

Another idea I had to go without data migration would be to compare here in the authenticators using SQL WHERE LOWER("email") = ?. But that might introduce be a security issue, and would break our undocumented feature which allows users to have both a normal login and an OAuth login to the same account.

$user = $profile?->user;

if (is_null($profile)) {
Expand Down
3 changes: 2 additions & 1 deletion api/src/Security/OAuth/HitobitoAuthenticator.php
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,8 @@ public function authenticate(Request $request): Passport {
}

// do we have a matching user by email?
$profile = $profileRepository->findOneBy(['email' => $email]);
$profiles = $profileRepository->findBy(['email' => strtolower($email)]);
$profile = count($profiles) > 0 ? $profiles[0] : null;
$user = $profile?->user;

if (is_null($profile)) {
Expand Down
23 changes: 23 additions & 0 deletions api/tests/Api/CampCollaborations/CreateCampCollaborationTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,29 @@ public function testCreateCampCollaborationTrimsInviteEmail() {
]));
}

public function testCreateCampCollaborationLowercasesInviteEmail(): void {
static::createClientWithCredentials()->request(
'POST',
'/camp_collaborations',
[
'json' => $this->getExampleWritePayload(
[
'inviteEmail' => 'sOmeonE@example.COM',
],
['user']
),
]
);

$this->assertResponseStatusCodeSame(201);
$this->assertJsonContains($this->getExampleReadPayload([
'inviteEmail' => 'someone@example.com',
'_links' => [
'user' => null,
],
]));
}

public function testCreateCampCollaborationWithInviteEmailInsteadOfUserIsPossible() {
static::createClientWithCredentials()->request('POST', '/camp_collaborations', ['json' => $this->getExampleWritePayload([
'inviteEmail' => 'someone@example.com',
Expand Down
18 changes: 18 additions & 0 deletions api/tests/Api/ResetPassword/ResetPasswordTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,24 @@ public function testPostResetPasswordTrimsEmail() {
self::assertEmailCount(1);
}

public function testPostResetPasswordLowercasesEmail() {
/** @var User $user */
$user = static::getFixture('user1manager');

$this->createBasicClient()->request(
'POST',
'/auth/reset_password',
[
'json' => [
'email' => strtoupper("{$user->getEmail()}"),
],
]
);

$this->assertResponseStatusCodeSame(204);
self::assertEmailCount(1);
}

public function testPostResetPasswordReturns204ForUnknownEmailButSendsNoEmails() {
$this->createBasicClient()->request(
'POST',
Expand Down
28 changes: 28 additions & 0 deletions api/tests/Api/Users/CreateUserTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,34 @@ public function testCreateUserTrimsEmail() {
));
}

public function testCreateUserLowercaseEmail(): void {
static::createBasicClient()->request(
'POST',
'/users',
[
'json' => $this->getExampleWritePayload(
mergeEmbeddedAttributes: [
'profile' => [
'email' => 'Bi-pi@example.COM',
],
]
),
]
);

$this->assertResponseStatusCodeSame(201);
$this->assertJsonContains($this->getExampleReadPayload(
[
'_embedded' => [
'profile' => [
'email' => 'bi-pi@example.com',
],
],
],
['password']
));
}

public function testCreateUserValidatesMissingEmail() {
// use this easy way here, because unsetting a nested attribute would be complicated
$exampleWritePayload = $this->getExampleWritePayload();
Expand Down
77 changes: 77 additions & 0 deletions api/tests/InputFilter/LowercaseFilterTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
<?php

namespace App\Tests\InputFilter;

use App\InputFilter\LowercaseFilter;
use App\InputFilter\UnexpectedValueException;
use PHPUnit\Framework\Attributes\DataProvider;
use PHPUnit\Framework\TestCase;

/**
* @internal
*/
class LowercaseFilterTest extends TestCase {
#[DataProvider('getExamples')]
public function testInputFiltering(string $input, string $output): void {
// given
$data = ['key' => $input];
$outputData = ['key' => $output];
$trim = new LowercaseFilter();

// when
$result = $trim->applyTo($data, 'key');

// then
$this->assertEquals($outputData, $result);
}

public static function getExamples(): array {
return [
['', ''],
['abc', 'abc'],
['AbC', 'abc'],
['This Is A Test', 'this is a test'],
['TeSt123', 'test123'],
['Test@Example.com', 'test@example.com'],
['JohnDoe@GMail.COM', 'johndoe@gmail.com'],
['USER-NAME+TAG@EXAMPLE.NET', 'user-name+tag@example.net'],
['info@sub.domain.COM', 'info@sub.domain.com'],
];
}

public function testDoesNothingWhenKeyIsMissing(): void {
// given
$data = ['otherkey' => 'something'];
$lowercase = new LowercaseFilter();

// when
$result = $lowercase->applyTo($data, 'key');

// then
$this->assertEquals($data, $result);
}

public function testDoesNothingWhenValueIsNull(): void {
// given
$data = ['key' => null];
$trim = new LowercaseFilter();

// when
$result = $trim->applyTo($data, 'key');

// then
$this->assertEquals($data, $result);
}

public function testThrowsWhenValueIsNotStringable(): void {
// given
$data = ['key' => new \stdClass()];
$trim = new LowercaseFilter();

// then
$this->expectException(UnexpectedValueException::class);

// when
$trim->applyTo($data, 'key');
}
}
Loading