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

Fix flask required decorator to redirect on expired credentials. #452

Merged
merged 1 commit into from Mar 11, 2016

Conversation

Projects
None yet
6 participants
@theacodes
Member

theacodes commented Mar 4, 2016

No description provided.

@googlebot googlebot added the cla: yes label Mar 4, 2016

@theacodes

This comment has been minimized.

Show comment
Hide comment
@theacodes

theacodes Mar 4, 2016

Member

/r @dhermes or @nathanielmanistaatgoogle

@elibixby ran into this in his application. Apparently I was mistaken in just checking credentials.invalid which doesn't actually get set to True until a request fails.

@waprin a similar change is probably need to the django helper.

Member

theacodes commented Mar 4, 2016

/r @dhermes or @nathanielmanistaatgoogle

@elibixby ran into this in his application. Apparently I was mistaken in just checking credentials.invalid which doesn't actually get set to True until a request fails.

@waprin a similar change is probably need to the django helper.

@waprin

This comment has been minimized.

Show comment
Hide comment
@waprin

waprin Mar 4, 2016

Contributor

acknowledged, I'll take a look at Django.

Contributor

waprin commented Mar 4, 2016

acknowledged, I'll take a look at Django.

@@ -443,7 +443,12 @@ def credentials(self):
def has_credentials(self):
"""Returns True if there are valid credentials for the current user."""
return self.credentials and not self.credentials.invalid

This comment has been minimized.

@dhermes

dhermes Mar 8, 2016

Contributor

Sidebar: If not for this use case, what is invalid useful for?

@dhermes

dhermes Mar 8, 2016

Contributor

Sidebar: If not for this use case, what is invalid useful for?

This comment has been minimized.

@elibixby

elibixby Mar 10, 2016

Contributor

Agreed, also surprisingly, invalid is not even set before a failed request even if there is no refresh token present. At the very least, the client can detect if the access token is expired, and there isn't a refresh token, and set invalid preemptively.

@elibixby

elibixby Mar 10, 2016

Contributor

Agreed, also surprisingly, invalid is not even set before a failed request even if there is no refresh token present. At the very least, the client can detect if the access token is expired, and there isn't a refresh token, and set invalid preemptively.

This comment has been minimized.

@theacodes

theacodes Mar 10, 2016

Member

AFAIT, invalid is more misleading than useful.

@theacodes

theacodes Mar 10, 2016

Member

AFAIT, invalid is more misleading than useful.

This comment has been minimized.

@dhermes

dhermes Mar 10, 2016

Contributor

@nathanielmanistaatgoogle Is there any way to do a survey of how and if people are using features like this?

@dhermes

dhermes Mar 10, 2016

Contributor

@nathanielmanistaatgoogle Is there any way to do a survey of how and if people are using features like this?

This comment has been minimized.

@nathanielmanistaatgoogle

nathanielmanistaatgoogle Mar 11, 2016

Contributor

I suspect it's basically impossible.

In case you needed more understanding of why I try so hard to minimize the public interface of a library released into the wild...

@nathanielmanistaatgoogle

nathanielmanistaatgoogle Mar 11, 2016

Contributor

I suspect it's basically impossible.

In case you needed more understanding of why I try so hard to minimize the public interface of a library released into the wild...

@theacodes

This comment has been minimized.

Show comment
Hide comment
@theacodes

theacodes Mar 10, 2016

Member

Thanks for the review, just FYI I won't be able to fix the few nits for a few days. Will ping when ready.

Member

theacodes commented Mar 10, 2016

Thanks for the review, just FYI I won't be able to fix the few nits for a few days. Will ping when ready.

@theacodes

This comment has been minimized.

Show comment
Hide comment
@theacodes

theacodes Mar 11, 2016

Member

Comments addressed.

Member

theacodes commented Mar 11, 2016

Comments addressed.

@dhermes

This comment has been minimized.

Show comment
Hide comment
@dhermes

dhermes Mar 11, 2016

Contributor

LoL Python 2.6. You need to swap out for unittest2 in tests.contrib.test_flask_util.

Contributor

dhermes commented Mar 11, 2016

LoL Python 2.6. You need to swap out for unittest2 in tests.contrib.test_flask_util.

@theacodes

This comment has been minimized.

Show comment
Hide comment
@theacodes

theacodes Mar 11, 2016

Member

LoL Python 2.6

ugh. Fixed.

Member

theacodes commented Mar 11, 2016

LoL Python 2.6

ugh. Fixed.

@theacodes

This comment has been minimized.

Show comment
Hide comment
@theacodes

theacodes Mar 11, 2016

Member

@nathanielmanistaatgoogle all of the instances self.assertTrue(y in z) have been changed to self.assertIn(y, z).

Member

theacodes commented Mar 11, 2016

@nathanielmanistaatgoogle all of the instances self.assertTrue(y in z) have been changed to self.assertIn(y, z).

@@ -443,7 +443,12 @@ def credentials(self):
def has_credentials(self):
"""Returns True if there are valid credentials for the current user."""
return self.credentials and not self.credentials.invalid
if not self.credentials:
return False

This comment has been minimized.

@nathanielmanistaatgoogle

nathanielmanistaatgoogle Mar 11, 2016

Contributor

How bad does this look as one boolean expression? Is it just

return (
    self.credentials and
    (not self.credentials.access_token_expired or self.credentials.refresh_token))

?

If that's too unattractive, please change

if <condition>:
  return False
if <other condition>:
  return False
return True

to

if <condition>:
  return False
elif <other condition>:
  return False
else:
  return True

.

@nathanielmanistaatgoogle

nathanielmanistaatgoogle Mar 11, 2016

Contributor

How bad does this look as one boolean expression? Is it just

return (
    self.credentials and
    (not self.credentials.access_token_expired or self.credentials.refresh_token))

?

If that's too unattractive, please change

if <condition>:
  return False
if <other condition>:
  return False
return True

to

if <condition>:
  return False
elif <other condition>:
  return False
else:
  return True

.

This comment has been minimized.

@theacodes

theacodes Mar 11, 2016

Member

Definitely clearer as two statements. I added a comment as well.

@theacodes

theacodes Mar 11, 2016

Member

Definitely clearer as two statements. I added a comment as well.

@nathanielmanistaatgoogle

This comment has been minimized.

Show comment
Hide comment
@nathanielmanistaatgoogle

nathanielmanistaatgoogle Mar 11, 2016

Contributor

Looks good; waiting on fresh test results.

Contributor

nathanielmanistaatgoogle commented Mar 11, 2016

Looks good; waiting on fresh test results.

nathanielmanistaatgoogle added a commit that referenced this pull request Mar 11, 2016

Merge pull request #452 from jonparrott/flask-util-expired-credentials
Fix flask required decorator to redirect on expired credentials.

@nathanielmanistaatgoogle nathanielmanistaatgoogle merged commit 20dcfe2 into googleapis:master Mar 11, 2016

2 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@theacodes

This comment has been minimized.

Show comment
Hide comment
@theacodes

theacodes Mar 11, 2016

Member

Thanks. :)

On Fri, Mar 11, 2016 at 1:53 PM Nathaniel Manista notifications@github.com
wrote:

Merged #452 #452.


Reply to this email directly or view it on GitHub
#452 (comment).

Member

theacodes commented Mar 11, 2016

Thanks. :)

On Fri, Mar 11, 2016 at 1:53 PM Nathaniel Manista notifications@github.com
wrote:

Merged #452 #452.


Reply to this email directly or view it on GitHub
#452 (comment).

@theacodes theacodes referenced this pull request Apr 15, 2016

Merged

Release v2.0.2. #501

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment