Skip to content

Implemented canLogIn check for authenticate method. FIXES #52 #53

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Implemented canLogIn check for authenticate method. FIXES #52 #53

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 4 additions & 0 deletions code/authenticator/RESTfulAPI_TokenAuthenticator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -452,6 +452,10 @@ private function validateAPIToken($token)
//all good, log Member in
if ( is_a($tokenOwner, 'Member') )
{
$loginCheck = $tokenOwner->canLogIn();
if(!$loginCheck->valid()){
return new RESTfulAPI_Error(403, $loginCheck->message());
}
$tokenOwner->logIn();
}

Expand Down
89 changes: 87 additions & 2 deletions tests/authenticator/RESTfulAPI_TokenAuthenticator_Test.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
class RESTfulAPI_TokenAuthenticator_Test extends RESTfulAPI_Tester
{
protected $requiredExtensions = array(
'Member' => array('RESTfulAPI_TokenAuthExtension')
'Member' => array('RESTfulAPI_TokenAuthExtension', 'TestCanLoginExtension')
);

protected function getAuthenticator()
Expand All @@ -35,6 +35,11 @@ public function setUpOnce()
'Email' => '[email protected]',
'Password' => 'test'
))->write();

Member::create(array(
'Email' => '[email protected]',
'Password' => 'flame.sword'
));
}


Expand Down Expand Up @@ -209,4 +214,84 @@ public function testAuthenticate()
"TokenAuth authentication failure should return a RESTfulAPI_Error"
);
}
}

public function testCanLogin(){
$auth = $this->getAuthenticator();

// check login when canLogIn check fails
$request = new SS_HTTPRequest(
'GET',
'api/auth/login',
array(
'email' => '[email protected]',
'pwd' => 'flame.sword'
)
);

$result = $auth->login($request);

$this->assertEquals(
$result['code'],
RESTfulAPI_TokenAuthenticator::AUTH_CODE_LOGIN_FAIL,
"Logging in a member whose `canLogIn` check fails should not be allowed."
);

// check authenticate when canLogIn check fails

$request = new SS_HTTPRequest(
Copy link
Owner

@colymba colymba Jul 16, 2015

Choose a reason for hiding this comment

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

Do we really need to do another test here? Since all that changes is the if logic in the canLogInExtesnion?

Copy link
Contributor Author

@bummzack bummzack Jul 16, 2015

Choose a reason for hiding this comment

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

We need to test login and authenticate. The first test is for a user that can't login, the second test is for a user that was logged in (eg. has a token) but then some condition changes, so his access via token (authenticate) should fail.

'GET',
'api/auth/login',
array(
'email' => '[email protected]',
'pwd' => 'test'
)
);

$result = $auth->login($request);

$this->assertEquals(
$result['code'],
RESTfulAPI_TokenAuthenticator::AUTH_CODE_LOGGED_IN,
"Logging in a member whose `canLogIn` check doesn't fail should be allowed."
);

$request = new SS_HTTPRequest(
'GET',
'api/ApiTest_Book/1'
);
$request->addHeader('X-Silverstripe-Apitoken', $result['token']);

// deny all users for the authenticate request
TestCanLoginExtension::$denyAll = true;

$result = $auth->authenticate($request);

TestCanLoginExtension::$denyAll = false;

$this->assertContainsOnlyInstancesOf(
'RESTfulAPI_Error',
array($result),
"TokenAuth authentication should fail when 'canLogIn' fails"
);
}
}

/**
* Extension to test "canLogin"
*/
class TestCanLoginExtension extends DataExtension
Copy link
Owner

@colymba colymba Jul 16, 2015

Choose a reason for hiding this comment

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

Would be cleaner to have this as a different file and rename the class to follow the same pattern as the other ones like RESTfulAPI_CanLogInExtension_Test.php

Copy link
Contributor Author

@bummzack bummzack Jul 16, 2015

Choose a reason for hiding this comment

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

It's only being used for this test-case.. so moving it to an external file seems counter-intuitive as it makes understanding of the test more difficult. As for naming, feel free to rename it.

{
public static $denyAll = false;

public function canLogIn(ValidationResult &$result)
{
if(self::$denyAll){
Copy link
Owner

@colymba colymba Jul 16, 2015

Choose a reason for hiding this comment

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

Do we need all this login? We could just have one line like $result->error('You shall not pass!'); which would have the same effect.. this would simplify the whole thing.

Copy link
Contributor Author

@bummzack bummzack Jul 16, 2015

Choose a reason for hiding this comment

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

We need to test login and authenticate. denyAll is being used to deny access to a member that can login ([email protected]). We could just use a single user and only the denyAll flag...

$result->error('All access denied');
}

if($this->owner->Email === '[email protected]'){
// deny access to the balrog!
$result->error('You shall not pass!');
}
}
}