PR#259: Make OpenID Connect tokens revoke when consent is revoked - ipsilon

ipsilon

#259 Make OpenID Connect tokens revoke when consent is revoked

Merged 7 years ago by puiterwijk. Opened 7 years ago by puiterwijk.

puiterwijk/ipsilon oidc-revoke into master

Make OpenID Connect revoke active tokens when consent is revoked

Patrick Uiterwijk • 7 years ago

433cb51

Allow provider to act on revoking consent

Patrick Uiterwijk • 7 years ago

850cba4

ipsilon/providers/common.py

file modified

		`@@ -85,6 +85,9 @@`
		`def consent_to_display(self, consentdata):`
		`raise NotImplementedError`

		`+ def revoke_consent(self, user, clientid):`
		`+ return True`
		`+`

		`class ProviderPageBase(Page):`

ipsilon/providers/openidc/store.py

file modified

+12

		`@@ -289,6 +289,18 @@`

		`self.del_unique_data('token', token_id)`

		`+ def revokeConsent(self, username, client_id):`
bowlofeggs commented 7 years ago PEP-8 would suggest that you call this method `revoke_consent`.
alert("boo! (puiterwijk was here)"); Uiterwijk (puiterwijk)">puiterwijk commented 7 years ago Yep. And I opened #261 for this. However, since this file is currently camelCase, I wanted to do all the fixing in one commit so that it doesn't become inconsistent in a file.
		`+ data = self.get_unique_data('token', name='username', value=username)`
		`+`
		`+ removed_token = False`
		`+ for uuid in data.keys():`
		`+ token = self.get_unique_data('token', uuid)[uuid]`
		`+ if token['client_id'] == client_id:`
		`+ self.invalidateToken(uuid)`
		`+ removed_token = True`
		`+`
		`+ return removed_token`
		`+`
		`def storeUserInfo(self, userinfo):`
		`to_store = {}`
		`for key in userinfo:`

ipsilon/providers/openidcp.py

file modified

		`@@ -223,6 +223,9 @@`

		`return d`

		`+ def revoke_consent(self, user, clientid):`
		`+ return self.datastore.revokeConsent(user, clientid)`
		`+`
		`def on_reconfigure(self):`
		`super(IdpProvider, self).on_reconfigure()`
		`self.init_idp()`

ipsilon/user/common.py

file modified

		`@@ -22,6 +22,13 @@`
		`def revoke(self, provider, clientid):`
		`us = UserSession()`
		`user = us.get_user()`
		`+`
		`+ provname = provider`
		`+ provmod = self._site['provider_config'].available.get(provname,`
		`+ None)`
		`+ if provmod is not None:`
		`+ if not provmod.revoke_consent(user.name, clientid):`
		`+ raise Exception('Provider refused to revoke')`
		`user.revoke_consent(provider, clientid)`
		`raise cherrypy.HTTPRedirect(self._master.url)`
		`revoke.public_function = True`

tests/openidc.py

file modified

+34 -8

		`@@ -257,7 +257,7 @@`
		`'amr': json.dumps([]),`
		`'acr': '0'`
		`}`
		`- token = check_info_results(page.text, expect)`
		`+ old_token = check_info_results(page.text, expect)`
		`except ValueError, e:`
		`print >> sys.stderr, " ERROR: %s" % repr(e)`
		`sys.exit(1)`
		`@@ -279,7 +279,7 @@`
		`try:`
		`page = sess.revoke_all_consent(idpname)`
		`except ValueError, e:`
		`- print >> sys.stderr, "" % repr(e)`
		`+ print >> sys.stderr, " ERROR: %s" % repr(e)`
		`sys.exit(1)`
		`print " SUCCESS"`

		`@@ -289,6 +289,17 @@`
		`'https://127.0.0.11:45081/sp/redirect_uri?log'`
		`'out=https%3A%2F%2F127.0.0.11%3A45081%2Fsp%2F',`
		`require_consent=True)`
		`+ h = hashlib.sha256()`
		`+ h.update('127.0.0.11')`
		`+ h.update(user)`
		`+ h.update('testcase')`
		`+ expect = {`
		`+ 'sub': h.hexdigest(),`
		`+ 'iss': 'https://127.0.0.10:45080/idp1/openidc/',`
		`+ 'amr': json.dumps([]),`
		`+ 'acr': '0'`
		`+ }`
		`+ new_token = check_info_results(page.text, expect)`
		`except ValueError, e:`
		`print >> sys.stderr, " ERROR: %s" % repr(e)`
		`sys.exit(1)`
		`@@ -309,13 +320,13 @@`
		`try:`
		`# Testing token without client auth`
		`r = requests.post('https://127.0.0.10:45080/idp1/openidc/TokenInfo',`
		`- data={'token': token['access_token']})`
		`+ data={'token': new_token['access_token']})`
		`if r.status_code != 401:`
		`raise Exception('No 401 provided')`

		`# Testing token where we removed part of token ID`
		`r = requests.post('https://127.0.0.10:45080/idp1/openidc/TokenInfo',`
		`- data={'token': token['access_token'][1:],`
		`+ data={'token': new_token['access_token'][1:],`
		`'client_id': reg_resp['client_id'],`
		`'client_secret': reg_resp['client_secret']})`
		`r.raise_for_status()`
		`@@ -325,7 +336,7 @@`

		`# Testing token where we rempoved part of check string`
		`r = requests.post('https://127.0.0.10:45080/idp1/openidc/TokenInfo',`
		`- data={'token': token['access_token'][:-1],`
		`+ data={'token': new_token['access_token'][:-1],`
		`'client_id': reg_resp['client_id'],`
		`'client_secret': reg_resp['client_secret']})`
		`r.raise_for_status()`
		`@@ -335,7 +346,7 @@`

		`# Testing valid token`
		`r = requests.post('https://127.0.0.10:45080/idp1/openidc/TokenInfo',`
		`- data={'token': token['access_token'],`
		`+ data={'token': new_token['access_token'],`
		`'client_id': reg_resp['client_id'],`
		`'client_secret': reg_resp['client_secret']})`
		`r.raise_for_status()`
		`@@ -359,10 +370,25 @@`
		`if len(info['scope']) != 0:`
		`raise Exception('Unexpected scopes found: %s' % info['scope'])`

		`+ # Testing previously revoked token`
		`+ r = requests.post('https://127.0.0.10:45080/idp1/openidc/TokenInfo',`
		`+ data={'token': old_token['access_token'],`
		`+ 'client_id': reg_resp['client_id'],`
		`+ 'client_secret': reg_resp['client_secret']})`
		`+ r.raise_for_status()`
		`+ info = r.json()`
		`+ if 'error' in info:`
		`+ raise Exception('Token introspection returned error: %s'`
		`+ % info['error'])`
		`+ if info['active']:`
		`+ raise Exception('Revoked token active')`
		`+ if len(info) != 1:`
		`+ raise Exception('Token contained more info then inactive')`
		`+`
		`# Delete test client and then try to use it`
		`sess.delete_oidc_client(idpname, reg_resp['client_id'])`
		`r = requests.post('https://127.0.0.10:45080/idp1/openidc/TokenInfo',`
		`- data={'token': token['access_token'],`
		`+ data={'token': new_token['access_token'],`
		`'client_id': reg_resp['client_id'],`
		`'client_secret': reg_resp['client_secret']})`
		`if r.status_code != 400:`
		`@@ -381,7 +407,7 @@`

		`# Testing valid token`
		`r = requests.post('https://127.0.0.10:45080/idp1/openidc/UserInfo',`
		`- data={'access_token': token['access_token']})`
		`+ data={'access_token': new_token['access_token']})`
		`r.raise_for_status()`
		`info = r.json()`
		`if 'sub' not in info:`

no initial comment

bowlofeggs commented on line 5 of ipsilon/providers/openidc/store.py 7 years ago

PEP-8 would suggest that you call this method revoke_consent.

puiterwijk commented on line 5 of ipsilon/providers/openidc/store.py 7 years ago

Yep. And I opened #261 for this.
However, since this file is currently camelCase, I wanted to do all the fixing in one commit so that it doesn't become inconsistent in a file.

bowlofeggs commented 7 years ago

LGTM