FIX Generate salt if needed

[lekoala]

Description

Some Members may not have a Salt defined. Yet, it's possible to call encryptWithUserSettings nonetheless and the framework will assume a Salt exist. This is not correct, since it will fail to encrypt things properly. I don't see any reason to assume anything and not generate a Salt if needed.

Alternative option if we don't want to generate a salt: at least throw a proper exception explaining that you cannot encrypt with an empty salt instead of assuming there is a valid value there.

Manual testing steps

• On a Member without a password, but with a PasswordEncryption
• Call $member->encryptWithUserSettings('somestring')
• You get a blowfish exception because it could not encrypt with an empty salt

Issues

https://github.com/silverstripe/silverstripe-framework/issues/11159

Pull request checklist

• [x] The target branch is correct
  • See picking the right version
• [x] All commits are relevant to the purpose of the PR (e.g. no debug statements, unrelated refactoring, or arbitrary linting)
  • Small amounts of additional linting are usually okay, but if it makes it hard to concentrate on the relevant changes, ask for the unrelated changes to be reverted, and submitted as a separate PR.
• [x] The commit messages follow our commit message guidelines
• [x] The PR follows our contribution guidelines
• [x] Code changes follow our coding conventions
• [x] This change is covered with tests (or tests aren't necessary for this change)
• [x] Any relevant User Help/Developer documentation is updated; for impactful changes, information is added to the changelog for the intended release
• [x] CI is green

[GuySartorelli]

Please create an issue for this and link to it (I've added that section back to the PR template for you), so we can track this. We track issues, not PRs.

If this is indeed a bug and affects CMS 4, please target the 4.13 branch so it can be fixed there as well.

[GuySartorelli]

In your reproduction steps you have noted "On a Member without a password" - is that the only scenario where a member doesn't have a salt? If there are scenarios where a member who does have a password doesn't have a salt, will this change stop them from logging in until they change their password?

[lekoala]

i added the issue

https://github.com/silverstripe/silverstripe-framework/issues/11159

i didn't dive further than my use case, but from the code, it's clear enough:

• either a proper exception is thrown (instead of waiting bcrypt to fail because of the missing salt)
• a salt is generated

i think both options make sense, but i'd rather generate a salt unless there is a good reason not to do it there

[GuySartorelli]

I've looked a little deeper into this.

If a user does not have a salt, one is generated for them in this flow when saving the record:

1. call $member->write()
2. in $member->onBeforeWrite() if the password has changed or this is a new record, call $member->encryptPassword()
3. Member's salt is cleared, and a new one is created via Security::encrypt_password()
4. New salt is set to $member->Salt

Because of this flow, a member with no salt should only occur if that member has been created programatically and has not yet been saved to the database.

What is your use case where the above flow doesn't occur? Since there's a write() in the implemented solution it seems like you're expecting the member to be in the database, so it might just be that you need to call write() earlier than you currently are? It feels (based on the above flow) that this PR is more of a workaround than a fix but that's without knowing what you're specifically trying to achieve.

[lekoala]

@GuySartorelli well, the issue was with my admin user in dev mode, so while I fully agree that this should not happen, it technically can. I don't see the advantage of "assuming" there is a salt: either an exception is thrown (there is no salt, and it should not happen) or a salt is generated

[GuySartorelli]

By admin user do you mean the default admin account? I'll see if I can figure out what's causing that - there might be an underlying problem to solve there.

As far as the change this PR is making, it seems like if anything should happen it should throw an exception - having a write there seems unexpected, and setting a salt seems like it's just hiding an underlying problem.

[lekoala]

agreed, i updated the PR. Actually by reading the comment above, it seems the original intent is to check that both the PasswordEncryption AND the Salt are available, so that's what I did