Refactor password checker, add extender

[askvortsov1]

Part of #1891

Changes proposed in this pull request:

• Deprecate CheckingPassword event
• Use callbacks for registering password checkers
• Add extender for adding/removing password checkers
• Add integration tests for password checkers

Reviewers should focus on:

• Anything I missed?

Confirmed

• Frontend changes: tested on a local Flarum installation.
• Backend changes: tests are green (run composer test).

[luceos]

What's the benefit of not immediately returning false here. In any future iteration the value will only be set to true if true anyway. Allowing more iteration will only cost us performance here.

[askvortsov1]

Do you mean returning true? It's because an output of false overrides any outputs of true, regardless of order of evaluation. Unless order is a feature (as with middleware), I'm trying to maximally avoid order of evaluation mattering.

As such, an explicit false, denies, an explicit true allows as long as there are no explicit denials, and no trues means deny.

[clarkwinkelmann]

So if I understand correctly, we need at least one return to be === true and none === false for checkPassword to return true.

I'm assuming it's so checks can return null without impacting the result if they have neither success nor failure.

[askvortsov1]

To clarify, checkers should return as follows:

• If the password is valid, return true
• If not, return null
• If the request must be blocked, return false

This is what's currently used in Clark's extensions. Before I merge this in, is this schema fine with everyone?

[clarkwinkelmann]

Maybe we could offer CONST aliases just like we did for policies? I'm ok with the raw value, but maybe it would be a bit easier in the documentation if each of these option had a human readable constant associated with it?

[askvortsov1]

Hmm, I'm not sure. They'd have to be properties of the User object. More importantly though, I have no idea what to call the null one. User::PASSWORD_VALID and User::PASSWORD_INVALID make sense, but maybe User::PASSWORD_SKIP?

[clarkwinkelmann]

Not sure 🙃

Maybe I'd call them something including AUTHORIZE, FAIL_AUTHORIZATION and something with PASS/SKIP/NEXT?

[clarkwinkelmann]

Also, technically you don't really need to return null ever. It's also what happens when you don't return anything so there might not be a need for a constant.

[askvortsov1]

I think that this might make things more confusing to be completely honest. Let's keep it as is for now; if we see confusion in the developer community we can always add aliases later I think?

Also, technically you don't really need to return null ever. It's also what happens when you don't return anything so there might not be a need for a constant.

True, I'll try to clarify this in the docblock

[franzliedke]

Code looks good, mostly. Thanks!

[franzliedke]

On a higher level: This feels a bit too oriented around the old event, but I'd like us to focus on use cases with the extenders. What are we (extension authors) hoping to achieve when using this extender?

Doesn't changing the password checker usually mean we'd need to change the hasher as well? We might need an abstraction around this combination of actions: persisting and checking passwords. (PHP's password hashing abstractions also adds a rehash check to the mix.)

[askvortsov1]

Well, I'd agree with that point. This reminds me most of Django's authentication backends: https://docs.djangoproject.com/en/3.0/topics/auth/customizing/. We could expand the scope of the extender to entail that?

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. We do this to keep the amount of open issues to a manageable minimum.
In any case, thanks for taking an interest in this software and contributing by opening the issue in the first place!

[askvortsov1]

Shoo!

[askvortsov1]

@clarkwinkelmann The only examples of real-life use of this event are in your extensions, so congrats, you're hereby subpoenaed as an expert witness 😛!

I agree with Franz that the password checker and hasher are related concepts. But from what I'm seeing in https://github.com/clarkwinkelmann/flarum-ext-passwordless/blob/230ef8a5602bb4a72a25a42f50823d142d44f932/src/Listeners/CheckPassword.php and https://github.com/migratetoflarum/old-passwords/blob/d6b78d2ac6cd609d41e5df1af69c548bcda920f7/src/Listeners/CheckPassword.php, it's not quite that direct. IMO, we have a few options:

• Add methods to the extender to change the password hasher, this might be a good idea regardless (but what if multiple extensions do it at once?)
• Combine the hasher and password checker into a "password backend" driver. Not sure I like this idea though
• Go a step beyond, and substitute the whole "checking password" method with a Django-esque auth driver. But honestly that's pretty similar to what we have now.

When it comes to the evaluation logic, I'm fine dropping the "priorities of approve/deny" if we replace this to use insertBefore and insertAfter like with middleware.

[clarkwinkelmann]

Indeed, seems like I'm the only one to have attempted to extend this.

One use case is definitely replacing the default hasher. But this is probably a rare use case, since the default is very sensible.

On the hasher front, I'd suggest the following, if that's not already the case: allow customizing the hasher of the Laravel Hash object with a config value (assuming this is customizable with a constructor parameter), and have the hasher in the container (that's already the case).

This offers a lot of flexibility and an extension can always replace the hasher in the container if they wish so. No need to build any extender or configuration UI for the password hasher IMO.

---

The other use case is the one I have used in these two occasions: allowing additional things to be used as password, which aren't in the password column, and possibly not in the database at all.

I think the proposed solution in this PR looks good. I don't think we really need priority to be customizable. Ability to add and remove should be enough. Having the default check be in the stack also allows extensions to use this path if they want to replace the default hasher with something that's not plug-and-play into the Laravel Hasher.

[clarkwinkelmann]

Any reason ContainerUtil::wrapCallback is called in the provider and not in the extender? Are we consistent across extenders with that way of doing things?

[clarkwinkelmann]

@askvortsov1 was something holding this back?

Should I do a final review now or wait for something else?

[askvortsov1]

I don't believe so, should be good to go

[askvortsov1]

Merge conflicts resolved, this should be good to review.

[clarkwinkelmann]

I have not tested locally, but it's looking all good to me.

I'll try to update my old-passwords extension before release to actually test it with the new system once this is merged to master.

I think you will want to update the deprecation comment before merging? It's the only thing I noticed.