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.

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

Improve token stability by making the API token a "unique" index. #40

Closed
wants to merge 1 commit into from
Closed

Improve token stability by making the API token a "unique" index. #40

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
9 changes: 9 additions & 0 deletions code/authenticator/RESTfulAPI_TokenAuthExtension.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,15 @@ class RESTfulAPI_TokenAuthExtension extends DataExtension
'ApiTokenExpire' => 'Int'
);

// Add a unique index to the API token.
// Should increase lookup performance and prevent duplicate tokens.
private static $indexes = array(
'ApiToken' => array(
'type' => 'unique',
'value' => '"ApiToken"'
)
);

function updateCMSFields(FieldList $fields)
{
$fields->removeByName('ApiToken');
Expand Down
65 changes: 37 additions & 28 deletions code/authenticator/RESTfulAPI_TokenAuthenticator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,14 +152,7 @@ public function login(SS_HTTPRequest $request)
));
if ( $member )
{
$tokenData = $this->generateToken();

$tokenDBColumn = $this->tokenConfig['DBColumn'];
$expireDBColumn = $this->tokenConfig['expireDBColumn'];

$member->{$tokenDBColumn} = $tokenData['token'];
$member->{$expireDBColumn} = $tokenData['expire'];
$member->write();
$tokenData = $this->updateToken($member);
$member->login();
}
}
Expand Down Expand Up @@ -203,16 +196,7 @@ public function logout(SS_HTTPRequest $request)

if ( $this->tokenConfig['owner'] === 'Member' )
{
//generate expired token
$tokenData = $this->generateToken( true );

//write
$tokenDBColumn = $this->tokenConfig['DBColumn'];
$expireDBColumn = $this->tokenConfig['expireDBColumn'];

$member->{$tokenDBColumn} = $tokenData['token'];
$member->{$expireDBColumn} = $tokenData['expire'];
$member->write();
$this->updateToken($member, true);
}
}
}
Expand Down Expand Up @@ -291,16 +275,7 @@ public function resetToken($id, $expired = false)

if ( $owner )
{
//generate token
$tokenData = $this->generateToken( $expired );

//write
$tokenDBColumn = $this->tokenConfig['DBColumn'];
$expireDBColumn = $this->tokenConfig['expireDBColumn'];

$owner->{$tokenDBColumn} = $tokenData['token'];
$owner->{$expireDBColumn} = $tokenData['expire'];
$owner->write();
$this->updateToken($owner, $expired);
}
else{
user_error("API Token owner '$ownerClass' not found with ID = $id", E_USER_WARNING);
Expand Down Expand Up @@ -344,6 +319,40 @@ private function generateToken($expired = false)
);
}

/**
* Update the token of a token owner
* @param $owner The token owner instance to update
* @param bool $expired Set to true to generate an outdated token
* @return array|null Token data array('token' => HASH, 'expire' => EXPIRY_DATE)
*/
private function updateToken($owner, $expired = false)
{
$ownerId = null;
$tokenData = null;

// DB field names
$tokenDBColumn = $this->tokenConfig['DBColumn'];
$expireDBColumn = $this->tokenConfig['expireDBColumn'];

do {
try {
//generate token
$tokenData = $this->generateToken( $expired );

// ensure we never regenerate the same token!
if($owner->{$tokenDBColumn} != $tokenData['token']){
Copy link
Owner

Choose a reason for hiding this comment

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

Should we use '!==' ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The point where the token would collide is in the Database. So, if we add the type check, do we improve or worsen the stability of the check?

The only distinction on the DB side would be 'null' != null? Otherwise everything should be treated as string (varchar). I think it's sensible to assume both tokens will become strings, so I'd stick with the != check, since checking for type inequality isn't important to the DB... or am I missing something important here?

Copy link
Owner

Choose a reason for hiding this comment

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

tokenData should always be a string, so it should be fine to use !== no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I guess stricter is better here. So using !== is fine.

$owner->{$tokenDBColumn} = $tokenData['token'];
$owner->{$expireDBColumn} = $tokenData['expire'];
$ownerId = $owner->write();
}
} catch(Exception $e){
$ownerId = null;
Copy link
Owner

Choose a reason for hiding this comment

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

Should we check the type of exception? In case the write error is something else than a non unique key?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm that would be sensible, yes. But does the ORM throw a special exception for non-uniqueness of a key?

Copy link
Owner

Choose a reason for hiding this comment

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

Thought I would raise the question, but no idea if the ORM actually returns the relevant exception info... Might be good to investigate...

}
} while(!$ownerId);

return $tokenData;
}


/**
* Returns the DataObject related to the token
Expand Down
31 changes: 31 additions & 0 deletions tests/authenticator/RESTfulAPI_TokenAuthenticator_Test.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,4 +209,35 @@ public function testAuthenticate()
"TokenAuth authentication failure should return a RESTfulAPI_Error"
);
}

/**
* Test edge case of a member with the same API token
*/
public function testTokenUniqueness()
{
$writtenCount = 0;
// attempt to write two members with the same API token
for($i = 0; $i < 2; $i++){
try {
$memberID = Member::create(array(
'Email' => 'TestMember' . $i,
'Password' => 'test',
'ApiToken' => 'Same token'
))->write();

if($memberID){
$writtenCount++;
}
} catch(Exception $e){
// catch exception to not raise errors
}
}


$this->assertEquals(
1,
$writtenCount,
'The API token must be unique'
);
}
}