signebedi / libreforms-fastapi

FastAPI implementation of the libreForms spec
GNU Affero General Public License v3.0
1 stars 1 forks source link

by default, exclude the user's password from the return value of the create_user API call #257

Open rl-utility-man opened 1 month ago

rl-utility-man commented 1 month ago

the return value of the create_user API call is useful in logging and trouble shooting. There is reason why the results might be both kept in persistent logs and reason why those logs might be made available to a reasonably large number of people for debugging, code reviewing, or auditing purposes. By default, the response.content contains the password which makes it easy for routine logging to undermine authentication since anyone with the log will be able to log into any accounts for which the users have not changed their passwords. Please exclude this information by default. I personally am likely to never see a situation in which I think that returning a password from the API as the best available option, but a boolean parameter like "respond_with_password" would be totally reasonable.

signebedi commented 1 month ago

The API route that we use to create a new user as an admin is described below. It's a POST endpoint, so URL query params are a little more limited. I suppose we could probablyimplement this by adding something like mask_user_password: bool = Query(True) to the route parameters and use this bool to decide whether to strip passwords from the response ... I have not tested using Query() extensively with POST endpoints, though...

Is this really something that we need? I understand there are some circumstances where we would want to mask passwords from admins, but when an admin is creating a user or resetting their password for them, these are usually circumstances where the passwords that they create are intended to be temporary, and it is usually appropriate for them to see these passwords in order to help communicate them to the end user. If we are worried about improper access of a user account by an admin, it's important to note that all logins are logged and auditable, see #144.

https://github.com/signebedi/libreforms-fastapi/blob/33a80cf83e9d9e23e3cb8479774b5652dad624b2/libreforms_fastapi/app/__init__.py#L3007-L3121

rl-utility-man commented 3 weeks ago

There must be security best practices with respect to passwords and I defer to them. I also welcome a discussion about whether this tool -- which will largely be used inside professional organizations -- needs the highest level of password security. That said, it seems consistent with the practices of many modern IT systems to 1) design so we are reasonably certain that when username logs in, its that user and not an admin behaving badly -- which we might or might not be able to determine with an audit, and 2) keep passwords a secret that only users know. Equipping users to reset their own passwords -- which we have already done -- means that admins will rarely need to convey passwords to users. One way to split the difference would be to create create_user and create_user_return_password API routes and perhaps then let organizations disable create_user_return_password .