Why is identity forced to be string?

[dpopowich]

The module-level function, remember() asserts that the identity is a string:

    assert isinstance(identity, str), identity
    assert identity

Shouldn't that be the burden of the underlying implementation of an IdentityPolicy?

My use-case: I'm building a custom IdentityPolicy and associated AuthorizationPolicy that sends a JWT token back to the client with a custom header. In my user-session code I want to be able to do, e.g.:

   def login(request):
      # Get `username` and `password` from request data, validate, etc.
      username, password = ...
      # Using an underlying model, User, call a classmethod `login` which
      # validates the credentials and returns a user instance on success; None on failure
      user = User.login(username, password)
      if user is None:
         raise web.HTTPUnauthorized()
      # I have a valid, logged in user, json-ify the object and set header
      resp = web.json_response(user)
      remember(request, resp, user)  # <== BLOWS UP WITH ASSERTION ERROR
      return resp

The implementation of my policies expects a User instance in all places an identity is passed around. When creating the JWT the state of the user instance dictates the claims made. I want this logic in the IdentityPolicy not in the caller.

It seems to me that at the API level identities should be opaque and leave serialization/validation up to the underlying policy implementations.

[Dreamsorcerer]

I could be mistaken, as the details are not fresh in my mind, but that sounds like it makes it impossible to mix and match identity policies.

If you are passing some custom User object to remember() and a CookiesIdentityPolicy is used, what is it supposed to save into the cookie?

Expected behaviour, would be that you pass the user's ID (it is called an identity for a reason), which can then be remembered safely by any IdentityPolicy and your code should be able to load it from that on subsequent requests. e.g. If User is a sqlalchemy model, then you might do something like: remember(request, resp, user.id)

Then in the other functions where you receive the identity, you would do something like user = session.get(User, identity).

[dpopowich]

While a trivial point, I note even your suggestion of remember(request, resp, user.id) doesn't work because User.id is not a string. I also note the implementation of the module-level remember() accepts arbitrary keyword args for, presumably, IdentityPolicy implementation needs, so there is some expectation of more than just a string being required to generate a token/header/cookie/etc.

As for mix&matching...why would I want to mix and match a home-grown, specific-to-my-application identity? Especially considering JWT which allows you to store arbitrary claims (i.e., by design JWT is meant to store far more than just an id).

Can remember() be wrapped to do what I want? Yes and that's what I'll have to do to use this API, but I note, except for the assertion that it's a string where I pointed out, the rest of the API makes no assumption as to the type or value of the identity and passes it around as an opaque object. I think the API will be more useful by not forcing an assumption of usage. And by making my identity an object, implementing the AuthorizationPolicy is soooo simple.

I note there's a protoype of a JWT IdentityPolicy in the source (undocumented -- considered not ready for primetime?) which doesn't implement remember() or forget(). It only implements identify(). Is the assumption that by some means outside the implementation the client has received a JWT, but this implementation knows how to decode? Strikes me that it makes more sense to tie encode/decoding together in one IdentityPolicy.

My implementation ties together an IdentityPolicy, AuthorizationPolicy, and middleware (processing requests with the Authorization header set, decoding and reconstituting the user instance), all working in harmonious beauty. ;-)

Meanwhile, here's what I'll have to do to wrap the stock remember if I want to continue with my usage:

import aiohttp_security as aiosec

async def remember(request, response, *, user, **claims):
   await aiosec.remember(request, response, '__unused__', user=user, **claims)

And then code my impl accordingly:

   async def remember(self, req, resp, identity, *, user, **claims):
      ...

[Dreamsorcerer]

While a trivial point, I note even your suggestion of remember(request, resp, user.id) doesn't work because User.id is not a string.

I often use the username as the ID (which is what is shown in most of the examples). If it's an int, you can still convert it easily with str() and int().

As for mix&matching...why would I want to mix and match a home-grown, specific-to-my-application identity? Especially considering JWT which allows you to store arbitrary claims (i.e., by design JWT is meant to store far more than just an id).

If you don't want compatibility with the defined API in order to have mix-and-match things, then I'm not sure what this library is actually offering you. You might as well just call identity_policy.remember() directly, there's not much else the library is going to do for you. That's pretty much all these functions do anyway: https://github.com/aio-libs/aiohttp-security/blob/master/aiohttp_security/api.py

It'll also make static typing a lot easier for you. I'm looking at adding annotations soon, and if we went with your approach, then all the functions would need to be annotated with Any and you would get no typing information, as there is no obvious way we can infer the type from the identity policy in these functions.

I note there's a protoype of a JWT IdentityPolicy in the source (undocumented -- considered not ready for primetime?) which doesn't implement remember() or forget(). It only implements identify(). Is the assumption that by some means outside the implementation the client has received a JWT, but this implementation knows how to decode? Strikes me that it makes more sense to tie encode/decoding together in one IdentityPolicy.

Seems to be deliberate: https://github.com/aio-libs/aiohttp-security/pull/147#discussion_r180704786

I think there is some mistunderstanding/disagreement about how the library is meant to work. I don't have enough in-depth knowledge right now to provide more useful feedback though.

[dpopowich]

If you don't want compatibility with the defined API...

All I'm saying is forcing identity to be a str by the assertion is arbitrary and limits the potential of the API. I believe the API will be more useful for wider usage than is currently allowed if identity is left an opaque type passed through to implementations.

[toolforger]

The API accepts an identity, you want to send it a User which is identity plus password plus whatever may be appropriate for your use case.

The problem is that the library cannot know what the operations on your User object are. There could be some variation to equality, for example, and your calling code should know have to know whether the current or future versions of aiohttp-security may ever want to use equality (for example somebody might want to add some caching operations and uses the identity as a key, then you try caching and everything works, but it will fail in production because the wrong fields are compared).

It's the wrong mental model for "identity". Identity is a dict (or database) key: it identifies all the data associated with a user, it does not contain them. I.e. store the User objects outside aiohttp-security. If you need to deal with multiple identities and associated User objects, use an identity->User dict.