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

feat: adds ability to provide redirect uri #58

Merged
merged 5 commits into from
Jan 9, 2023

Conversation

chalmerlowe
Copy link
Collaborator

@chalmerlowe chalmerlowe commented Dec 8, 2022

WIP PR for discussion: aiming to provide the ability to include a redirect URI, client ID, and client secrets to facilitate the migration away from "out of band" (OOB) OAuth authentication.

NOTE: the OOB process is sunsetting in January so this update will be important to incorporate prior to then.

@tswast

See also changes in these repos:

@chalmerlowe chalmerlowe requested a review from tswast December 21, 2022 20:58
@chalmerlowe chalmerlowe self-assigned this Dec 21, 2022
@chalmerlowe chalmerlowe added the type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. label Dec 21, 2022
@chalmerlowe chalmerlowe marked this pull request as ready for review January 4, 2023 21:24
"auth_uri": GOOGLE_AUTH_URI,
"token_uri": GOOGLE_TOKEN_URI,
}
}

def run_webapp(self,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this might be slightly easier to follow if we rename "self" to "flow" or something similar. AFAIK, the name "self" is only a convention, and even if we're monkeypatching, I think a different name would be easier to follow.

Also, if we're passing in redirect_uri does this actually need to be a closure function? Could we move this to a private method of the auth module?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -263,7 +301,10 @@ def get_user_credentials(
if use_local_webserver:
credentials = _webserver.run_local_server(app_flow)
else:
credentials = app_flow.run_console()
flow.InstalledAppFlow.run_webapp = run_webapp
credentials = app_flow.run_webapp(redirect_uri=redirect_uri)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell, we only use public methods in run_webapp. I wonder if monkeypatching is actually necessary?

Suggested change
credentials = app_flow.run_webapp(redirect_uri=redirect_uri)
credentials = run_webapp(app_flow, redirect_uri=redirect_uri)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@tswast tswast merged commit 227e99d into pydata:main Jan 9, 2023
@tswast tswast mentioned this pull request Jan 9, 2023
@TiPPeX2
Copy link

TiPPeX2 commented Apr 9, 2023

@chalmerlowe, I got an email stating the deprecation scheduled onset May 1st.
Does this fixes(adding separate client/secret/uri) addition will work?

We are writing to let you know that using the OOB (Out of Band) OAuth authentication workflow with the BigQuery connector library for pandas (pandas-gbq) and auth helper library ([pydata-google-auth] will be disabled on May 1, 2023. At the onset, we’d like to apologize for any impact this issue may have caused on your organization, and will do our best to support you.

According to definition of OOB, it still an OOB activity and users will still need to copy/paste the token.
Am I correct?

What is OOB? OAuth [out-of-band (OOB)](https://developers.google.com/identity/protocols/oauth2/native-app#manual-copypaste), also referred to as the manual copy/paste option, is a legacy flow developed to support native clients which do not have a redirect URI to accept the credentials after a user approves an OAuth consent request. The OOB flow poses a remote phishing risk and clients must migrate to an alternative method to protect against this vulnerability.
From what I saw you already have a fix of supply a default(non localhost) redirect_uri(WEBAPPS_URI), doesn't it already fix the issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants