feat(integrations): Vsts OAuth refresh #8725

lauryndbrown · 2018-06-13T00:02:02Z

No description provided.

lauryndbrown · 2018-06-13T00:02:58Z

src/sentry/identity/oauth2.py

@@ -64,6 +65,9 @@ def _get_oauth_parameter(self, parameter_name):
    def get_oauth_access_token_url(self):
        return self._get_oauth_parameter('access_token_url')

+    def get_oauth_refresh_token_url(self):


Had anticipated moving things in here, but couldn't figure out a clean way of doing so. Open to suggestions, otherwise I'll just remove this code.

Did you see what we did in the auth version of the Oauth2 provider?

sentry/src/sentry/auth/providers/oauth2.py

Lines 181 to 216 in f683f22

def refresh_identity(self, auth_identity):

refresh_token = auth_identity.data.get('refresh_token')

if not refresh_token:

raise IdentityNotValid('Missing refresh token')

data = self.get_refresh_token_params(

refresh_token=refresh_token,

)

req = safe_urlopen(self.get_refresh_token_url(), data=data)

try:

body = safe_urlread(req)

payload = json.loads(body)

except Exception:

payload = {}

error = payload.get('error', 'unknown_error')

error_description = payload.get('error_description', 'no description available')

formatted_error = 'HTTP {} ({}): {}'.format(req.status_code, error, error_description)

if req.status_code == 401:

raise IdentityNotValid(formatted_error)

if req.status_code == 400:

# this may not be common, but at the very least Google will return

# an invalid grant when a user is suspended

if error == 'invalid_grant':

raise IdentityNotValid(formatted_error)

if req.status_code != 200:

raise Exception(formatted_error)

auth_identity.data.update(self.get_oauth_data(payload))

auth_identity.update(data=auth_identity.data)

We can probably do something really similar here and whatever needs to call refresh can do `identity.get_provider().refresh_identity(identity)

lauryndbrown · 2018-06-15T02:12:42Z

src/sentry/identity/oauth2.py

+                raise IdentityNotValid(formatted_error)
+
+        if req.status_code != 200:
+            raise Exception(formatted_error)


The error handling is the same code from the old oauth code. I was wondering if that was still the desired behavior here?

Yeah this is fine I think, we probably want to know what the error is

lauryndbrown · 2018-06-15T02:13:42Z

src/sentry/integrations/client.py

+        super(OAuth2ApiClient, self).__init__(*args, **kwargs)
+        self.identity = identity
+
+    def check_auth(self):


This is small, but I sort of wish I had a different name for this. Open to suggestions.

The name sounds good to me actually

lauryndbrown · 2018-06-15T02:15:36Z

src/sentry/integrations/vsts/client.py


    def request(self, method, path, data=None, params=None):
+        self.check_auth()


I really wanted to put this in the OAuth2ApiClient class, but I didn't also want to have to go back and alter the headers/url/whatever the subclass using the old identity did.So, I felt this was the best option. Open to suggestions.

This looks good to me :)

evanpurkhiser · 2018-06-18T19:25:18Z

src/sentry/identity/oauth2.py

@@ -102,6 +119,48 @@ def get_oauth_data(self, payload):

        return data

+    def refresh_identity(self, auth_identity):


I think I'd change the parameter name from auth_identity to just identity, since where this code was previously was specific to auth identities (which are still a separate thing)

evanpurkhiser · 2018-06-18T19:26:15Z

src/sentry/identity/oauth2.py

+        if not refresh_token:
+            raise IdentityNotValid('Missing refresh token')
+
+        data = self.get_refresh_token_params(


nit: Does this need to be on multiple lines?

evanpurkhiser · 2018-06-18T19:27:21Z

src/sentry/identity/oauth2.py

@@ -76,6 +80,12 @@ def get_oauth_client_secret(self):
    def get_oauth_scopes(self):
        return self.config.get('oauth_scopes', self.oauth_scopes)

+    def get_oauth_refresh_token(self):
+        return self.config.get('refresh_token')


This seems a little strange, why would the refresh token be something that comes from the configuration object that's passed to the provider?

I'm not sure if this is used anywhere, maybe it's just some code that didn't get removed?

Good catch. I'll remove it.

evanpurkhiser · 2018-06-18T19:34:21Z

src/sentry/identity/oauth2.py

+                raise IdentityNotValid(formatted_error)
+
+        if req.status_code != 200:
+            raise Exception(formatted_error)


Yeah this is fine I think, we probably want to know what the error is

evanpurkhiser · 2018-06-18T19:42:13Z

src/sentry/identity/vsts/provider.py

@@ -16,7 +17,7 @@ class VSTSIdentityProvider(OAuth2Provider):

    oauth_access_token_url = 'https://app.vssps.visualstudio.com/oauth2/token'
    oauth_authorize_url = 'https://app.vssps.visualstudio.com/oauth2/authorize'
-
+    oauth_redirect_url = '/extensions/vsts/setup/'


The identity provider has redirect URLs abstracted from it, I think this is breaking that abstraction. For example you can see her how it works

sentry/src/sentry/identity/oauth2.py

Line 144 in ea1b647

redirect_uri=absolute_uri(pipeline.redirect_url()),

However in this case we have access to the pipeline from the dispatched method.

Reading further though I'd like to understand more why we actually need this, checkout some of my following comments.

evanpurkhiser · 2018-06-18T19:43:59Z

src/sentry/identity/vsts/provider.py

+            'client_assertion': self.get_oauth_client_secret(),
+            'grant_type': 'refresh_token',
+            'assertion': refresh_token,
+            'redirect_uri': absolute_uri(self.oauth_redirect_url),


I'm not sure I understand why there's this redirect URL? Doesn't this call happen from our server?

So @evanpurkhiser it's a VSTS thing. If you check out the docs the redirect URL is used as a check. The redirect URL is required to match the redirect url that was registered within the app, so passing it is required. This is true even in the case you're refreshing the token.

More here: https://docs.microsoft.com/en-us/vsts/integrate/get-started/authentication/oauth?view=vsts

Let me know if there's a better way of doing it. This was the best I could think to make the minimal changes possible

evanpurkhiser · 2018-06-18T19:49:30Z

src/sentry/integrations/client.py

+class OAuth2ApiClient(ApiClient):
+    def __init__(self, identity, *args, **kwargs):
+        super(OAuth2ApiClient, self).__init__(*args, **kwargs)
+        self.identity = identity


I wonder if maybe we could actually make this more of a mixin OAuth2RefreshingMixin, so instead of this extending ApiClient, it's just a mixin that has this functionality. Since this also ties the client specifically to using identities.

evanpurkhiser · 2018-06-18T19:49:51Z

src/sentry/integrations/client.py

+        super(OAuth2ApiClient, self).__init__(*args, **kwargs)
+        self.identity = identity
+
+    def check_auth(self):


The name sounds good to me actually

evanpurkhiser · 2018-06-18T19:54:32Z

src/sentry/integrations/vsts/client.py


    def request(self, method, path, data=None, params=None):
+        self.check_auth()


This looks good to me :)

evanpurkhiser · 2018-06-18T19:56:03Z

src/sentry/integrations/vsts/integration.py

@@ -100,7 +97,7 @@ class VstsIntegrationProvider(IntegrationProvider):

    def get_pipeline_views(self):
        identity_pipeline_config = {
-            'redirect_url': absolute_uri('/extensions/vsts/setup/'),
+            'redirect_url': absolute_uri(VSTSIdentityProvider.oauth_redirect_url),


Actually we want to keep this hardcoded here I think, since the extensions/vsts/setup is integration specific, where as identity related things not specific to integrations.

…dentity field would be updated as well.

…rovider.

lauryndbrown added the Component: Integrations label Jun 13, 2018

lauryndbrown self-assigned this Jun 13, 2018

lauryndbrown requested review from evanpurkhiser, macqueen and MeredithAnya June 13, 2018 00:02

lauryndbrown commented Jun 13, 2018

View reviewed changes

lauryndbrown mentioned this pull request Jun 13, 2018

ref(integrations): Moved Vsts default_project to the config form #8698

Merged

lauryndbrown force-pushed the vsts-auth-refresh branch from 0d2844b to dfce3d0 Compare June 15, 2018 02:06

lauryndbrown commented Jun 15, 2018

View reviewed changes

evanpurkhiser requested changes Jun 18, 2018

View reviewed changes

evanpurkhiser approved these changes Jun 19, 2018

View reviewed changes

lauryndbrown added 16 commits June 19, 2018 16:47

rough draft.

810cbfe

Moved functionality to integration installation so that the default_i…

a2a44e7

…dentity field would be updated as well.

Removed code from vsts provider

a4019f9

Added tests for refresh identity.

5100d90

Saving place.

f5d0420

Added stuff, but need a way to get the provider from the id.

f2d0242

Added the test not done.:

86b0726

got tests tp work and moved functionality to providers."

01da3a6

removed unneeded conversion.

c3f8c0c

Removed whitespace.

53e7eff

Removed whitespace.

7135c0e

Fixed tests.

94b37d4

changed url in tests.

baca68a

changed auth_identity to identity. changed oauth client into a mixin.

ed37e6e

pass in redirect url rather than have it as a class variable in the p…

85e209b

…rovider.

merged and added everything back in

c66c039

lauryndbrown force-pushed the vsts-auth-refresh branch from 2dea6eb to c66c039 Compare June 20, 2018 00:45

lauryndbrown merged commit e82d127 into master Jun 20, 2018

lauryndbrown deleted the vsts-auth-refresh branch June 20, 2018 17:48

GitHub Actions bot locked and limited conversation to collaborators Dec 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(integrations): Vsts OAuth refresh #8725

feat(integrations): Vsts OAuth refresh #8725

lauryndbrown commented Jun 13, 2018 •

edited

Loading

lauryndbrown Jun 13, 2018

evanpurkhiser Jun 13, 2018

lauryndbrown Jun 15, 2018

evanpurkhiser Jun 18, 2018

lauryndbrown Jun 15, 2018

evanpurkhiser Jun 18, 2018

lauryndbrown Jun 15, 2018

evanpurkhiser Jun 18, 2018

evanpurkhiser Jun 18, 2018

evanpurkhiser Jun 18, 2018

evanpurkhiser Jun 18, 2018

lauryndbrown Jun 18, 2018

evanpurkhiser Jun 18, 2018

evanpurkhiser Jun 18, 2018

evanpurkhiser Jun 18, 2018

lauryndbrown Jun 18, 2018

lauryndbrown Jun 18, 2018

evanpurkhiser Jun 18, 2018

evanpurkhiser Jun 18, 2018

evanpurkhiser Jun 18, 2018

evanpurkhiser Jun 18, 2018

	def refresh_identity(self, auth_identity):
	refresh_token = auth_identity.data.get('refresh_token')

	if not refresh_token:
	raise IdentityNotValid('Missing refresh token')

	data = self.get_refresh_token_params(
	refresh_token=refresh_token,
	)
	req = safe_urlopen(self.get_refresh_token_url(), data=data)

	try:
	body = safe_urlread(req)
	payload = json.loads(body)
	except Exception:
	payload = {}

	error = payload.get('error', 'unknown_error')
	error_description = payload.get('error_description', 'no description available')

	formatted_error = 'HTTP {} ({}): {}'.format(req.status_code, error, error_description)

	if req.status_code == 401:
	raise IdentityNotValid(formatted_error)

	if req.status_code == 400:
	# this may not be common, but at the very least Google will return
	# an invalid grant when a user is suspended
	if error == 'invalid_grant':
	raise IdentityNotValid(formatted_error)

	if req.status_code != 200:
	raise Exception(formatted_error)

	auth_identity.data.update(self.get_oauth_data(payload))
	auth_identity.update(data=auth_identity.data)


		def request(self, method, path, data=None, params=None):
		self.check_auth()

		@@ -102,6 +119,48 @@ def get_oauth_data(self, payload):

		return data

		def refresh_identity(self, auth_identity):

feat(integrations): Vsts OAuth refresh #8725

feat(integrations): Vsts OAuth refresh #8725

Conversation

lauryndbrown commented Jun 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lauryndbrown commented Jun 13, 2018 •

edited

Loading