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

check oauth creds type using isinstance #1398

Merged
merged 2 commits into from Feb 1, 2024
Merged

Conversation

alifeee
Copy link
Collaborator

@alifeee alifeee commented Jan 31, 2024

closes #1390

Credentials object is renamed OAuthCredentials in v6.0.0, and is no longer Credentials type, so the type-check for "already logged in" was failing. See #1390 (comment) for more context.

It is worth noting that types were being checked with type(). This it not reccomended as per pylint.

If we had been using isinstance, this bug would not appear, as

if not type(creds) is Credentials: did not work with OAuthCredentials
if not isinstance(creds, Credentials): would have worked with OAuthCredentials as it is a subclass of Credentials

  • I do not know enough about oauth to add an adequate test so that this problem does not happen again
  • I do not know enough about oauth to know if this renaming of Credentials -> OAuthCredentials could have other problems in auth.py or elsewhere

Thus, the result of this change is only tested manually, by me, on my computer.... :/

Copy link
Collaborator

@lavigne958 lavigne958 left a comment

Choose a reason for hiding this comment

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

very good catch ! I believe what we need is betwenn what you found and what was originally here:

  • using isinstance instead of type
  • using the generic base class Credentials

gspread/auth.py Outdated Show resolved Hide resolved
@lavigne958
Copy link
Collaborator

closes #1390

Credentials object is renamed OAuthCredentials in v6.0.0, and is no longer Credentials type, so the type-check for "already logged in" was failing. See #1390 (comment) for more context.

It is worth noting that types were being checked with type(). This it not reccomended as per pylint.

If we had been using isinstance, this bug would not appear, as

if not type(creds) is Credentials: did not work with OAuthCredentials if not isinstance(creds, Credentials): would have worked with OAuthCredentials as it is a subclass of Credentials

  • I do not know enough about oauth to add an adequate test so that this problem does not happen again

Unfortunately we can't add tests on oauth because it requires your to open a browser to allow the access.
There are possible way to make it work... but it would take us a considerable amount of time that can be used elsewhere I suppose. We must remind ourselves to manually test it from time to time or when changing anything around it !

  • I do not know enough about oauth to know if this renaming of Credentials -> OAuthCredentials could have other problems in auth.py or elsewhere

as I recommended we should keep comparing to the base class so this won't be a problem.

Thus, the result of this change is only tested manually, by me, on my computer.... :/

@alifeee alifeee merged commit 0c8d18f into master Feb 1, 2024
10 checks passed
@alifeee alifeee deleted the fix/oauth-re-authentication branch February 1, 2024 00:26
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

Successfully merging this pull request may close these issues.

gspread 6.0.0 OAuth requires re-authentication every time.
2 participants