In addition to the excellent points made by steventhedev and koper:
user.password = await hashPassword(user.password);
Just this one line of code alone is wrong.
- It’s unclear, but quite likely that the type has changed here. Even in a duck typed language this is hard to manage and often leads to bugs.
- Even without a type change, you shouldn’t reuse an object member like this. Dramatically better to have password and hashed_password so that they never get mixed up. If you don’t want the raw password available after this point, zero it out or delete it.
- All of these style considerations apply 4x as strongly when it’s a piece of code that’s important to the security of your service, which obviously hashing passwords is.
I appreciate the security concerns, but I wouldn’t consider overriding the password property with the hashed password to be wrong. Raw passwords are typically only needed in three places: user creation, login, and password reset. I’d argue that having both password and hashedPassword properties in the user object may actually lead to confusion, since user objects are normally used in hundreds of places throughout the codebase. I think, when applicable, we should consider balancing security with code maintainability by avoiding redundancy and potential confusion.
When is the hashed password needed other than user creation, login or password resets? Once you have verified the user you should not need it at all. If anything storing it on the user at all is likely a bad idea. Really you have two states here - the unauthed user which has their login details, and an authed user which has required info about the user but not their password, hashed or not.
Personally I would construct the user object from the request after doing auth - that way you know that any user object is already authed and it never needs to store the password or hash at all.
Perhaps I was unclear. What I meant to say is that, whenever possible, we shouldn’t have multiple versions of a field, especially when there is no corresponding plaintext password field in the database, as is the case here.
And they were arguing the same - just renaming the property rather than reusing it. You should only have one not both but naming them differently can make it clear which one you have.
But here I am arguing to not have either on the user object at all. They are only needed at the start of a request and should never be needed after that point. So no point in attaching them to a user object - just verify the username and password and pass around user object after that without either the password or hash. Not everything needs to be added to a object.
I agree. The field shouldn’t have been called ‘password’ in the first place, but rather ‘plaintextPassword’ or similar. That makes the code much more readable, if at a glance I know if I’m dealing with the hash or the plaintext version.
I absolutely agree. An even better structure wouldn’t have a raw password field on the user object at all.
Figuring out what the code is doing is not the hard part. Documenting the reason you want it to do that (domain knowledge) is the hard part.
Agreed.
And sometimes code is not the right medium for communicating domain knowledge. For example, if you are writing code the does some geometric calculations, with lot of trigonometry, etc. Even with clear variable names, it can be hard to decipher without a generous comment or splitting it up into functions with verbose names. Sometimes you really just want a picture of what’s happening, in SVG format, embedded into the function documentation HTML.
Yeah. I advocate for self explanatory code, but I definitely don’t frown upon comments. Comments are super useful but soooo overused. I have coworkers that aren’t that great that would definitely comment on the most basic if statements. That’s why we have to push self explanatory code, because some beginners think they need to say:
//prints to the console console.log("hello world");I think by my logic, comments are kind of an advanced level concept, lol. Like you shouldn’t really start using comments often until you’re writing some pretty complex code, or using a giant codebase.
Comments are super useful but soooo overused
I think overusing comments is a non-issue. I’d rather have over-commented code that doesn’t need it, over undocumented code without comments that needs them. If this over-commenting causes some comments to be out of date, those instances should hopefully be obvious from the code itself or the other comments and easily fixed.
I understand what you’re saying and I mostly agree, but those few instances where a line of code is only slightly different and the comment is the same, can really be confusing.
Sometimes when I don’t leave comments like that, I get review comments asking what the line does. Code like
ThisMethodInitsTheService()with comments like “what does this do?” in the review.So now I comment a lot. Apparently reading code is hard for some people, even code that tells you exactly what it does in very simple terms.
Fair. I guess in this case, it’s a manner of gauging who you’re working with. I’d much rather answer a question once in a while than over-comment (since refactors often make comments worthless and they’re so easy to miss…), but if it’s a regular occurrence, yeah it would get on my nerves. Read the fuckin name of the function! Or better yet go check out what the function does!
Worse, refactors make comments wrong. And there is nothing more annoying then having the comment conflict with the code. Which is right? Is it a bug or did someone just forget to update the comments… The latter is far more common.
Comments that just repeat the code mean you now have two places to update and keep in sync - a pointless waste of time and confusion.
Yes- exactly, they make comments wrong. But comments aren’t always a waste of time, like in legacy code, or just in general code that isn’t gonna change (mathematical equations too)
Comments are not always a waste of time, but comments that repeat or tell you what the code is doing (rather than why) are a waste. For legacy code you generally don’t have comments anyway and the code is hard to read/understand.
But if you can understand the code enough to write a comment you can likely refactor the code to just make it more readable to start with.
For code that does not change generally does not need to be read much so does not need comments to describe what it is doing. And again, if you understand it enough to write a comment to explain what it is doing you can refactor it to be readable to begin with. Even for mathematical equations I would either expect the reader to be able to read them or link to documentation that describes what it is in much more detail to name the function enough that the reader can look it up to understand the principals behind it.
TempleOS: Hold my communion wine
Why the
password.trim()? Silently removing parts of the password can lead to dangerous bugs and tells me the developer didn’t peoperly consider how to sanitize input.I remember once my password for a particular organization had a space at the end. I could log in to all LDAP-connected applications, except for one that would insist my password was wrong. A
trim()or similar was likely the culprit.Another favorite of mine is truncating the password to a certain length w/o informing the user.
Saving the password truncates but validation doesn’t. So it just fails every time you try to log in with no explanation. The number of times I have seen this in a production website is too damn high.
It also can truncate on the BE side when using the damn varchar
Passwords should be hashed, not stored plain text! Hashes are always the same length so this is an immediate sign they are doing horribly insecure things with your password.
The password needs to be 8 letters long and may only contain the alphabet. Also we don’t tell you this requirement or tell you that setting the password went wrong. We just lock you out.
The reason for leaving in the
password.trim()would be one of the few things that I would ever document with a comment.Thanks for the tip. password.trim() can indeed be problematic. I just removed that line.
A quick glance and this seemed nothing to do with self documenting code and everything to do with the flaws when code isn’t strictly typed.
Code should be generated from documentation imo
Documentation should be generated from code imo
If the doco we’re talking about is specifically an API reference, then the documentation should be written first. Generate code stubs (can be as little as an interface, or include some basic actual code such as validating required properties are included, if you can get that code working purely with a generated template). Then write your actual functional implementation implementing those stubs.
That way you can regenerate when you change the doco without overriding your implementation, but you are still forced to think about the user (as in the programmer implementing your API) experience first and foremost, rather than the often more haphazard result you can get if you write code first.
For example, if writing a web API, write documentation in something like OpenAPI and generate stubs using Swagger.
Same for drivers. Generate headers from documentation and distribute it you fucking morons
Yup absolutely. I mentioned web APIs because that’s what I’ve got the most experience with, but .h files, class library public interfaces, and any other time users who are not the implementor of the functionality might want to call it, the code they’ll be interacting with should be tailored to be good to interact with.
Code should be generated from documentation generated from code
This means war
Could be
.jartoo.
