-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Changes to support Bitbucket OAuth2 #1850
Conversation
Changes most API calls to use APIv2 and relies on OAuth2 for sessions. Bitbucket's OAuth2 implementation expires tokens after 2 hours, so this also implements token refreshing on the general session creation. For now, this relies on a patched version of django-allauth. A new ticket should be created to track migrating django-allauth back to upstream once the PRs are merged in and released. Refs pennersr/django-allauth#1215 Refs pennersr/django-allauth#1212
2e364ba
to
a3ddeae
Compare
Move oauth syncing code to composed classes, instead of a nest of functions. Push common configuration and code to a base class. Repo/org models will start to refer to the SocialAccount they are connected through.
The current implementation split up code between syncing and creating objects in an awkward way. This way, all code particular to the service is contained in the service class. Adds migrations to fix remote org/repo models, moves `source` to `account` foreign key, allowing for more logical queries and cascading deletes from account deletion. Migrates source field data to the account field via lookup, drops source field, and also sets org slug to non-unique, as we don't perform lookups on org slugs.
@agjohnson good job, i was thinking about same kind of implementation using an abstract base class, which you implemented using a non-abstract
Making it abstract would make more sense to me, what do you think? |
import_github(user=User.objects.get(username=slug), sync=True) | ||
service = GitHubService.for_user(User.objects.get(username=slug)) | ||
if service is not None: | ||
service.sync(sync=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.sync(sync=True)
feels like a weird API. Are there times we'll call sync without syncing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't see any, this is brought over from before the refactor. Not sure what sync=false would ever be used for.
@saily interesting, I almost settled on a similar pattern. In the end, I kept the pattern here simple, as we have some complications with inheritance for readthedocs.com. There's a good deal of common functionality implemented in the base class, so I think it's fair to consider services as subclasses of this class anyways. |
Adds a warning to the listing if any of the accounts connected for the | ||
user are not supported accounts. | ||
''' | ||
deprecated_accounts = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do these SocialAccount's get removed when they set up their new account? Or are we expecting them to "unlink" them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it requires manually unlinking them in the connections page. Or, rather, both can exist, we just need the Bitbucket OAuth2 provider. So as long as the user sets that up, everything will work -- albeit there will be a warning still. I considered making a view that unlinks the account and redirects to django-allauth, but felt that was over-complicating the issue.
LGTM 👍 -- definitely happy to have this stuff sanely refactored. |
Changes to support Bitbucket OAuth2
Changes most API calls to use APIv2 and relies on OAuth2 for sessions. Bitbucket's OAuth2
implementation expires tokens after 2 hours, so this also implements token refreshing on
the general session creation.
This updates the UI around connecting accounts, namely:
The project import page will now warn when deprecated accounts are connected, the new API sync code for Bitbucket uses version 2 of the API for almost all requests. Our API will filter out repos/orgs from these deprecated services, but the services themselves are kept around to allow for user administration on the connected services page.
Refs pennersr/django-allauth#1215
Refs pennersr/django-allauth#1212
Fixes #1675
I believe this fixes #1864
Fixes #1696
Refs #1893 - repin django-allauth next release
Fixes #1684 - "social accounts" rename
Fixes #1668 - social account page 500 error
Fixes #1446 - add bitbucket tests
Outstanding: