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

Avoided suggestion of plain text database password in sessions topic. #15276

Merged

Conversation

spookylukey
Copy link
Member

The example code as written suggests that we are storing password directly in the database, instead of hashed etc. Although this is example code for sessions documentation, not auth, and is labelled as "simplistic", people have a habit of copying any code they see. We can avoid the suggestion by using a made up check_password(password) method instead of member.password == password.

@kezabelle
Copy link
Contributor

kezabelle commented Jan 3, 2022

I think changing this may make sense, but in isolation it maybe doesn't work because the "simplistic" example doesn't expound on what the "member" object shape is. For the proposed change to work it'd need to be subclassing AbstractBaseUser I think? If we can re-word the preceding sentence to actively describe what the "member" object is (e.g. describing it as a custom user model, or simply pointing out that it supposedly inherits from AbstractBaseUser) then it's a more cohesive thing to copy.

What do you think?

@spookylukey
Copy link
Member Author

I think changing this may make sense, but in isolation it maybe doesn't work because the "simplistic" example doesn't expound on what the "member" object shape is. For the proposed change to work it'd need to be subclassing AbstractBaseUser I think? If we can re-word the preceding sentence to actively describe what the "member" object is (e.g. describing it as a custom user model, or simply pointing out that it supposedly inherits from AbstractBaseUser) then it's a more cohesive thing to copy.

What do you think?

I wasn't thinking specifically of AbstractBaseUser (although it helps that if someone searches for check_password they will get to an implementation that doesn't include storing the password). The code here invents both Member and its password attribute (a field or other property) without defining either at all, because it's not relevant to the example. In that case, we're free to invent anything, and inventing a model that has a check_password() method is less problematic than one that has a password attribute.

@charettes
Copy link
Member

charettes commented Jan 3, 2022

I agree with Luke here, I don't feel like inventing a check_password method complexifies the example given how little context is given around the shape of member.

FWIW the previous example that relied on = was also wrong with regards to not using a constant time string comparison function.

@felixxm
Copy link
Member

felixxm commented Jan 4, 2022

@spookylukey Thanks 👍

I agree with Luke and Simon, this is an example in topics not an instruction in how-to, so we don't need to describe the Member class in detail.

@felixxm felixxm merged commit ccafad2 into django:main Jan 4, 2022
@felixxm felixxm changed the title Avoided suggestion of plain-text database password in docs Avoided suggestion of plain text database password in sessions topic. Jan 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants