Pylons / pyramid

Pyramid - A Python web framework
https://trypyramid.com/
Other
3.97k stars 887 forks source link

come up with a better name for authenticated_identity #3611

Closed mmerickel closed 4 years ago

mmerickel commented 4 years ago

The authenticated_identity API is one of the major contributions of the new security policy. However it has already been a source of confusion and I think it is poorly named. For example request.is_authenticated is currently testing request.authenticated_identity is not None which I'm proposing below to be incorrect. It should instead test request.authenticated_userid is not None which remains bw-compat with Pyramid 1.x and frees us to do different things with the authenticated_identity object.

The purpose of the API is to allow the security policy to make available to user code an opaque, policy-specific object that they may use with the only contract being that it was created by the security policy. This solves a lot of problems with circular dependencies between things like unauthenticated_userid, request.user, authenticated_userid etc in which a common pattern right now is for a large portion of the "security" work to be done inside request.user instead of in the security policy itself. This is great.

The problem is that this opaque object should not necessarily indicate whether a request is authenticated or not. It's important to me that the API is generally useful to indicate to the calling app interesting information about the request such as what principals are included, whether or not the user is logged in, etc. At the same time, I do not want to break the existing API in with authenticated_userid returns None if the user is not authenticated (it's left to the app / security policy to define what "unauthenticated" actually means).

So I've been trying to come up with a name for this API that does not indicate whether the request is "authenticated" or not. So far, the one I like best is security_identity but I wanted to give some other stakeholders a chance to weigh in (@bertjwregeer, @luhn, @merwok, @dstufft come to mind).

If it is more clear the purpose of this object, it can be used much more similarly to how Pyramid 1.x uses effective_principals which can be used whether or not a user is logged in. This singular, and admittedly dumb, issue is the reason I haven't shipped 2.0 yet and naming is hard and so I've just dragged my feet on it. I'm very much leaning toward security_identity, or security_data, or something to that effect. I want it to potentially contain access token information / metadata regardless of authenticated status of the request.

merwok commented 4 years ago

If it is more clear the purpose of this object, it can be used much more similarly to how Pyramid 1.x uses effective_principals which can be used whether or not a user is logged in.

This makes sense. To give another precedent, with Django there is always access to request.user, you need to check properties such as is_anonymous or is_authenticated to determine if the request was authenticated.

Naming idea: identity_object (feels less clumsy than security_identity, and object is meant to allude to the opaqueness.

stevepiercy commented 4 years ago

As a person who has a weak grasp of the concepts discussed, and assuming the new name would become identity_object, what would it essentially provide? Stuff like this?

If y'all can explain it to a dummy like me, and I understand it, then you're on your way to a good solution.

mmerickel commented 4 years ago

The object is opaque, but would potentially contain the user object, principals, etc. It is explicitly undefined what it contains - it's just something the security policy computes and makes available for the app to use. In simple cases it's just a user object or None.

mmerickel commented 4 years ago

I personally think of it as a representation of an access token... in my own apps I have request.access_token which is never None, and contains provenance info as well as request.access_token.user, request.access_token.effective_principals, request.access_token.authenticated_userid, etc which I can forward into the relevant systems. However I do not really think access_token is a good name for this api generally speaking. I like the security prefix to map it back to the security api.

stevepiercy commented 4 years ago

Gotcha. It is None or an object that contains stuff, which could include, but is not limited to, the user object, principals, or access token.

Along those lines, *_object sounds agreeable to everyone because it's a "thing". identity_object implies it contains an identity when it could contain None, so that doesn't sound quite right in my dinosaur brain. security_object leaves it more open to both None and "stuff", and it pertains to the security policy, so I think that better represents its actuality.

digitalresistor commented 4 years ago

In all of my own policies that I've written this value would never be None, in it's place I always have some sort of placeholder or "empty" value that still matches the contract required elsewhere in the code.

I do like security_identity because it doesn't attribute any other information to it, and it doesn't try to force it into any particular type. I don't like identity_object because object has a certain connotation associated with it in the Python world, and it is perfectly acceptable for someone to set security_identity to just a string, or a tuple, or a list. As Michael mentioned it's an opaque value.

archoversight commented 4 years ago

This here be a fools errand.

luhn commented 4 years ago

I personally prefer identity because it's simple and doesn't imply anything, which I think is a positive. However, we did specifically move away from identity because it was too "all-encompassing". Have minds been changed on that?

I really, really don't like access_token because many apps don't have a concept of access tokens and the name is so loaded it really detracts from the idea of an opaque object.

digitalresistor commented 4 years ago

The discussion around this:

We want to make it explicit that it is related to security/authentication/authorization, and identity is too all-encompassing.

Was related to having it be something that is used by Pyramid, the all-encompassing part is because it felt like it was being used for too much at the time (it had a use directly within Pyramid for a very narrow scope, but with a big concept surrounding it)

In the context of that discussion there was no idea or thought behind it being a request identity, which has no relation to anything authorization/authentication at all.

I don't know what else to call it besides an identity. I think that is where the main hangup continues to be. If the authorization/authentication doesn't need to use it, and it's basically just a property hanging out that is documented/well defined but has no further use, is it really worth adding? If not, then every single developer that adds a new authentication/authorization policy will need their own implementation of where to stash information, and in fact many people do that with request.user or request.token.

It ends up being a mess.

Also: I deleted my previous comment, I wanted to add extra info around my thought process. Thing is though that it is not strictly necessary to have an identity or authenticated_identity or any opaque value, it's just a nice to have.

Naming things is hard.

stevepiercy commented 4 years ago

After reading the comments from @bertjwregeer and @luhn, I am swayed to agree with the points they have made, and now think identity is the best way forward.

merwok commented 4 years ago

I like it too; it is a simple name that can be searched for in docs, and does not promise too much (as user or account would do).