Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

security docs enhancements #1399

Merged
merged 16 commits into from
Nov 18, 2014
Merged

Conversation

mmerickel
Copy link
Member

This PR replaces #1397.

@mmerickel
Copy link
Member Author

This PR changes the public signature for pyramid.security.remember to accept a userid. We should deprecate the principal parameter and raise an error if neither userid nor principal are supplied to the function. New signature: remember(request, userid=_marker, **kw).

@@ -16,7 +16,7 @@ Authentication API Functions

.. autofunction:: forget

.. autofunction:: remember
.. autofunction:: remember(request, userid, **kwargs)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant? autofunction already sniffs the signature, unless it is hidden by a decorator.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's done here to hide the userid=_marker from the new signature in the docs. We do this in some other areas as well where a function has some extra arg used only for testing.

@tseaver
Copy link
Member

tseaver commented Nov 10, 2014

-0 on renaming principal -> userid: they not universally equivalent. For instance, if the argument is a token or certificate, it is not a "user id". "Principal" is the correct security-industry jargon for this concept.

@mmerickel
Copy link
Member Author

Like it or not pyramid has named the identifier tracked between requests as a userid, whether it is a token or id or anything else. See authenticated_userid and unauthenticated_userid in the authentication policies. The oddity here is that you expect unauthenticated_userid to return exactly what remember(request, userid) was passed, yet it is called a principal in remember. Principal is used inside of pyramid strictly in the context of authorization, except for this one API.

mmerickel added a commit that referenced this pull request Nov 18, 2014
@mmerickel mmerickel merged commit 3be8955 into master Nov 18, 2014
@mmerickel mmerickel deleted the feature.security-docs-enhancements branch November 18, 2014 02:20
@sontek
Copy link
Member

sontek commented Jan 1, 2015

@mmerickel Would it make more sense to introduce authenticed_principal / unauthenticated_principal and deprecate the old user id ones instead? To me it does seem quirky to implemented a token based authentication policy and label everything userid.

@mmerickel
Copy link
Member Author

I'd be happy to name it a token but I think naming it a principal is an anti-pattern. The default policies treat the token as a principal but that is a security vulnerability if you are not careful and I think it should be more explicit which principals people are allowing into their authorization system.

@tseaver
Copy link
Member

tseaver commented Jan 2, 2015

I don't get that: principal is the precise term for what the authn/authz machinery deals with.

@mmerickel
Copy link
Member Author

They need to be identified and authenticated before they can be assigned rights and privileges over resources in the network. A principal typically has an associated identifier (such as a security identifier) that allows it to be referenced for identification or assignment of properties and permissions.

I'd say authz, not authn.

@tseaver
Copy link
Member

tseaver commented Jan 2, 2015

Principals are first "authenticated"/"identified"; only then can their authorization be tested. We didn't make this jargon up: it widely used in software security analysis. If presenting a valid token as part of a request gets you access to otherwise protected resources, then a token is a principal: that is what principal means.

@mmerickel
Copy link
Member Author

In my mind, it'd be authenticated_principals instead of authenticated_token (note the plural) because to me translating a single token into multiple principals (the relationship between authenticated_userid and effective_principals currently) is natural. I also like how userid is pretty simply to explain as an arbitrary token so we don't have to rename the API at all to clarify the behavior. Basically everyone using Pyramid is probably using that particular API and changing it just for docs seems not so great.

I genuinely think the distinction is useful in understanding the API. If you feel so strongly about it I won't complain if you want to change it. I've made my feelings pretty clear at this point. But I'd appreciate having some comments from someone else as well.

@mmerickel mmerickel added this to the 1.6 milestone Feb 6, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants