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

Nylas auth refactor #27

Merged
merged 20 commits into from
Sep 22, 2020
Merged

Nylas auth refactor #27

merged 20 commits into from
Sep 22, 2020

Conversation

thomasst
Copy link
Member

@thomasst thomasst commented Sep 9, 2020

  • Refactored and simplified auth handlers (inbox/auth/*)
    • Supported providers are "custom", "gmail", "outlook", no more complex module registering logic.
    • Only one token per account (we're assuming the user doesn't have different tokens for email, calendar, contacts)
    • Get rid of GmailAuthCredentials table. We only support one app token.
    • By default store the client ID on the account table to still allow migrating accounts from one client ID to another (This would require some minor work to feed multiple client secrets via config).
    • Kept the verification methods due to many tests using them, but we aren't using them so far.
  • API (inbox/api/srv.py) and auth handler now uses attr objects instead of a dict. Supports both token and AuthAlligator auth info.
  • Single entry point to obtain an access token via existing TokenManager which can use a refresh token or AuthAlligator (not implemented yet).
    • Email uses get_authenticated_imap_connection for convenience.
    • Events uses existing requests client with token from the token manager.
    • Contacts uses existing Google library but doesn't touch the refresh token since it uses gdata.gauth.AuthSubToken with the token from the token manager.

To do:

  • DB Migration (may be in a subsequent PR so it can be deployed later)
  • Fix tests
  • More testing. Test marking accounts as invalid if token is invalid vs getting a new token.
  • Actual AuthAlligator integration (may be in a subsequent PR)

Future to-dos:

  • Upgrade SQLAlchemy and use the Enum type properly.
  • Automatically expunge objects from sessions so we can shorten SQLAlchemy sessions.

@thomasst thomasst self-assigned this Sep 9, 2020
Copy link
Contributor

@AlecRosenbaum AlecRosenbaum left a comment

Choose a reason for hiding this comment

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

I don't see anything here really objectionable, but the size of this diff makes me nervous.

Beyond automated tests, are we going to do any more testing internally before merging this?

inbox/auth/base.py Show resolved Hide resolved
inbox/auth/base.py Show resolved Hide resolved
inbox/auth/generic.py Outdated Show resolved Hide resolved
inbox/auth/generic.py Outdated Show resolved Hide resolved
@thomasst thomasst force-pushed the auth-refactor branch 3 times, most recently from 2cb5e53 to aae53db Compare September 15, 2020 11:35
@thomasst
Copy link
Member Author

@AlecRosenbaum Take another look please. The tests should now pass and I did some more local testing. I will still do further testing this week.

@thomasst thomasst changed the title WIP auth refactor Nylas auth refactor Sep 17, 2020
Copy link
Contributor

@AlecRosenbaum AlecRosenbaum left a comment

Choose a reason for hiding this comment

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

There's a lot of stuff going on here so hopefully I didn't miss anything too major.

If possible we should try to add rollbar support before we test this so error response is more proactive than reactive.

"email", # email address
"https://mail.google.com/", # email
"https://www.google.com/m8/feeds", # contacts
"https://www.googleapis.com/auth/calendar", # calendar
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need any of the profile information? And they're not reusing the G+ scope for other profile information?

Copy link
Member Author

Choose a reason for hiding this comment

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

The profile info was filled in the Gmail account table but is not needed for the sync, so I took it out.

tests/auth/test_generic_auth.py Outdated Show resolved Hide resolved
tests/auth/test_generic_auth.py Show resolved Hide resolved
tests/auth/test_generic_auth.py Show resolved Hide resolved
@AlecRosenbaum
Copy link
Contributor

I also definitely think we should merge this separate from actual AuthAlligator integration, primarily to get a baseline that this works and to make it easier to review the more authalligator-specific changes (beyond SecretType + company).

@thomasst thomasst merged commit 16cbcc1 into master Sep 22, 2020
@thomasst thomasst mentioned this pull request Sep 22, 2020
3 tasks
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.

2 participants