feat(integrations): github enterprise support by MaxBittker · Pull Request #8348 · getsentry/sentry

MaxBittker · 2018-05-08T00:43:33Z

https://paper.dropbox.com/doc/Github-Enterprise-Development-Guide-KUZuzK9mVLv2NmHZKcJ7P

benvinegar · 2018-05-08T01:02:40Z

In keeping with our Python filename conventions, should we not rename the githubEnterprise folder to github_enterprise?

mattrobenolt · 2018-05-08T02:32:30Z

In keeping with our Python filename conventions, should we not rename the githubEnterprise folder to github_enterprise?

Yes. And don't use capital letters in filenames.

MaxBittker · 2018-05-10T22:57:53Z

    allow_redirects=False,
    timeout=30,
-    verify_ssl=True,
+    verify_ssl=False,


these are in here temporarily because the github enterprise test instance doesn't have a valid cert

Let's not forget to turn this off, or at least make it a configuration of the integration or something.

👀

This should absolutely be configurable in the integration. It's likely that more than 0 GH enterprise installs use an non-standard cert. Whether it's an internally trusted cert, self signed, something that we can't validate.

Yeah, it will have to be a toggle on in the configuration form.

MaxBittker · 2018-05-10T22:58:40Z

+        if self.oauth_access_token_url is not '':
+            return self.oauth_access_token_url
+
+        # # check the model for instalation information


these commented out blocks will be used later when the installation flow has completed successfully

MaxBittker · 2018-05-10T22:59:16Z

        return [
-            OAuth2LoginView(
-                authorize_url=self.oauth_authorize_url,
+            lambda: OAuth2LoginView(


this was needed to be postponed because we can't evaluate the urls until earlier parts of the flow have completed

MaxBittker · 2018-05-10T22:59:45Z

+        installation_data = state['installation_data']
+        # user = get_user_info(installation_data['url'], identity['access_token'])
+        # this doesn't work yet (404s):
+        installation = self.get_installation_info(


this is the main issue right now.

I think you were maybe using the wrong id?

macqueen · 2018-05-17T02:02:23Z

        self.provider_model = provider_model

        self.config = config
+        self.provider.set_pipeline(self)


this is definitely why the tests are failing. @MeredithAnya tomorrow maybe you can try removing this change and seeing what breaks for GHE?

and/or @evanpurkhiser do you by chance have any context into why this change would be necessary?

This is for sure needed so that the provider is able to lookup the OAuth configuration from the parent pipleine (available on the executing pipeline). I'll take a look at tests.

Should be fixed. The tests overrode the variable this assigned to.

evanpurkhiser · 2018-05-17T21:53:32Z

    allow_redirects=False,
    timeout=30,
-    verify_ssl=True,
+    verify_ssl=False,


Let's not forget to turn this off, or at least make it a configuration of the integration or something.

evanpurkhiser · 2018-05-17T21:54:43Z

        self.config = config
        self.logger = logging.getLogger('sentry.identity.%s'.format(self.key))

-    def get_pipeline(self):


Can you pull this into a separate PR. This is pretty unrelated and just about removing a totally unused abstract method.

evanpurkhiser · 2018-05-17T21:57:01Z

+from sentry.identity.oauth2 import OAuth2Provider
+
+
+def get_user_info(url, access_token):


maybe we can have this reused in the github integration? Definitely some duplication here. Or maybe this can get sucked into the github client.

@macqueen thoughts here? I could also put it in github/utils with the jwt stuff if that makes sense

punting on this.

evanpurkhiser · 2018-05-17T21:57:57Z

+    return resp
+
+
+class GitHubEnterpriseIdentityProvider(OAuth2Provider):


Can this just extend GithubIdentityProvider and override the key and name?

the GitHubIdentityProvider also has other methods and whatnot that the GHE doesn't need so wouldn't you just end up needing to overriding those too?

Ah okay, that makes sense. This is fine.

evanpurkhiser · 2018-05-17T21:58:24Z

+    def build_identity(self, data):
+        data = data['data']
+
+        user = get_user_info(data['access_token'])


This call is wrong, doesn't provide the url.

evanpurkhiser · 2018-05-17T22:45:32Z

+                GitHubEnterpriseInstallationRedirect(),
+                identity_pipeline_view]
+
+    def get_installation_info(self, installation_data, access_token, installation_id):


It would be nice for this to be abstracted between the two github integrations.

evanpurkhiser · 2018-05-17T22:46:32Z

+logger = logging.getLogger('sentry.integrations.github-enterprise')
+
+
+class GitHubEnterpriseAppsEndpoint(Endpoint):


Are these webhooks going to be so different that we have to use an entirely different endpoint?

evanpurkhiser · 2018-05-17T22:56:40Z

+            'redirect_url': absolute_uri('/extensions/github-enterprise/setup/'),
+        }
+
+        identity_pipeline_view = NestedPipelineView(


Okay so I realized, because we now allow for late binding of pipeline steps, we can actually do a better job providing the oauth parameters to the identity pipeline through it's config object, versus it expecting to find it in a parent pipeline. How about this

If we have a helper method to construct the identity pipeline view

def _make_identity_pipeline_view(self): """ Make the nested identity provider view. It is important that this view is not constructed until we reach this step and the ``oauth_config_information`` is available in the pipeline state. This method should be late bound into the pipeline vies. """ identity_pipeline_config = dict( oauth_scopes=(), redirect_url=absolute_uri('/extensions/github-enterprise/setup/'), *self.pipeline.fetch_state('oauth_config_information'), ) return NestedPipelineView( bind_key='identity', provider_key='github-enterprise', pipeline_cls=IdentityProviderPipeline, config=identity_pipeline_config, )

Then in the get_pipeline_views

return [ InstallationConfigView(), GitHubEnterpriseInstallationRedirect(), # The identity provider pipeline should be constructed at execution # time, this allows for the oauth configuration parameters to be made # available from the installation config view. lambda: self._make_identity_pipeline_view(), ]

evanpurkhiser · 2018-05-17T23:03:16Z


+    def set_pipeline(self, pipeline):
+        """
+        todo(maxbittker)


This should probably say something along the lines of

Used by the pipeline to give the provider access to the executing pipeline.

evanpurkhiser · 2018-05-17T23:03:35Z

        self.provider_model = provider_model

        self.config = config
+        self.provider.set_pipeline(self)


Should be fixed. The tests overrode the variable this assigned to.

evanpurkhiser

A couple minor nits, but looks awesome. Great work @MaxBittker and @MeredithAnya!

evanpurkhiser · 2018-05-21T20:58:59Z

+        if model and model.config.get(parameter_name) is not None:
+            return model.config.get(parameter_name)
+
+        raise KeyError


Could you give this a message like

KeyError(u'Unable to resolve OAuth parameter "{}"'.format(parameter_name))

evanpurkhiser · 2018-05-21T20:59:46Z

        return [
-            OAuth2LoginView(
-                authorize_url=self.oauth_authorize_url,
+            lambda: OAuth2LoginView(


You can switch these back to being initialized at class construction time.

adhiraj · 2018-05-24T04:50:32Z

+                defaults={'config': identity_config},
            )

+            if created:


@MeredithAnya should this be if not created

MaxBittker added the WIP label May 8, 2018

MaxBittker force-pushed the feat/github-enterprise branch from 6d18402 to 1f8fc12 Compare May 9, 2018 17:49

MaxBittker added 7 commits May 10, 2018 10:37

wip

6e5afa8

wip

311341d

lambsa

6e8882f

fix import

17575fc

wip

a2441b6

remove unused method

3eccac1

progress

7c6a34b

MaxBittker force-pushed the feat/github-enterprise branch from 21f4ff2 to 7c6a34b Compare May 10, 2018 17:38

MaxBittker added 3 commits May 10, 2018 10:53

fix lint

d772531

fix lint

7180b96

comment

64c53a4

MaxBittker commented May 10, 2018

View reviewed changes

logging strings

8b97ef9

macqueen assigned MeredithAnya May 14, 2018

MeredithAnya and others added 6 commits May 14, 2018 17:33

use github identifier for jwt

716960a

add user indentity back in

ebfc141

add identity provider config

5cadb2a

minor fix

5120514

add test

da09286

Merge branch 'master' into feat/github-enterprise

ecf433b

MeredithAnya requested review from evanpurkhiser and macqueen May 16, 2018 23:36

macqueen reviewed May 17, 2018

View reviewed changes

MeredithAnya added 2 commits May 17, 2018 10:59

fix failing test

99624c7

rename to pipeline_views for clarity

52d88eb

MeredithAnya mentioned this pull request May 17, 2018

rename to pipeline_views for clarity #8480

Merged

revert to 99624c7

f618560

evanpurkhiser requested changes May 17, 2018

View reviewed changes

MeredithAnya added 2 commits May 18, 2018 14:05

refactor get_jwt

9169ed1

dont remove get_pipeline in this pr

805f000

MeredithAnya mentioned this pull request May 18, 2018

feat(integrations): remove unused 'get_pipeline' #8497

Merged

MeredithAnya added 13 commits May 18, 2018 16:11

add _get_oauth_parameter helper

be3261e

modify helper

f6dce89

remove private key from oauth config

6cb484e

- to _

2ef77b7

fix source_url

c1df321

update description

2861c89

formatting and refactoring

ee9ce63

verify_ssl urlopen config option

f7c1e4d

move fields to class properties

1e67394

urlopen_options

cc1fb60

explicitly set verify_ssl

303396b

fix test

940d2c1

add GHE to internal integrations

a7e2701

evanpurkhiser approved these changes May 21, 2018

View reviewed changes

add error message

0a66f88

MeredithAnya merged commit f30320b into master May 22, 2018

MeredithAnya deleted the feat/github-enterprise branch May 22, 2018 16:07

adhiraj reviewed May 24, 2018

View reviewed changes

zetaab mentioned this pull request Oct 7, 2019

Can't setup github enterprise integration on a sentry 9.1.0 #12765

Closed

3 tasks

github-actions Bot locked and limited conversation to collaborators Dec 21, 2020

		from sentry.identity.oauth2 import OAuth2Provider


		def get_user_info(url, access_token):

		return resp


		class GitHubEnterpriseIdentityProvider(OAuth2Provider):

		logger = logging.getLogger('sentry.integrations.github-enterprise')


		class GitHubEnterpriseAppsEndpoint(Endpoint):

Uh oh!

Conversation

MaxBittker commented May 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benvinegar commented May 8, 2018

Uh oh!

mattrobenolt commented May 8, 2018

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!

MeredithAnya May 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

MeredithAnya May 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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 May 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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 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!

Reviewers

Assignees

Labels

Projects

MaxBittker commented May 8, 2018 •

edited

Loading

MeredithAnya May 15, 2018 •

edited

Loading

MeredithAnya May 18, 2018 •

edited

Loading

evanpurkhiser May 17, 2018 •

edited

Loading