Security policy implementation

[luhn]

• ISecurityPolicy
• Configurator.set_security_policy
• bwcompat security policy
• Request properties
• Security view deriver
• Built-in policies and helpers
• Narrative docs
• DeprecationWarning / zope.deprecation

Closes #3422

[stevepiercy]

Please include a signed CONTRIBUTORS.txt. Otherwise LGTM so far.

[luhn]

Security policies are working! Please review. Tests are passing locally, but it doesn't look like CI got triggered?

Test coverage is 100% except for this line. Looks like it's from the repoze.bfg days. Still necessary in Pyramid 2.0, or can I delete it?

I think I should add an integration test for security policies and legacy policies. Also update the other integration tests to use security policies.

I still need to implement built-in policies and helpers, which hasn't been discussed much:

• Existing authentication policies could be made into mixins providing identify, remember, and forget.
• Implement an ACL helper, as discussed in the post-mortem.
• I'd like to see a BasicSessionSecurityPolicy which remembers to the session and permits all or nothing based on the presence of an identity. I think this would be a good plug-in-and-play default for a lot of smaller projects.

[mmerickel]

I'd like to see a BasicSessionSecurityPolicy which remembers to the session and permits all or nothing based on the presence of an identity. I think this would be a good plug-in-and-play default for a lot of smaller projects.

The issue with shipping security policy classes is that they tie together a lot of decisions into one class. We should not add any without first having the building blocks and at least testing out the class in the existing tutorial (probably in the wiki2 tutorial like https://docs.pylonsproject.org/projects/pyramid/en/1.10-branch/tutorials/wiki2/authentication.html).

[mmerickel]

Existing authentication policies could be made into mixins providing identify, remember, and forget.

Mixins work for some things but we're actively trying to reduce the usage of things like the CallbackAuthenticationPolicy and prefer a composition approach instead. It's come up several times but helper objects would be a better way to start, mixins could be built using them, but the helper objects need to exist first.

[mmerickel]

I added a bullet for deprecations. Anywhere that you're putting into the docs that something is deprecated should be followed by calls to zope.deprecation in the code to trigger DeprecationWarning warnings. There's a lot of code that needs to be deprecated, but it can come after things settle down.

[luhn]

Related to my question above, I realize remember() and forget() are still dealing with userid as well. I think that makes sense, since most applications will just remember a user ID rather than a full identity object. So do we want to keep around the concept of "userid" around and separate from identity?

Other than that, I've reviewed the code and everything looks good AFAICT. Please review yourselves and let me know if anything needs to change.

[digitalresistor]

I need this PR to land and a new release of Pyramid, like yesterday ;-) This would solve some problems I am currently working on.

I'll review in a second, but changing "userid" to "identity" may make sense, but I'd like @mmerickel's opinion on that.

[mmerickel]

I think the api for remember/forget should remain a userid and not an opaque object. The identity object is opaque and is meant for exposing data to permits but there's no standard way to load one outside of the policy in order to pass it in. I think authenticated_identity is basically replacing effective_principals but there's still use cases for userid.

[luhn]

Since we're still using userid, should I undo the deprecation of authenticated_userid?

[mmerickel]

@luhn that’s my feeling right now. Definitely open to alternative opinions though. I think it’s important to maintain a bidirectional identifier for the authenticated payload maintained by the policy.

[luhn]

Done!

[mmerickel]

I confirmed over the weekend that some $work apps continue to function unchanged using this branch which is great. I'm planning to get into the rest of the details more this week and try to get this merged.

[luhn]

Thanks @mmerickel! I've applied the suggestions.

[mmerickel]

I'm ready to merge this. I do want to continue the conversation around other issues in #3422 and #3423.

[mmerickel]

@luhn can you do me a tiny favor and rename this PR to remove the WIP just to make sure you're happy with things before I merge?

[luhn]

👍

[luhn]

Excited to see this merged! Thanks to everyone for working with me on this.

Cheers! 🍻

[merwok]

Hi! Sorry to comment after this is merged, but I was following the issue and not aware of this PR.

I don’t understand the reason for using __str__ as the method to convert Identity to user ID.

• __str__ is meant to control what happens with print, %s formatting, etc.
• IMO it is good practice to have __str__ fast, without side effects, without possibility of failure (so no parsing, casting, etc)
• repr uses __str__ if the class doesn’t define __repr__
• REPL, logging, others use repr or str, so seeing something that’s a user ID instead of custom str/repr output (like <SomeIdentityClass id=blah source=value>) in debug logs, etc. could make inspection harder
• depending on the system, user ID may not be a string
• calling str(identity) instead of identity.some_explicit_method() seems a bit obscure (not discoverable)

If people agree with the disadvantages here, could this API decision be reconsidered?

[mmerickel]

This is a discussion I'm happy to have. I'm not in love with __str__ myself. I'd ask that since this is merged we open a new issue to talk about it though, instead of here.

[ztane]

There are 2 good options that come to my mind: either return storable_id, IdentityClass tuple or have another function in the policy that derives the serializable id from the identity class...

[luhn]

There's been some discussion on this, I dug up what I could find:

#3422 (comment)
#3465 (comment)

[luhn]

Opened an issue over at #3520

[merwok]

Minor: using outputs here (and other parts of this PR) could be misleading (does it mean print?), compared to returns

[merwok]

@luhn can I ask why this method checks old-style authn policy before security policy?
The other methods in the mixin only work with security policy, without falling back on (or checking first!) an authentication policy.

Is there a goal to support a config with security policy + authentication policy?

[luhn]

Usually we just let LegacySecurityPolicy handle the bw compat, but in this case we need to circumvent the str(security.identify(self)), otherwise we'd be changing the behavior of the legacy method.

Now that str(identity) is out and authenticated_userid is back, we should be able to just reference the security policy and let LegacySecurityPolicy work its magic.

[merwok]

Great! ✨

[merwok]

Noticed today that the actual default is md5: https://github.com/Pylons/pyramid/pull/3465/files#diff-00f789cd007443672848b9eb23973a86R861

Ping @mmerickel this could be changed in your doc PR too? Fix the default to match the doc (as this is a new class for Pyramid 2), then the docs examples can remove the explicit hashalg parameter!

For the backward-compatible authn policy, its constructor could be kept to md5 (if that is indeed the previous behaviour).

[mmerickel]

Unfortunately this class is not new.

[mmerickel]

I can't believe we forgot to fix it when we changed it on the policy. I'm changing it now and we'll just release a deprecation in 1.10.5.

[merwok]

Alright then I should send a doc PR for 1.x / stable branch?
(not sure how many versions of Pyramid are maintained and how backports work)

[mmerickel]

I did it yesterday in #3558

[invisibleroads]

These are significant changes!

[stevepiercy]

@invisibleroads you can continue to use existing auth policy stuff, too. See the What's New in Pyramid 2.0 document for details in the blue box.

[invisibleroads]

Pyramid 2.0 is big news. I am very happy to see that Pyramid is still very alive after 10+ years thanks to lots of work from all of you.