Had to add this so our password leak system allows me to commit.

The user is valid without a password when it's an SSO user. So there's no need to enforce a random password. Also, this random password is not shown to the caller anywhere, so it couldn't be used anyway.

After this change, The SSO users for partners don't have a password, the same as any other SSO user in the project.

Same, no password required.

why rescue instead of rely on @user.save return value?

Because the previous call to @user.activate! can raise an exception, which was uncaught.

This is to fix a mass-assignment error from PUT /admin/api/users/{id}.xml. Since the endpoint calls the update with role: :admin. The password was excluded from mass-assignment, because it only allowed the default role.

I'm not sure I understand... if there is no as: argument here, doesn't it mean that the role set in :as when assigning attributes doesn't matter?

I think it will be much better to set the STRONG_PASSWORD_MIN_SIZE based on this setting than overriding it here.

Because you said in a comment:

Nah, I don't like that. I prefer UI and backend to be in sync, the opposite would be pretty confusing for client ad for future us.

But this setting is essentially doing the same - UI will still want strong passwords but backend will allow weaker.

But this setting is essentially doing the same - UI will still want strong passwords but backend will allow weaker.

You're right on this

I think it will be much better to set the STRONG_PASSWORD_MIN_SIZE based on this setting than overriding it here.

What do you mean by "overriding here"? overriding the setting? Also, if we set STRONG_PASSWORD_MIN_SIZE based on the setting, we still need this method to check for sample data.

Yes, I meant that this specific line overrides STRONG_PASSWORD_MIN_SIZE based on a configuration setting. So my suggestion was to simplify by setting STRONG_PASSWORD_MIN_SIZE based on a configuration value instead of moving the logic here.

Agreed that we still need the sample data check. Hopefully later we get rid of it by also generating strong password for John Doe.

then we would need to get rid of john doe default password and signup_types before implementing your suggestion

We can get rid of this specific line regardless of John Doe.

I've been trying but I don't think it's worth it right now. Having the setting checked in the method is pretty convenient for tests, so I can stub the value and easily see the different behavior. However, if we move the logic to the macro, then the setting is only read once when loading the code, so stubbing the setting has no effect, which leads to a few tests failing.

I prefer to not spend the time redefining tests right now, when we actually get rid of sample data, I'll remove the validate_strong_password? method and then I'll worry about the tests.

Ok, sure. Even if it stays, it will not be a big issue.

Follow up on previous question about sample data ending up in production, I would imagine the logic to something like this.

Instead of this, is we agreed to not allow weak passwords for sample dat ain production, I'd prefer to completely remove the :sample_data signup type. I wouldn't be needed anymore.

I would be happy not having :sample_data login type. After all the idea behind sample data is to look like real data and being a separate type is not it.

Yeah, it'd be good to remove the sample data signup type, but that would imply all the additional logic: Creating Jonh Doe with a random password, send it via internal message to admin, and updating the places where that password is visible, like the CMS toolbar.

@mayorova WDYT?

I vote for keeping it like it is now - John Doe with 123456 password still being valid. I think it's good to keep the changes less disruptive for the user.

We can also generate default passwords derived from e.g., buyer id and SECRET_KEY_BASE derivative and we can present that password to the provider in the UI when viewing the user.

• provider opens the user
• system detects that this is the first default user John Doe
• system generates the default password
• checks whether it matches the user
• warns that the user has the default password of "..." OR reports that the password has changed from the default

I think this will be a good UX while keeping things way more secure.

Do we need to call this again?
As we have

      validates_presence_of :password, if: :validate_password?
      validates :password, length: { minimum: STRONG_PASSWORD_MIN_SIZE }, if: :validate_strong_password?

It means that most for when strong password is used (most of the time) validatee_password? will be called twice?

It will be called not twice, but three times, because it's also called by:

But I think that's fine because it's only called when updating a user, I don't know how common is that but doesn't seem supper common. Also, it makes the code more clear, otherwise, it would be confusing, e.g. a scenario where validate_password? is false but validate_strong_password? is true. Like for instance when you are modifying a user attribute which is not the password. What would happen then? would we validate the length of a password when that attribute is not involved in the update? Even more, password is a virtual attribute so I don't really know what would be there at the validation moment, most probably nil I guess.

This way validate_password? and validate_strong_password? always have consistent values.

This replaces password_required? Because it was pretty confusing:

• Being called from views to decide whether making password inputs required. IMO that's wrong, password inputs are required or not according to their purpose, not to some computed value.
• Being called also to decide whether validate the password or not. Which was wrong as well, since it didn't match all scenarios.

The new methods are tested and return proper values for all known scenarios.

wait, if password_digest was set but not yet in the database, shouldn't we still return true here?

It was causing issues in the user edit form. If I recall correctly, on some scenarios the form required the user to provide their current password in situations where that password was still not persisted, so it couldn't work.

I think it's correct this way. We only consider a user is using a password when it indeed has a password in DB.

Ok, fine, if not too annoying, I'd suggest renaming the method to already_using_password? to make this immediately obvious.

I renamed it everywhere except in the User liquid drop, to not break existing dev portals 8b6697b

In order to exclude "John Doe" from strong password requirement, I added a new signup type :sample_data to identify it.

The question is, shouldn't we prevent sample weak passwords in production environment? I think we discussed this somewhere, about how to notify provider of the sample buyer user account. e.g. email/internal messaging.

But I'm not sure we came to an agreement.

We discussed this via slack, I even suggested to get rid of John Doe, we agreed on keeping the sample data. About the sample data having a weak password, that's another story. I remember you mentioned about sending the password to admin via internal messaging, but nothing was agreed at the end.

Dead code AFAIK

This method determines whether the new user must be automatically activated or not. I added the :sample_data case to it. Sample user must be activated by default.

This was wrong, there's no scenario when it's required to change a password in this form.

In fact, it was a "fake" requirement, the form submit was working without provided values.