Integrations Identity pipeline refactoring #6708

evanpurkhiser · 2017-12-07T02:20:26Z

Probably easiest to review the 4 commits individually.

This is a large refactoring of the v3 Integration setup pipeline to use a new style of user identity lookup, based on a new identity module, which provides a pipeline + identity providers.

This is the foundation for unifying the SSO Authentication identity, integrations identity, and (at some point probably in the far future) plugin identity (likely when they're moved to new-style integrations).

This implements the following:

The identity module, which provides an Identity pipeline, with a generic identity association finish_pipeline implementation. This may be used both directly as part of an identity association endpoint, as well as made part of a pipeline through the pipleline.NestedProviderView. A generic base OAuth2 provider is also implemented, which is mostly a copy and paste from both the auth module and integration modules implementations (which this will replace).
The OAuth identity provider supports scope customization through the pipeline config object.
The models.identity module has been updated with models that more accurately reflect how Identity is stored.
- One identity per user
- IdentityProvider is associated to an organization, allowing lookup of identity providers configured for an organization. Originally the idea was to have one identity provider per 'instance' of the identity provider, for example there'd be one Slack IDP since there's only one Sentry app on slack. However, with this implementation we'd stil need some way to know it's configured for an organization, and that linking table would likely take just as much space. Further, having a direct association to an identity provider makes generic identity association possible.
- Also Important to note is that these tables were not yet in use outside of the feature-flagged v3 integrations, so there are two migrations here, the first simply removes the tables, the second re-creates the tables using the updated model fields.
The Integrations base object has it's id class attribute renamed to key. This is a little more clear as it differentiates it from the provider models database id.
Integrations now compose the slack identity provider pipeline to do setup and store the setup users identity.

ghost · 2017-12-07T02:22:47Z

	3 Warnings
⚠️	Changes require @getsentry/security sign-off
⚠️	You should update CHANGES due to the size of this PR
⚠️	PR includes migrations

Security concerns found

src/sentry/{integrations/oauth.py => identity/oauth2.py}

Migration Checklist

new columns need to be nullable (unless table is new)
migration with any new index needs to be done concurrently
data migrations should not be done inside a transaction
before merging, check to make sure there aren't conflicting migration ids

Generated by 🚫 danger

evanpurkhiser · 2017-12-13T20:53:27Z

Assigning @dcramer since I know he knows this part of the code, but I know he's busy, so also assigning @alex-hofsteede for that knowledge share.

alex-hofsteede · 2017-12-18T22:37:34Z

src/sentry/identity/base.py

+from sentry.utils.pipeline import PipelineProvider
+
+
+class MigratingIdentityId(namedtuple('MigratingIdentityId', ['id', 'legacy_id'])):


Ha, haven't seen this trick before. I'm guessing it's just to avoid a bunch of self.foo = foo boilerplate?

Yeah, I actually introduced this over here #6577, but am moving it into the identity module, and it will be removed from the auth provider later once the auth providers make use of the identity provider.

alex-hofsteede · 2017-12-18T22:40:47Z

src/sentry/identity/manager.py

+
+class IdentityManager(object):
+    def __init__(self):
+        self.__values = {}


If the values are assumed to be providers, then can we call this __providers ?

This manager was just copied over from other provider managers. The auth module and integration module both have one of these. At some point we'll likely want to just provide some general lookup manager that can be used to this fashion. It doesn't have anything specific to identity providers.

alex-hofsteede · 2017-12-18T23:14:05Z

src/sentry/identity/__init__.py

 register = default_manager.register
 unregister = default_manager.unregister
+
+# TODO(epurkhiser): needs to be initialized some other way


does this need to be addressed before merge?

I've updated the comment for this, but basically the idea is that this registering of the provider should happen with the slack plugin most likely.

alex-hofsteede · 2017-12-18T23:15:22Z

src/sentry/integrations/slack/integration.py

-    oauth_access_token_url = 'https://slack.com/api/oauth.access'
-    oauth_authorize_url = 'https://slack.com/oauth/authorize'
-    oauth_scopes = tuple(sorted((
+    # TODO: Remove id and use key everywhere for integrations


this was done right?

Yeah, updated. good catch.

alex-hofsteede · 2017-12-18T23:17:50Z

tests/sentry/web/frontend/test_integration_setup.py

        assert resp.status_code == 200
        self.assertDialogSuccess(resp)

-        integration = Integration.objects.get(provider=self.provider.id)


Do we need any more tests aside from the ones refactored here?

I have some identity linking acceptance tests that I need to finish up, I'll try and get those in here, but I'd like to get this merged in sooner than later, I'll try and get those in, but otherwise they'll come in as a separate PR I think.

Other than that more tests will show up as we solidify more of what integration setup looks like from a configuration standpoint

alex-hofsteede

LGTM

mattrobenolt

This migration is a blocker. Let's chat IRL to come up with the plan.

mattrobenolt · 2018-01-02T19:06:32Z

src/sentry/identity/slack/provider.py

+from sentry import options
+from sentry.identity.oauth2 import OAuth2Provider
+
+options.register('slack.client-id')


Set flags=FLAG_PRIORITIZE_DISK at least on these since I assume we'll be hardcoding these for us in config.

Yeah these will be hardcoded in the configs. Updated here.

mattrobenolt · 2018-01-02T19:07:00Z

src/sentry/integrations/pipeline.py

+
+__all__ = ['IntegrationPipeline']
+
+import json


Use sentry.utils.json instead unless there was a reason not to.

Updated here.

mattrobenolt · 2018-01-02T19:10:36Z

src/sentry/models/identity.py

        unique_together = (('idp', 'external_id'),)
-
-
-class UserIdentity(Model):


Let's not actually drop this yet until a later migration.

Addressed in this comment

mattrobenolt · 2018-01-02T19:10:55Z

src/sentry/models/identity.py

    __core__ = False

    idp = FlexibleForeignKey('sentry.IdentityProvider')
+    user = FlexibleForeignKey(settings.AUTH_USER_MODEL)


This needs to be nullable.

Addressed in this comment

mattrobenolt · 2018-01-02T19:11:19Z

src/sentry/models/identity.py


    type = models.CharField(max_length=64)
-    instance = models.CharField(max_length=64)
+    organization = FlexibleForeignKey('sentry.Organization')


Ok, this migration is going to be problematic.

We need to come up with a transition plan. We can't drop and add these new columns at the same time, nor can we simply edit this unique_together, or add not nullable columns.

Addressed in this comment

evanpurkhiser · 2018-01-02T19:34:33Z

Talked IRL with @mattrobenolt. I've updated the description of the PR to note that the Identity models where not yet used anywhere outside of the V3 integration work, so the changes that were made to these tables can be safely be applied by dropping and recreating these tables as is done n the migrations here.

I will still however need to update the IDs of the migrations.

evanpurkhiser · 2018-01-03T23:07:41Z

@mattrobenolt: bumped my migrations.

Refactors integrations to compose the identity provider found in the sentry.identity module.

evanpurkhiser · 2018-01-08T21:25:00Z

Had to bump the migrations one more time because of #6147. Just need your sign off @mattrobenolt.

mattrobenolt

Punch it.

evanpurkhiser added the WIP label Dec 7, 2017

evanpurkhiser force-pushed the refintegrations-composing-the-identity-pipeline branch 10 times, most recently from 1917af7 to a46346d Compare December 13, 2017 19:44

evanpurkhiser requested review from alex-hofsteede and dcramer December 13, 2017 20:52

evanpurkhiser added Component: Integrations and removed WIP labels Dec 13, 2017

alex-hofsteede reviewed Dec 18, 2017

View reviewed changes

evanpurkhiser force-pushed the refintegrations-composing-the-identity-pipeline branch 2 times, most recently from 582be30 to 8abad79 Compare December 21, 2017 19:23

evanpurkhiser requested a review from mattrobenolt January 2, 2018 18:05

alex-hofsteede approved these changes Jan 2, 2018

View reviewed changes

mattrobenolt suggested changes Jan 2, 2018

View reviewed changes

evanpurkhiser force-pushed the refintegrations-composing-the-identity-pipeline branch 2 times, most recently from c4f666c to 448cc71 Compare January 3, 2018 23:04

evanpurkhiser force-pushed the refintegrations-composing-the-identity-pipeline branch 2 times, most recently from e762f87 to bad62a0 Compare January 4, 2018 22:28

ref: Swap (most) usage of integration 'id' to 'key'

35d7f53

evanpurkhiser added 3 commits January 8, 2018 13:21

ref(identity): Refactor models

a6a9ae4

feat: Implement generic identity association pipeline

fd9f184

ref(integrations): Composing the identity pipeline

2fd8285

Refactors integrations to compose the identity provider found in the sentry.identity module.

evanpurkhiser force-pushed the refintegrations-composing-the-identity-pipeline branch from bad62a0 to 2fd8285 Compare January 8, 2018 21:21

mattrobenolt added the Impact: Migration label Jan 8, 2018

mattrobenolt approved these changes Jan 8, 2018

View reviewed changes

evanpurkhiser merged commit b8cf1e2 into master Jan 8, 2018

evanpurkhiser deleted the refintegrations-composing-the-identity-pipeline branch January 8, 2018 22:09

github-actions bot locked and limited conversation to collaborators Dec 22, 2020

		from sentry.utils.pipeline import PipelineProvider


		class MigratingIdentityId(namedtuple('MigratingIdentityId', ['id', 'legacy_id'])):

		unique_together = (('idp', 'external_id'),)


		class UserIdentity(Model):

Uh oh!

Integrations Identity pipeline refactoring #6708

Integrations Identity pipeline refactoring #6708

Uh oh!

Conversation

evanpurkhiser commented Dec 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Dec 7, 2017 • edited by evanpurkhiser Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Security concerns found

Migration Checklist

Uh oh!

evanpurkhiser commented Dec 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alex-hofsteede left a comment

Choose a reason for hiding this comment

Uh oh!

mattrobenolt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evanpurkhiser commented Jan 2, 2018

Uh oh!

evanpurkhiser commented Jan 3, 2018

Uh oh!

evanpurkhiser commented Jan 8, 2018

Uh oh!

mattrobenolt left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

evanpurkhiser commented Dec 7, 2017 •

edited

Loading

ghost commented Dec 7, 2017 •

edited by evanpurkhiser

Loading