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 django authorization redirect by correctly checking validity of credentials #651

Merged
merged 1 commit into from Sep 16, 2016

Conversation

Projects
None yet
4 participants
@waprin
Contributor

waprin commented Sep 16, 2016

I was checking for credentials but I need to use the has_credentials method for cases of existing but invalid credentials.

@googlebot googlebot added the cla: yes label Sep 16, 2016

@theacodes

This comment has been minimized.

Show comment
Hide comment
@theacodes

theacodes Sep 16, 2016

Member

@waprin looks like we have a coverage miss:

oauth2client/contrib/django_util/views.py              73      1     14      1    98%   187, 186->187

Can you fix?

Member

theacodes commented Sep 16, 2016

@waprin looks like we have a coverage miss:

oauth2client/contrib/django_util/views.py              73      1     14      1    98%   187, 186->187

Can you fix?

@theacodes theacodes self-assigned this Sep 16, 2016

@theacodes theacodes changed the title from Fix bug by correctly checking validity of credentials to Fix django authorization redirect by correctly checking validity of credentials Sep 16, 2016

@waprin

This comment has been minimized.

Show comment
Hide comment
@waprin

waprin Sep 16, 2016

Contributor

Yep fixing now

Contributor

waprin commented Sep 16, 2016

Yep fixing now

@waprin

This comment has been minimized.

Show comment
Hide comment
@waprin

waprin Sep 16, 2016

Contributor

@jonparrott Looks like all my tests pass and django_util now has 100% coverage, failing on cover because of other code I think?

Contributor

waprin commented Sep 16, 2016

@jonparrott Looks like all my tests pass and django_util now has 100% coverage, failing on cover because of other code I think?

@waprin

This comment has been minimized.

Show comment
Hide comment
@waprin

waprin Sep 16, 2016

Contributor

Ah never mind, now I see still some missing lines in the tests themselves

Contributor

waprin commented Sep 16, 2016

Ah never mind, now I see still some missing lines in the tests themselves

@theacodes theacodes merged commit 8a6e3b2 into googleapis:master Sep 16, 2016

3 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 100.0%
Details
@@ -170,21 +170,22 @@ def oauth2_authorize(request):
A redirect to Google OAuth2 Authorization.
"""
return_url = request.GET.get('return_url', None)
if not return_url:

This comment has been minimized.

@nathanielmanistaatgoogle

nathanielmanistaatgoogle Sep 17, 2016

Contributor

Prefer is None/is not None when comparing to the None object.

If request.GET.get('return_url', None) is the empty string, that's a bug, right? And by unintentionally sweeping it along the same code path as request.GET-does-not-contain-a-key-value-mapping-for-key-return_url we make finding and fixing the bug harder, right?

@nathanielmanistaatgoogle

nathanielmanistaatgoogle Sep 17, 2016

Contributor

Prefer is None/is not None when comparing to the None object.

If request.GET.get('return_url', None) is the empty string, that's a bug, right? And by unintentionally sweeping it along the same code path as request.GET-does-not-contain-a-key-value-mapping-for-key-return_url we make finding and fixing the bug harder, right?

@waprin waprin referenced this pull request Sep 20, 2016

Merged

Django samples #636

@pferate pferate referenced this pull request Oct 14, 2016

Closed

return_url might be None #640

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