Enable user authentication by access token #225

Merged
merged 5 commits into from Feb 4, 2013

Projects

None yet

2 participants

@jhkchan

As the case written in http://stackoverflow.com/questions/4623974/design-for-facebook-authentication-in-an-ios-app-that-also-accesses-a-secured-we, it is possible that access tokens obtained from mobile apps (iOS or Android) would be used for server side authentication.

This change set retrieves access_token pass by request during facebook login check to enable facebook user authentication by access token. This also handles the issue #210.

@diegoholiveira
FriendsOfSymfony member

Using the access_token for auth can cause security issues? I mean, if someone get your access_token it will be possible to pass as you, right?

@jhkchan

I agree. So developers should understand that it should be passed by HTTPS only just like using graph.facebook.com. This is a valid use-case.

@diegoholiveira
FriendsOfSymfony member

Can you provide some documentation about it? If you include examples of how and where to use it will be awesome.

@jhkchan

Should I do it here, or edit README.mdand pull again? I will provide detailed steps on API calls using Facebook SDK and called by mobile app.

@diegoholiveira
FriendsOfSymfony member

Do it in the README.md and pull again, please. :)

@jhkchan

Is this readme okay?

@diegoholiveira
FriendsOfSymfony member

It's fine.

I'll merge it once the tests are green. Take a look here: https://travis-ci.org/FriendsOfSymfony/FOSFacebookBundle/jobs/4073267

@jhkchan

I read it but don't understand why updating readme will make build fail? I saw the signature of AuthenticationException is different but I didn't change there. Is there something that I need to fix?

@diegoholiveira diegoholiveira commented on the diff Jan 13, 2013
Security/Authentication/Provider/FacebookProvider.php
@@ -69,6 +69,9 @@ public function authenticate(TokenInterface $token)
}
try {
+ if (!is_null($token->getAccessToken())) {
+ $this->facebook->setAccessToken($token->getAccessToken());
@diegoholiveira
diegoholiveira Jan 13, 2013

@jhkchan probably this is the cause of failure on the tests. You should verify and treat any exception raised by this call.

@jhkchan
jhkchan Jan 16, 2013

@diegoholiveira This line throws no exception. I am still investigating the cause of the failed test. thanks.

@jhkchan

I passed the unit test on my local side, but not on Travis. I find Travis uses Symfony 2.2, not Symfony 2.1. That could be the problem.

@diegoholiveira
FriendsOfSymfony member

I'll tag specific versions for symfony 2.1 and symfony 2.2 (I guess I can do it later today).

@jhkchan

I have tried to reproduce the issue and identified the problem exists in the current codebase.

To illustrate, I would like to reproduce the error by the following reduced example:

<?php

try {
    throw new \Exception('message');
} catch (\Exception $failed) {
    throw new \RuntimeException($failed->getMessage(), null, (int)$failed->getCode(), $failed);
}

?>

This will return

PHP Fatal error:  Wrong parameters for Exception([string $exception [, long $code [, Exception $previous = NULL]]]) in
XXX/xxx.php on line 6

The correct code would be removing the argument null.

References:
https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Security/Core/Exception/AuthenticationException.php
http://php.net/manual/en/class.runtimeexception.php
http://php.net/manual/en/class.exception.php

Why the current codebase would not trigger this because all are caught by the first catch identifying AuthenticationException. I am not sure why my code triggered the coverage path to \Exception but that is the case. Should I commit the fix to here as well? Thanks!

@diegoholiveira
FriendsOfSymfony member

Could you update your PR with the last version on master? This last version use a stable version of symfony 2.1 and maybe it can solve this problem. If not, I'll take a look to see what's happen.

thanks.

@jhkchan

Please let me know what I can do. Thanks.

@diegoholiveira
FriendsOfSymfony member

I'll take a look on it and give you an answer.

@jhkchan

I would suggest you to rebuild again on Travis. Something was wrong. Now I have mine passed: https://travis-ci.org/jhkchan/FOSFacebookBundle

@diegoholiveira
FriendsOfSymfony member
@diegoholiveira diegoholiveira merged commit e693acd into FriendsOfSymfony:master Feb 4, 2013

1 check failed

Details default The Travis build failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment