Keycloak modules retry request on authentication error, support refresh token parameter

[armkeh]

SUMMARY

Fixes #8857.

Wraps all requests to Keycloak in the Keycloak modules (keycloak_authentication, keycloak_authz_authorization_scope, keycloak_authz_custom_policy, etc.) with retry logic to make use of a new refresh_token module parameter.

This improves the user experience when using Keycloak modules with the auth_token parameter; previously if that token expired during playbook execution, subsequent tasks would fail. Now they "fall back" to using the refresh_token, or, if it is not provided or is expired itself, to using the auth_username and auth_password.

    def _request(self, url, method, data=None):
        def make_request_ignoring_401():
            try:
                return open_url(url, method=method, data=data,
                                http_agent=self.http_agent, headers=self.restheaders,
                                timeout=self.connection_timeout,
                                validate_certs=self.validate_certs)
            except HTTPError as e:
                if e.code != 401:
                    raise e

            return None

        r = make_request_ignoring_401()
        if r is not None:
            return r

        # Authentication may have expired, re-authenticate with refresh token and retry
        refresh_token = self.module.params.get('refresh_token')
        if refresh_token is not None:
            token = _get_token_using_refresh_token(self.module.params)
            self.restheaders['Authorization'] = 'Bearer ' + token

        r = make_request_ignoring_401()
        if r is not None:
            return r

        # Retry once more with username and password
        auth_username = self.module.params.get('auth_username')
        auth_password = self.module.params.get('auth_password')
        if auth_username is not None and auth_password is not None:
            token = _get_token_using_credentials(self.module.params)
            self.restheaders['Authorization'] = 'Bearer ' + token

        return make_request_ignoring_401()

ISSUE TYPE

• Feature Pull Request

COMPONENT NAME

keycloak

[russoz]

hi @armkeh thanks for your contribution!

Couple of comments on the PR.

[russoz]

LGTM

[felixfontein]

Thanks for your contribution! Please add a changelog fragment. Thanks.

[felixfontein]

Shouldn't the last request be made without ignoring 401?

[armkeh]

I've fixed this now; to make the code wrapping the re-requests more consistent, the last request still catches the 401, but it's re-raised afterwards (thanks @russoz for pointing out the failure to raise exceptions in earlier commits).

[desand01]

I think we should not make a new request if the access token has not been refreshed.

Suggested change

	if refresh_token is not None:
	token = _get_token_using_refresh_token(self.module.params)
	self.restheaders['Authorization'] = 'Bearer ' + token
	r = make_request_ignoring_401()
	if r is not None:
	return r
	if refresh_token is not None:
	token = _get_token_using_refresh_token(self.module.params)
	self.restheaders['Authorization'] = 'Bearer ' + token
	r = make_request_ignoring_401()
	if r is not None:
	return r

[armkeh]

Thanks, good catch; moved the re-requests inside the if statements to avoid making them unnecessarily.

[russoz]

Couple of comments more.

[russoz]

Little late in the game for that, I know, but I believe this would be leaner as:

Suggested change

	def _request(self, url, method, data=None):
	def make_request_catching_401():
	try:
	return open_url(url, method=method, data=data,
	http_agent=self.http_agent, headers=self.restheaders,
	timeout=self.connection_timeout,
	validate_certs=self.validate_certs)
	except HTTPError as e:
	if e.code != 401:
	raise e
	return e
	r = make_request_catching_401()
	if not isinstance(r, Exception):
	return r
	def _request(self, url, method, data=None):
	class Unauthorized(Exception):
	pass
	def make_request_catching_401():
	try:
	return open_url(url, method=method, data=data,
	http_agent=self.http_agent, headers=self.restheaders,
	timeout=self.connection_timeout,
	validate_certs=self.validate_certs)
	except HTTPError as e:
	if e.code != 401:
	raise e
	raise Unauthorized()
	try:
	return make_request_catching_401()
	exception Unauthorized:
	pass
	except Exception:
	raise

and so on

[armkeh]

I like this, however we need to keep the original 401 exception in case no re-auth options are available and it needs to be re-raised. (Thanks for catching the mistake where it was being returned and not raised in your other comment.)

[russoz]

This last call to make_request_catching_401() is not being verified. If, for whatever reason, the HTTP request inside that call returns a 401, that Exception object will be returned (not raised) and no error handling will be performed.

[armkeh]

Thanks, good catch; I've reworked the retry wrappers to make the flow a little clearer, removing early returns, and made sure to throw the exception if that's the final result.

[russoz]

one possible improvement and a comment, other than that LGTM

[russoz]

It might be simpler to go:

Suggested change

	try:
	r = json.loads(to_native(open_url(auth_url, method='POST',
	validate_certs=validate_certs, http_agent=http_agent, timeout=connection_timeout,
	data=urlencode(payload)).read()))
	except ValueError as e:
	raise KeycloakError(
	'API returned invalid JSON when trying to obtain access token from %s: %s'
	% (auth_url, str(e)))
	except Exception as e:
	raise KeycloakError('Could not obtain access token from %s: %s'
	% (auth_url, str(e)), authError=e)
	try:
	token = r['access_token']
	except KeyError:
	raise KeycloakError(
	'Could not obtain access token from %s' % auth_url)
	return token
	try:
	r = json.loads(to_native(open_url(auth_url, method='POST',
	validate_certs=validate_certs, http_agent=http_agent, timeout=connection_timeout,
	data=urlencode(payload)).read()))
	return r['access_token']
	except ValueError as e:
	raise KeycloakError(
	'API returned invalid JSON when trying to obtain access token from %s: %s'
	% (auth_url, str(e)))
	except KeyError:
	raise KeycloakError(
	'Could not obtain access token from %s' % auth_url)
	except Exception as e:
	raise KeycloakError('Could not obtain access token from %s: %s'
	% (auth_url, str(e)), authError=e)

or remove the except KeyError block entirely - the raised error after that is almost the same.

[russoz]

This still bugs me on principle - exceptions are meant to be raised not returned :-) - but let's not hold this PR back because of it. I'll make a note to myself to return to this later and see if I can make it any better.