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

Change Password should verify password every time #11

Open
anomalyze opened this issue May 28, 2022 · 1 comment
Open

Change Password should verify password every time #11

anomalyze opened this issue May 28, 2022 · 1 comment
Labels
bug Something isn't working Security Things that are or will be security relevant

Comments

@anomalyze
Copy link

Hey, I was just looking through your code. I noticed that you have a verify_password function and a change_password function.

It would probably be more ideal from a security perspective if you called the verify_password function inside the change_password function. This will allow you to reuse your change_password function (when you start to implement multiple ways to access the system. I.e. Website, terminal, etc.) It'll prevent you from forgetting to add a verification into every implementation.

I noticed when you create a new user, you use the change_password function as well here:
https://github.com/MTRNord/erooster/blob/b7c1da4c3fba816b3fc25e94a92d2b7826b283ac/src/cmds/eroosterctl.rs#L270

If you wanted to refactor it you could add the password field to your add_user SQL query/function. then add in a current_password parameter to your change_password function, and then call verify_password inside change_password.
After all it makes more sense, since you aren't 'changing' a password that hasn't been set.

I hope that makes sense. This is an interesting project and I'd be happy to contribute if you're accepting PR's.

@MTRNord MTRNord added bug Something isn't working Security Things that are or will be security relevant labels May 28, 2022
@MTRNord MTRNord assigned MTRNord and unassigned MTRNord May 28, 2022
@MTRNord
Copy link
Collaborator

MTRNord commented May 28, 2022

Hi thanks for noting that :) This makes sense for sure.

I am certainly happy to accept PR's 👍

But I think while I am at it I probably can combine this with #9 at least in parts. Aka moving the current auth to some trait based abstraction which then calls db stuff in it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Security Things that are or will be security relevant
Projects
None yet
Development

No branches or pull requests

2 participants