Inconsistent usage of user id vs login/name in plugin method call signature

BUG/PROBLEM REPORT (OR OTHER COMMON ISSUE)

What I did:

I was implementing the Products.PluggableAuthService.interfaces.plugins.ICredentialsUpdatePlugin interface for plugin. Specifically I was interpreting the arguments to the updateCredentials() method.

What I expect to happen:

I assumed that the login argument defined in the interface means that the method receives the mutable user login/name (as opposed to the immutable user id).

What actually happened:

I first implemented it per the assumption above. Then in a real world test (a Plone Volto project), I saw that the method was receiving the immutable user id in at least one case. I grepped the source of all installed dists and found that real code is passing in both logins and user ids in various places including within Products.PluggableAuthService. I doubt this one method in this one interface is the only case of this issue, so an audit is probably in order.

What version of Python and Zope/Addons I am using:

$ find -L . -type f \( -name \*.py \) -not -ipath '*test*' -exec grep --color=auto -nH --null -e \\.updateCredentials\( \{\} +
./Products.PlonePAS-6.0.8-py3.9.egg/Products/PlonePAS/pas.py\0359:    self.updateCredentials(request, response, login, new_password)
./Products.PlonePAS-6.0.8-py3.9.egg/Products/PlonePAS/plugins/cookie_handler.py\0106:            user_pas.updateCredentials(request, response, login, password)
./Products.PlonePAS-6.0.8-py3.9.egg/Products/PlonePAS/plugins/cookie_handler.py\0114:                pas_instance.updateCredentials(request, response, login,
./Products.CMFPlone-5.2.6-py3.9.egg/Products/CMFPlone/browser/login/password_reset.py\096:            plugin.updateCredentials(
./Products.PluggableAuthService-2.6.4-py3.9.egg/Products/PluggableAuthService/PluggableAuthService.py\01153:            updater.updateCredentials(request, response, login, new_password)
./Products.PluggableAuthService-2.6.4-py3.9.egg/Products/PluggableAuthService/events.py\097:    pas.updateCredentials(
./Products.PluggableAuthService-2.6.4-py3.9.egg/Products/PluggableAuthService/plugins/CookieAuthHelper.py\0279:            pas_instance.updateCredentials(request, response, login, password)

Ross Patterson wrote at 2021-12-26 10:33 -0800:

BUG/PROBLEM REPORT (OR OTHER COMMON ISSUE)

What I did:

I was implementing the Products.PluggableAuthService.interfaces.plugins.ICredentialsUpdatePlugin interface for plugin. Specifically I was interpreting the arguments to the updateCredentials() method.

What I expect to happen:

I assumed that the login argument defined in the interface means that the method receives the mutable user login/name (as opposed to the immutable user id).

This is the intended interpretation: updateCredentialsis supposed to interact with extractCredentials The latter one does not yet know about user ids (only user login names).

What actually happened:

I first implemented it per the assumption above. Then in a real world test (a Plone Volto project), I saw that the method was receiving the immutable user id in at least one case.

I think this use is wrong: updateCredentials should be called with the login name not the user id.

This is the intended interpretation: updateCredentialsis supposed to interact with extractCredentials The latter one does not yet know about user ids (only user login names). I think this use is wrong: updateCredentials should be called with the login name not the user id.

Sure, but I'm not sure how this addresses the issue being reported?

real code is passing in both logins and user ids in various places including within Products.PluggableAuthService

Did you intend that as clarification/direction for whoever tackles this issue?

Ross Patterson wrote at 2021-12-26 11:31 -0800:

This is the intended interpretation: updateCredentialsis supposed to interact with extractCredentials The latter one does not yet know about user ids (only user login names). I think this use is wrong: updateCredentials should be called with the login name not the user id.

Sure, but I'm not sure how this addresses the issue being reported?

real code is passing in both logins and user ids in various places including within Products.PluggableAuthService

Did you intend that as clarification/direction for whoever tackles this issue?

In my view, it is not a PluggableAuthService issue but one of the component which passes the user id. As a consequence, it should get reported there.

You have been very vague about this component (in the context of Plone 6). When I remember right, issues related to Plone should be posted to the Plone issue tracker when it is unknown which component is responsible in the first place.

In my view, it is not a PluggableAuthService issue but one of the component which passes the user id. As a consequence, it should get reported there.

Oh, I agree that it's not just this package, which is what I intended by the phrasing "including within":

real code is passing in both logins and user ids in various places including within Products.PluggableAuthService

Regarding your feedback:

You have been very vague about this component

I certainly didn't intend to be vague. Indeed I was trying to be very specific when I included that grep output. If one looks at those specific lines in that specific version of this package I think one would see the places within where this package's code is explicitly passing in user IDs instead of user logins/names. Can you elaborate on how I should be more specific and less vague? I'd be happy to accommodate!

Regardless, I intended to capture what I found as I found it as an issue so that my observations are searchable and available to others in the place where it seemed to me where the changes needed to start. For example, if I were to end up tackling this, I'd want to start here with the internal usage that violates the contract/interface. Do you think it's valuable for developers in the community to share findings on such issues they encounter as they work?

Ross Patterson wrote at 2021-12-26 14:49 -0800:

... I certainly didn't intend to be vague. Indeed I was trying to be very specific when I included that grep output. If one looks at those specific lines in that specific version of this package I think one would see the places within where this package's code is explicitly passing in user IDs instead of user logins/names

I did not follow the link embedded in your report -- sorry!

I have seen now that it shows a place (in plone.restapi) where the cookie set by updateCredentials actually refers to the user id and not the login name. But updateCredentials itself allows that its parameter name login contains either a user id or a login name. Therefore, it does not violate the contract ("login name can be passed").

How updateCredentials implements its task is its own decision. Provided that extractCredentials (and potentially authenticateCredentials, too) are ready to work with user ids, putting the user id into the cookie may be acceptable. Because extractCredentials usually does not know user ids, it would be more natural to use the login name, however.

I have seen now that it shows a place (in plone.restapi)

Actually that link is just to a workaround to this issue that I used in a plugin. In this discussion with you, I've been referring to the grep output:

Indeed I was trying to be very specific when I included that grep output

You can find it at the bottom of my original issue description above.

Therefore, it does not violate the contract ("login name can be passed"). How updateCredentials implements its task is its own decision.

Well at the very least the interface docstrings should specify that what you describe is work a plugin is expected to do. Elsewhere in the interface docstrings, multiple references are made to the terms userid and login and they're definitely distinct. In which case, how is a reasonable reader supposed to understand what you describe from:

def updateCredentials(request, response, login, new_password):

And if a plugin is expected to do that work, how would you propose it determine which it has received in that argument?

More importantly, I strongly disagree and I would be very surprised if other developers didn't disagree as well. The reason for having an immutable userid, which is used internally for things like ZODB OFS object ownership, ACL local roles, etc., and a separate, user-facing, mutable login is so that concerns can be separated. The an individual user can change their login or the application can change from using a short user "nick" to using their email for logging in without then having to walk entire the object graph and/or re-write every object to the DB. Elsewhere in PAS interfaces and implementation, which of the two is used is very explicit and they can't be used interchangeably, appropriately IMO. For example, but not limited to, PluggableAuthService.getUser() requires a login and PluggableAuthService.getUserById() requires a userid, again appropriately. For any application that uses different userids and logins, you can't and shouldn't use them interchangeably.

So I believe the use of login for the argument name and elsewhere in the in the interfaces and implementations correctly communicates to developers that plugins are expected and required to work with the mutable user login name in those contexts. Thus implementations or calls into plugins that use immutable user ids are incorrect and broken. That at least one of those is internal to this repo and an audit for other such internal errors should be done is what this issue is reporting. This issue can also serve as a central reference point as other, external packages with this problem, such as those listed in my grep output, make the changes to fix it in their own repos.

Ross Patterson wrote at 2021-12-26 18:41 -0800:

I have seen now that it shows a place (in plone.restapi)

Actually that link is just to a workaround to this issue that I used in a plugin. In this discussion with you, I've been referring to the grep output:

Indeed I was trying to be very specific when I included that grep output

The "grep output" does not contain user_id.

... Well at the very least the interface docstrings should specify that what you describe is work a plugin is expected to do. Elsewhere in the interface docstrings, multiple references are made to the terms userid and login and they're definitely distinct. In which case, how is a reasonable reader supposed to understand what you describe from:

def updateCredentials(request, response, login, new_password):

I like very much the use of "speaking names". login is such a "speaking name": it refers to the information you provide during login -- i.e. the login name, not the internal user id.

And if a plugin is expected to do that work, how would you propose it determine which it has received in that argument?

In general, I trust that meaningful (aka "speaking") names have been used.

... So I believe the use of login for the argument name and elsewhere in the in the interfaces and implementations correctly communicates to developers that plugins are expected and required to work with the mutable user login name in those contexts.

I agree.

Thus implementations or calls into plugins that use immutable user ids are incorrect and broken.

I agree for calls into plugins: at least some implementations of upgradeCredentials will expect to get login names.

I disagree for what the implementation does internally.

That at least one of those is internal to this repo and an audit for other such internal errors should be done is what this issue is reporting.

Apparently, you have located a place where PluggableAuthService passes the user id to upgradeCretentials's login parameter. Why do you not name this place explicitely rather than posting grep output which does not mention user_id?

The "grep output" does not contain user_id.

Ah! So that's what you find "vague". Now that I understand...

...how I should be more specific and less vague...

I'm...

...happy to accommodate!

;-)

From that grep output I can read which of those are internal to this package. Here, I'll narrow it down to this package for you:

$ find -L ./ -type f \( -name \*.py \) -not -ipath '*test*' -path './Products.PluggableAuthService*' -exec grep --color=auto -nH --null -e \\.updateCredentials\( \{\} +
./Products.PluggableAuthService-2.6.4-py3.9.egg/Products/PluggableAuthService/PluggableAuthService.py1153:            updater.updateCredentials(request, response, login, new_password)
./Products.PluggableAuthService-2.6.4-py3.9.egg/Products/PluggableAuthService/events.py97:    pas.updateCredentials(
./Products.PluggableAuthService-2.6.4-py3.9.egg/Products/PluggableAuthService/plugins/CookieAuthHelper.py279:            pas_instance.updateCredentials(request, response, login, password)

I'll help you navigate from there. From the start of those lines you can see which version of this package is being used. Then you can navigate to the specific files and lines to read what values are being passed in. Then you can see that the first and third matches are user logins. You can also see that the second match is explicitly sending in the usserid. Hence the part of this bug report concerning inconsistent usage within this package.

In general, I trust that meaningful (aka "speaking") names have been used.

I am reporting that at least in one case internally you cannot trust that login is used, and that the confusion that results in wider community of consumers means you definitely cannot trust this.

I agree for calls into plugins: at least some implementations of upgradeCredentials will expect to get login names.

To clarify, I believe it's important and necessary to be explicit that correct implementations accept logins. IOW, I disagree with the ambiguity in "at least some implementations".

I disagree for what the implementation does internally.

Please share the reasoning behind your disagreement.

Why do you not name this place explicitely rather than posting grep output which does not mention user_id?

I did not fail to name that place, I captured all the specifics needed for someone tackling this issue in the future to find specific examples. The issue I'm reporting is incorrect mixed usage. The grep output shows where those invocations are done. From the output one can see a manageable number of examples, some of which use login and others use userid. This is a library intended for use in external code and not just a product/application, so in the output one can see that some are internal and others are external to demonstrate the wider confusion in the community of consumers. In the output one can see which versions of which packages and the line numbers within the code at which to read. Since the examples include the actual command used to generate the output, one can also see how to audit for this issue in other code bases or for other plugin interface methods. Since the issue is mixed usage within and without this package, pasting code blocks for even a subset of examples would make the issue unreadable. Hence I provided an efficient yet still specific form of examples. See the top of this comment for a detailed walk through of how to trace these specific examples from the output to the corresponding code blocks.

zopefoundation / Products.PluggableAuthService