I don't think it's a good idea to raise any exception as-is and show it to users like this; the message might contain secrets.

The code should also make the difference between an error (log it) and an authentication failure (just return None).

Just use code_verifier = secrets.token_urlsafe(60); code_verifier_str=code_verifier.decode('ascii'). It's shorter and guarantees you don't lose bytes of entropy in the substitution.

no need for utf8, you can use .encode('ascii')

(or directly code_verifier with the change mentionned above)

Wrong name; this is not a nonce.

I'd rename the parameter next_path in the input too, so it's clearer what it is

it's very unclear what "state" means when not reading this function. Could you rename it to csrf_token?

You need to remove nonce['code_verifier'] from the _auth_exception dict defined in swh/web/auth/backends.py, or the dict will grow forever.

meh

well_known

from . import sample_data, so it's clearer in the code where these variables come from

Use urllib.parse

Use dict equality. pytest rewrites assert statements to show a nice diff in case they don't match.

Where is session_state used?

no parentheses

You should split this test into different tests after each assert isinstance(request.user, AnonymousUser).

And you're missing a test for invalid challenge code.

Because Django authentication backends should return None when authentication failed (see https://docs.djangoproject.com/fr/2.2/topics/auth/customizing/#writing-an-authentication-backend)

Ack, I got what you mean. Let sentry capture the exception and simply inform the user authentication has failed when authenticate returns None. We can get rid of the get_auth_exception function this way.

This only works if auth_url already has query parameters. Is this guaranteed by self._keycloak.auth_url?

Hopefully yes, the redirect_uri plus other parameters will be inserted as query parameters by self._keycloak.auth_url

In practice, the certs list contains a single element. Anyway, there is an alternative way to get the realm public key:

realm_public_key = self._keycloak.public_key()
self._realm_public_key = '-----BEGIN PUBLIC KEY-----\n'
self._realm_public_key += realm_public_key
self._realm_public_key += '\n-----END PUBLIC KEY-----'

I would go for that approach instead.

Ack

Using a tuple key seems a better choice indeed.

Ack, will improve.

Did not think of checking the user.backend value, much better indeed.

Oh I did not known that Python standard library has a secrets module. Thanks !

ack

This is for the case if we declare multiple clients in the realm to be used from swh-web.

Indeed, state is the nonce here

ack

I prefer to keep the OpenID Connect parameter name.

Ack

I will remove the _auth_exception dict instead.

Ack

I prefer empty strings to give a hint about the expected type.

Nope, I forgot to modify that template.

Typo in python-keycloak: https://github.com/marcospereirampj/python-keycloak/blob/master/keycloak/keycloak_openid.py#L146

Ack

I think it should be assert re.match(r'^[a-zA-Z0-9-\._~]+$', code_verifier) instead

ack

I can't as nonce and query_dict do not have the same content.

I simply want to produce the same type of redirect url Keycloak generates. The session_state value is intended to be used to check OpenID connect session from the frontend side (see https://openid.net/specs/openid-connect-session-1_0.html#ChangeNotification). I do not intend to implement it and will do that check using a Django middleware instead. But who knows, we may need to implement the frontend check in the future.

ack

whoopsie

Ack, will improve thoses tests.

It is a list of strings.

Allright

ack

Ack, I should have read the doc.

login_data and query_dict only share the state and redirect_uri keys so it does not worth it.