SessionAuthenticationHelper should be able to store arbitrary fields in the session

[petterroea]

Get Support

To get help or technical support, see Get Support.

Feature Request

Please search the issue tracker for similar issues before submitting a new issue.

Done

Is your feature request related to an issue? Please describe.

I am writing a Pyramid webserver that authenticates using an external OIDC provider. Unlike many test implementations I have seen on the internet, my main goal is to store the access and refresh tokens and use them to determine my logged in state, for several reasons:

• Allows to check if token is expired
• Allows OIDC issuer lookups to check if token is revoked or not if wanted
• The access token contains loads of extra useful metadata about the authenticated user, like their UID, which can then be extracted in the security policy

I want to store these tokens in the session, encrypted mind you, because I don't want to have to rely on filesystem/memory (not good for k8s-hosted workloads) or redis(i would like to avoid extra infrastructure complexity) or similar kinds of session backends. Based on suggestions from the security docs, I therefore had a look at the SessionAuthenticationHelper: https://github.com/Pylons/pyramid/blob/main/src/pyramid/authentication.py#L1249

It supports kwargs, and I initially assumed it would therefore allow me to store arbitrary information together with the userid, a fair compromise. However, this is not the case. The kwargs are simply ignored. I assume they only exist there for the sake of defensive programming, or because the remember function uses the same pattern. Allthough, for the remember function, the docs explicitly state that the kwargs are there so you can pass custom args to your security policy.

Describe the solution you'd like
I think the easiest solution here would be for SessionAuthenticationHelper to iterate over the provided kwargs and set them in the session together with the user id. The forget function could for example do something similar where it deletes all keys in the session that exist in kwargs.

Describe alternatives you've considered

My current alternative is to just write to the session object myself. SessionAuthenticationHelper is just a few lines of code anyways. I see AuthTktCookieHelper doesn't have this behavior either so maybe it's better to not make things inconsistent either.

[mmerickel]

I think it's a good question. My suggestion would be to simply write your own SessionAuthenticationHelper - they are very intentionally super lightweight. The kwargs are there for the purpose of conforming to the ISecurityPolicy API where remember/forget do accept extra kwargs that the policy may want to use on a per-policy basis as only the userid is required by default. So you'd write your own SecurityPolicy object and your remember/forget could store the data in the session - remember the helper is not necessary at all.

class MySecurityPolicy:
    def remember(self, request, userid, *, claims=None, **kw):
        request.session['auth.userid'] = userid
        if claims:
            request.session['auth.claims'] = claims
        return []

    def forget(self, request, **kw):
        request.session.pop('auth.userid', None)
        request.session.pop('auth.claims', None)
        return []

    # other security policy methods

I would consider any PR to extend the SessionAuthenticationHelper with some params like "allow_claims" or something to support this flow inherently but yeah, the helpers are just so lightweight that I don't know if it's useful for Pyramid to ship it explicitly.

[petterroea]

Hello and thank you for your reply. Yes, I ended up just writing something myself.

I think a SessionAuthenticationHelper extension with a simple allow_claims param might be a bit too narrow. I believe you basically have to keep hold of your tokens if you want to properly implement an OIDC client - especially if you want to handle token revocation and the fetching of a new access token from time to time. Storing claims doesn't seem sufficient.

This is why I think exposing an API that allows for storage of arbitrary fields is useful for consumers of the API