Overly broad try/catch in Facebook Provider?

[danfinnie]

Hi,

I was recently implementing FOSFacebook on top of a system that already had FOSUserBundle installed and ran into some troubles debugging because the included FacebookProvider caught exceptions from invalid configuration but presented them as exceptions from invalid form data. My Doctrine entities had problems, so when users were redirected to the /login_check route, they were presented with a login form with the text "Unrecognized field: fbid" where it would normally say "Invalid password" or something like that. I traced this down through the bundle and would like to suggest changing FacebookProvider's this very broad try catch block in the authenticate method with something more specific, at least when in non-prod environments.

What do you guys think? I can submit a pull request if there is interest.

[diegoholiveira]

Please send a PR. It will be very nice.

[danfinnie]

@diegoholiveira I just made a pull request at https://github.com/FriendsOfSymfony/FOSFacebookBundle/pull/231 . I'm not sure if the discussion should take place in this issue ticket or in the pull request, so please correct me if I'm commenting in the wrong spot.

The existing try/catch would catch exceptions thrown by any of these lines:

       if ($uid = $this->facebook->getUser()) { // No exceptions, 0 if no current user
            $newToken = $this->createAuthenticatedToken($uid); // UsernameNotFoundException or RuntimeException
            $newToken->setAttributes($token->getAttributes()); // No exceptions

            return $newToken;
        }

The $uid = line should never throw an exception, and neither should $newToken->setAttributes($token->getAttributes()). On the other hand, $this->createAuthenticatedToken can throw a UsernameNotFoundException or a RuntimeException, both of which would be rethrown as AuthenticationExceptions. I think that removing this try/catch will make it easier for devs to debug their code as it won't mask the source of the exceptions any more -- what do you think?