Update users

[calebbourg]

Description

This PR accomplishes two main goals:

• Allows a user to update their own "profile" data (name, display name, etc.)
• Allows a user to update their own password

In the case of updating a password, the Back End requires that the user's current password be provided and verified.
This is not the case for updating other user data.

GitHub Issue: [Closes|Fixes|Resolves] #your GitHub issue number here

Changes

• Provides endpoints for updating user data
• Updates Authentication middleware to redirect users to "/" rather than "/login" to match recent changes to the front end

Testing Strategy

Tested locally

[calebbourg]

Somewhere down the line I'd like to think through this specific data structure a bit more. It's somewhat unwieldy. It may make sense to just use a HashMap directly.. not sure.

[jhodapp]

I like it so far and it gives us control of the interface used on top of a HashMap. Where do you think that it's getting unwieldy more specifically?

[calebbourg]

I think it's less the data structure itself and more that the way it's implemented now, we need to explicitly wrap values in Value structs which exposes some seaORM constructs that I would prefer be hidden from this layer. We achieve semi decoupling by re-exporting the concepts through the app layers but maybe there's a better way. Not sure.

[jhodapp]

I see a lot of Rust crates reexport from underlying crate types/methods, but I agree the strongest interfaces are those that normally try to avoid this. I wouldn't call this a high priority though as long as it's not feeling overly broken and ugly to work with today.

[jhodapp]

Just a few security-related suggestions to think about. Let me know if you want to discuss any of them.

[jhodapp]

I asked GitHub Copilot for some security analysis of this method and I think what it mentions is worth considering (although I think password complexity checking may be more appropriately enforced by the frontend, but potentially both?):

The implementation of the update_password function has several key aspects that aim to ensure secure password updates, such as verifying the current password, checking the password confirmation, and hashing the new password before saving it. However, there are potential security concerns and areas for improvement:

1. Error Handling

• The error returned when confirm_password does not match password is vague (InternalErrorKind::Other). Providing a more descriptive error message (e.g., "Password confirmation does not match") could improve the developer's understanding of the issue during debugging. However, this message should not leak sensitive details when exposed to end-users.

2. Verification of current_password

• The verify_password function is used to check the current password. If this function is not implemented securely (e.g., resistant to timing attacks), it could be exploited. Ensure that verify_password uses constant-time comparison to avoid timing attacks.

3. Handling of Removed Fields

• The params.remove("current_password"), params.remove("confirm_password"), and params.remove("password") calls assume the fields exist in the params map. If any of these fields are missing, it could result in a runtime error or panic, depending on how remove is implemented. This could be exploited as a denial-of-service (DoS) vector. Adding explicit checks for the presence of these fields before removing them would make the function more robust.

4. Error Handling for Field Removal

• The params.remove("current_password")? syntax propagates an error if the field is missing. However, it seems to assume that the error will be handled appropriately elsewhere. If not handled correctly, it could result in unintended behavior or error messages leaking sensitive information.

5. Password Hashing

• The generate_hash function is used to hash the new password. Ensure that this function:
  • Uses a modern, secure hashing algorithm such as Argon2, bcrypt, or PBKDF2.
  • Is configured with appropriate parameters (e.g., cost factor) to resist brute-force attacks.
  • Includes a unique salt for each password to prevent rainbow table attacks.

6. Insertion of Hashed Password into params

• The hashed password is inserted back into params using:

  params.insert(
      "password".to_string(),
      Some(Value::String(Some(Box::new(generate_hash(password))))),
  );

  • This approach may expose the hashed password in the params map, which could be logged or leaked unintentionally. Consider directly updating the user model with the hashed password instead of storing it in params.

7. Lack of Rate Limiting

• There is no mention of rate limiting in the function or surrounding code. Without rate limiting, an attacker could brute-force the current_password by repeatedly calling this function. Consider implementing rate limiting or account lockout mechanisms after a certain number of failed attempts.

8. Audit Logging

• There is no mention of logging password update attempts. Logging these attempts (without sensitive information like raw passwords) can help detect and respond to suspicious activity.

9. Validation of New Password

• There is no indication of password strength validation. Weak passwords might be accepted, reducing security. Ensure the new password meets complexity requirements (e.g., minimum length, use of special characters, etc.).

10. Potential for Overwriting Fields

• The params map is modified throughout the function. If params contains unexpected or malicious data, it could potentially overwrite fields in the database. For example, if params includes a key that matches a column in the database, it could unintentionally update that column. Consider explicitly validating and sanitizing the params map before using it.

Recommendations

• Use explicit checks and robust error handling to avoid panics or unintended behavior.
• Ensure verify_password and generate_hash functions are secure and resistant to known attack vectors.
• Implement rate limiting and audit logging to detect and mitigate brute-force attacks.
• Validate the new password for strength and complexity requirements.
• Sanitize and validate the params map to prevent unintended updates to the database.
• Avoid storing sensitive data (like hashed passwords) in mutable structures like params that might be logged or leaked.

By addressing these concerns, the password update process can be made more secure and resilient to attacks.

[calebbourg]

This is all really great and many things I didn't think about at all. I'll go through 1 by 1

• Error Handling: Completely agree that the Other use here does not give a lot of information. I've intentionally deferred adding more descriptive variants to these errors as I would like to take a step back at some point and take inventory of all of the uses Other and see what other appropriate categories of errors there are. I wanted to avoid adding ad-hoc variants "kitchen sink" style. I'd like to address that as a separate PR and issue.

• Verification of current_password: I double checked this and our use of password_auth ensures OWASP recommended cryptography as stated in their docs:

Behind the scenes the crate uses the multi-algorithm support in the password-hash crate to support multiple password hashing algorithms simultaneously. By default it supports Argon2 (using the latest OWASP recommended parameters 8), but it can also optionally support PBKDF2 and scrypt by enabling crate features.

• Handling of Removed Fields and Error Handling for Field Removal: This is handled both upstream and downstream. Our params check will fail prior to this step if these fields are not present. If this function returns an error, that will be transformed downstream to an error response to the client. This, as far as I have thought through it, should not panic.

• Password Hashing: This is handled by password_auth

• Insertion of Hashed Password into params: Excellent point. We store this in the data structure as that is the contract between the domain layer and the entity_api layer. I think I would prefer some sort of global filtering of passwords from logs over handling it this way. Especially if this approach would require breaking these abstraction boundaries.

• Lack of Rate Limiting and Audit Logging: Love these. I think we should think through how we might implement in a way that's usable across the web interface.

• Validation of New Password: Let's discuss what our requirements would be here.

• Potential for Overwriting Fields: This is worth a larger discussion as well as the same issue exists across the entire web interface. We should look into how seaORM or Axum recommends sanitization.

[calebbourg]

I added an issue to add error variants #120

[jhodapp]

@calebbourg Would you also mind adding another new issue under the heading "Future robust password handling" and tag as security and enhancement? And then copy your comment here exactly as is as the issue body?

Maybe it's worth switching to using passkeys instead?

Thank you for responding to each one-by-one.

[jhodapp]

I like it so far and it gives us control of the interface used on top of a HashMap. Where do you think that it's getting unwieldy more specifically?