FriendsOfSymfony / FOSFacebookBundle

NOT MAINTAINED - see https://github.com/hwi/HWIOAuthBundle
322 stars 140 forks source link

Website really slow after loging in #89

Open babour opened 12 years ago

babour commented 12 years ago

Hi,

As soon as I logged in, my website becomes really slow to load pages, I used firebug to find out why, and it seems, at every page loading, it take seconds to check out the cookie created by the bundle. fbrs_(fb_id)...

Does anyone happen to have this problem? Or know why this would happen?

sergsu commented 12 years ago

Agree, having this issue. Does this check so really necessary to do at each request?

babour commented 12 years ago

I don't know, I only know that it takes tremendous times and, I wish someone who coded the bundle would respond.

Anyone?

sergsu commented 12 years ago

Well, seems like we both used dummy Facebook provider from ReadMe. Look at loadUserByUsername method of it. It does metadata request each time we loading user info. May be there is need to cache this info for some time (hour/week/day), no much people change their names so frequently. But I'm not sure this will not cause security trouble or internal oAuth protocol breaking, so if anyone using kind of caching in this method, I would really appreciate any info about pitfalls here. Thanks.

babour commented 12 years ago

Indeed, because seeing how slow it is, it's unusable... But I cannot give up security for speed...

We need some of insight of why do this check and if it is mandatory for security reason...

sergsu commented 12 years ago

So, I've investigated a bit. Looks like there is only profile data gets from FB, so we can cache it flawlessly. I've just added APC caching for loadUserByUsername method and request got to do just once in hour.

babour commented 12 years ago

It seems like a nice alternative, can you tell which method loadUserByUsername you cache, because there is more than one, is it in the facebook provider, or in the user entity?

jjhoyt commented 12 years ago

The problem is with doing the facebook lookup and cookie setting on every page load via the user provider. There are a lot of ways that one could resolve this, caching as stated above, is one. I did the following, though there may be better alternatives....

As a side issue, note that if you are allowing login via the regular non-Facebook provider then you need to also account for users who have possibly already registered using the same email as with Facebook. You will run into a duplicate entry error if you do not account for that in the loadUserByUsername method. The code shown below does not allow for this, you will need to add it yourself.

In your custom FacebookProvider, and just within the loadUserByUsername method, add this line after findUserByFbId

    if($user!=null) {
        return $user;
    } 

The full method

public function loadUserByUsername($username)
{
    $user = $this->findUserByFbId($username);

    if($user!=null) {
        return $user;
    }

    try {
        $fbdata = $this->facebook->api('/me');
    } catch (FacebookApiException $e) {
        $fbdata = null;
    }

    if (!empty($fbdata)) {
        if (empty($user)) {
            $user = $this->userManager->createUser();
            $user->setEnabled(true);
            $user->setPassword('');
        }

        // TODO use http://developers.facebook.com/docs/api/realtime
        $user->setFBData($fbdata);

        if (count($this->validator->validate($user, 'Facebook'))) {
            // TODO: the user was found obviously, but doesnt match our expectations, do something smart
            throw new UsernameNotFoundException('The facebook user could not be stored');
        }
        $this->userManager->updateUser($user);
    }

    if (empty($user)) {
        throw new UsernameNotFoundException('The user is not authenticated on facebook');
    }

    return $user;
}
gregoryb commented 12 years ago

Same problem (also if i cache the user...)

dangraetzer commented 12 years ago

Is there any intention to have this fix implemented in the distribution?

babour commented 12 years ago

You don't actually have to cache User, if you don't want to update the user each time he changes pages you just have to comment "$this->facebook->api('/me');" and it's done!

mjuchli commented 12 years ago

If you comment this line, you'll never have the possibility to get the data. I'm wondering now how did you guys solved this?

I see 3 options: - session, - set true/false while login/logout, - cache

Zertz commented 11 years ago

Did anyone ever find a definitive fix to this issue?

Edit: jjhoyt's fix works well actually. Perhaps a more solid implementation could be added to the bundle.

mariochampion commented 11 years ago

@jjhoyt -- thx! simple and totally effective fix.