Skip to content

Commit

Permalink
Enforce domain restrictions on the domain part of the email address
Browse files Browse the repository at this point in the history
  • Loading branch information
@fguillot
fguillot committed Nov 25, 2017
1 parent 81101c1 commit 2c283fc
Show file tree
Hide file tree
Showing 4 changed files with 26 additions and 4 deletions.
3 changes: 1 addition & 2 deletions .travis.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,9 @@ php:
env:
global:
- PLUGIN=GoogleAuth
- KANBOARD_REPO=https://github.com/fguillot/kanboard.git
- KANBOARD_REPO=https://github.com/kanboard/kanboard.git
matrix:
- DB=sqlite
- DB=mysql
- DB=postgres

matrix:
Expand Down
7 changes: 6 additions & 1 deletion Auth/GoogleAuthProvider.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,12 @@ public function validateDomainRestriction(array $profile, $domains)
foreach (explode(',', $domains) as $domain) {
$domain = trim($domain);

if (strpos($profile['email'], $domain) > 0) {
if (strpos($profile['email'], '@') === false) {
return false;
}

list(, $hostname) = explode('@', $profile['email']);
if (strpos($hostname, $domain) === 0) {
return true;
}
}
Expand Down
2 changes: 1 addition & 1 deletion Plugin.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ public function getPluginAuthor()

public function getPluginVersion()
{
return '1.0.4';
return '1.0.5';
}

public function getPluginHomepage()
Expand Down
18 changes: 18 additions & 0 deletions Test/GoogleAuthTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,24 @@ public function testEmailRestrictions()
$this->container['memoryCache']->flush();
$this->assertTrue($provider->isAccountCreationAllowed(array('email' => '[email protected]')));
$this->assertFalse($provider->isAccountCreationAllowed(array('email' => '[email protected]')));
$this->assertFalse($provider->isAccountCreationAllowed(array('email' => '[email protected]')));

$this->assertTrue($this->container['configModel']->save(array('google_account_creation' => '1', 'google_email_domains' => 'example.org, example.com')));
$this->container['memoryCache']->flush();
$this->assertTrue($provider->isAccountCreationAllowed(array('email' => '[email protected]')));
$this->assertTrue($provider->isAccountCreationAllowed(array('email' => '[email protected]')));
$this->assertFalse($provider->isAccountCreationAllowed(array('email' => '[email protected]')));
$this->assertFalse($provider->isAccountCreationAllowed(array('email' => 'invalid email')));

$this->assertTrue($this->container['configModel']->save(array('google_account_creation' => '1', 'google_email_domains' => 'example')));
$this->container['memoryCache']->flush();
$this->assertTrue($provider->isAccountCreationAllowed(array('email' => 'me@example')));
$this->assertFalse($provider->isAccountCreationAllowed(array('email' => 'example@localhost')));

$this->assertTrue($this->container['configModel']->save(array('google_account_creation' => '1', 'google_email_domains' => 'example.org')));
$this->container['memoryCache']->flush();
$this->assertTrue($provider->isAccountCreationAllowed(array('email' => '[email protected]')));
$this->assertFalse($provider->isAccountCreationAllowed(array('email' => '[email protected]')));
}

public function testGetClientId()
Expand Down

2 comments on commit 2c283fc

@kousu
Copy link

@kousu kousu commented on 2c283fc Nov 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Hi there.

There is a missing case. You covered

        $this->assertFalse($provider->isAccountCreationAllowed(array('email' => '[email protected]')));

but not

         $this->assertFalse($provider->isAccountCreationAllowed(array('email' => '[email protected]')));

if example.org is owned by an attacker they can break in. And google allows you to register any email address as a google account, so all someone needs is their own domain and email server to pull that off.

I think that rather than

            if (strpos($hostname, $domain) === 0) {

you should use

            if (trim($hostname) === trim($domain)) {

@kousu
Copy link

@kousu kousu commented on 2c283fc Nov 28, 2017

Choose a reason for hiding this comment

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

And a small improvement: you could also pull


+            if (strpos($profile['email'], '@') === false) {
+                return false;
+            }
+
+            list(, $hostname) = explode('@', $profile['email']);

out of the loop, since it doesn't depend on the iteration.

Please sign in to comment.