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.

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

How does twitter.authorized know it's me? #88

Closed
NathanWailes opened this issue Oct 29, 2017 · 14 comments
Closed

How does twitter.authorized know it's me? #88

NathanWailes opened this issue Oct 29, 2017 · 14 comments

Comments

@NathanWailes
Copy link
Contributor

NathanWailes commented Oct 29, 2017

I don't really understand how I can log out, delete my cookie, then click the "Sign in with Twitter" button and it somehow knows who I am and just logs me in directly. twitter.authorized is evaluating to True, and I saw that the code I think that's being evaluating to True is:

bool(self._client.client.client_secret) and
bool(self._client.client.resource_owner_key) and
bool(self._client.client.resource_owner_secret)

How can the session still have a resource_owner_key and resource_owner_secret after I've logged out, deleted my session cookie in Chrome, and restarted the server?

@singingwolfboy
Copy link
Owner

Because although you've logged out of your local website, you haven't logged out of Twitter.com. When you try to log in with Twitter using OAuth, the following things happen:

  1. Flask-Dance sends you to Twitter, along with some information about who sent you there.
  2. Twitter checks who sent you, and asks that you log in to Twitter. After you're logged in, Twitter asks you if you're OK with sharing your information with the website that sent you.
  3. When you agree, Twitter sends you back to the original website, along with a special token that the original website can use to call the Twitter API on your behalf.

Even if you log out of your website, restart everything, and so on, that doesn't change anything from Twitter's point of view. When you get to step 2 of the OAuth dance, Twitter is smart enough to see that you are already logged in on Twitter, and you have already agreed to sharing your information with this particular website, so they don't bother to make you confirm a second time. Instead, they just send you right back to the original website, with the special token for that website to use.

Does that make sense? If you want to truly start over the OAuth dance (usually for purposes of testing), you have to go to the provider website and revoke your permission for sharing information with the consumer website. In the case of Twitter, specifically, go to the Apps section of your Twitter settings and revoke the permission for your Twitter app. After you do that, if you try to use Flask-Dance to login with Twitter, Twitter will once again ask you for permission to share your information with the other website.

@NathanWailes
Copy link
Contributor Author

Thank you for that in-depth explanation!

So is it accurate to say that calling twitter.authorized pings Twitter?

@singingwolfboy
Copy link
Owner

No, it doesn't. If you have code that looks like this:

@app.route('/')
def index():
    if not twitter.authorized:
        return redirect(url_for('twitter.login'))
    # view continues here

The first time you hit this code, twitter.authorized will be False. That will start a chain of redirects that:

  • sends you to Twitter
  • sends you back to the /twitter/authorized page
  • sends you back to the index page

That will cause this code to run a second time, and at that point, twitter.authorized will be True. That's why it appears to all work so seamlessly. Does that make sense?

@NathanWailes
Copy link
Contributor Author

Ok, I'm still confused, because I was logging out, deleting my cookie for my website, then hitting the 'log in with twitter' button with a breakpoint set on that if not twitter.authorized line, and I was running twitter.authorized in my debugger console, and it was evaluating to True immediately. And so if it hadn't yet pinged Twitter or redirected me, how did it know who I (the user) was?

@singingwolfboy
Copy link
Owner

Hmm, that is mysterious! I forgot that Twitter is using OAuth 1 instead of OAuth 2, which makes the logic different -- I may have unintentionally lied to you earlier. I don't have time to look into this more right now, but I'll see if I can figure it out later!

@NathanWailes
Copy link
Contributor Author

NathanWailes commented Oct 31, 2017

More experiments done:

  • If I log out of my Twitter account and then log into a different Twitter account, then access my index page and set a breakpoint before it returns the template, twitter.token returns the token from the first (logged-out) Twitter account.
  • If I open an incognito window in Chrome and access the index page, the twitter.token value is still the value from my non-incognito browser session.
  • If I start an incognito session and then click the 'Log in with Twitter" button, it does not redirect me to Twitter and prompt me to log into Twitter, but instead (because the token is present) it just sends me straight through to the next_url.

In both cases, when I check the 'Application' tab of Chrome devtools, I don't see any cookies set for the domain http://127.0.0.1:5000/.


Digging through the code, this function in make_twitter_blueprint seems especially noteworthy:

@twitter_bp.before_app_request
def set_applocal_session():
    ctx = stack.top
    ctx.twitter_oauth = twitter_bp.session

The twitter_bp.session has a token entry which is the NathanWailes Twitter token that is being set (and I can't understand why/how).


Also this function in OAuth1Session:

@lazy
def token(self):
    return self.blueprint.token

I don't understand how a blueprint can have the user's token, I'm looking into that now.


Here's the line that seems to set the blueprint's token, it's in oauth1.py / OAuth1ConsumerBlueprint / authorized():

token = self.session.fetch_access_token(self.access_token_url)

It seems like the twitter_blueprint is instantiated once for each user since it has a token attribute which is specific to a particular user; how are incoming requests matched up with the right instantiation?

@NathanWailes
Copy link
Contributor Author

NathanWailes commented Oct 31, 2017

Ok, I think I've figured it out. I think the problem is with sqla.py, specifically these lines:

def get(self, blueprint, user=None, user_id=None):
    # check cache
    cache_key = self.make_cache_key(blueprint=blueprint, user=user, user_id=user_id)
    token = self.cache.get(cache_key)
    if token:
        return token

I think what is happening is that because my code isn't fully hooked up, the user_id column in my OAuth db table is None, and both the cache_key and also the db call are using "None" as a valid unique identifier. Which means that when I log out, because my user_id is still None, the twitter_blueprint is still faced with the same information for looking up the correct OAuth record, and thus it retrieves the same record.

@singingwolfboy
Copy link
Owner

Ah hah! Cache invalidation is one of the famously hard problems of computer science, and it looks like that's the root of the problem here. Are you using Flask-Cache here, or does this bug show up with the FakeCache implementation?

Is there any chance you can write an automated test that identifies this bug? That will make it easier to fix it so that it stays fixed.

@NathanWailes
Copy link
Contributor Author

NathanWailes commented Oct 31, 2017

I don't think the problem is that the cache isn't being invalidated when it should be; I think it's that both the cache and the db are allowing an entry to be created when the user_id field of the OAuth table entry is None, and so then they aren't able to distinguish between a logged-in user and a logged-out user.

Honestly, what I would be inclined to do is to just raise an exception if either(? or both?) the user_id and the user parameters are None:

def get(self, blueprint, user=None, user_id=None):
    if not user and not user_id:
        raise ValueError("Either 'user' or 'user_id' must be specified.")
    (...)

def set(self, blueprint, token, user=None, user_id=None):
    if not user and not user_id:
        raise ValueError("Either 'user' or 'user_id' must be specified.")
    (...)

Or maybe if you don't want to have it raise an exception, have it raise a warning(?): "Warning: Creating an OAuth record without an associated user will make the token used by all users, even anonymous users." That's not the best way to say it, but it might be a helpful first draft.

I'm also not sure if it's really a bug as much as a design decision (what should happen in this situation?), and a weird situation I got myself into (having half the functionality created without the other half).

@singingwolfboy
Copy link
Owner

That doesn't allow for using Flask-Dance without a user account system. (See the first option in this comment.) Maybe this is a problem that needs to be solved with better documentation?

@NathanWailes
Copy link
Contributor Author

Could you maybe force the developer to explicitly declare whether they want the OAuth to be linked with a user account system or not? And then have the exceptions depend on that?

@singingwolfboy
Copy link
Owner

Can you suggest an API for doing so? Which is to say, imagine that this feature was already implemented, and write some code that uses this already-implemented feature. That will help me understand what you're suggesting.

@NathanWailes
Copy link
Contributor Author

NathanWailes commented Oct 31, 2017

Yeah, so right now my twitter_blueprint assignment in my app.py looks like this:

twitter_blueprint = make_twitter_blueprint(
    api_key=config['twitter']['api_key'],
    api_secret=config['twitter']['api_secret'],
    redirect_to="web_routes.lyrics_rhythm_recorder"
)

I'm suggesting just adding a required parameter, so it'd look like this:

twitter_blueprint = make_twitter_blueprint(
    link_tokens_to_user_accounts=True,
    api_key=config['twitter']['api_key'],
    api_secret=config['twitter']['api_secret'],
    redirect_to="web_routes.lyrics_rhythm_recorder"
)

@NathanWailes
Copy link
Contributor Author

NathanWailes commented Oct 31, 2017

Update:

Ok, two things:

  1. I just realized that because I want the exception-raisers to be in the sqla.py file, it might make more sense to have the parameter as part of the twitter_blueprint.SQLAlchemyBackend() initialization.

  2. I also realized that I already have information in the twitter_blueprint.SQLAlchemyBackend() which seems like it might be sufficient to handle this situation without needing an additional parameter: the user=current_user parameter.

Here's what my code looks like right now:

twitter_blueprint.backend = SQLAlchemyBackend(OAuth, db.session, user=current_user)

So it might be sufficient to simply look for whether that user parameter is None or not. I'm not positive, but I figured it was worth bringing up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants