tempestphp
Authentication Layer Implementation
Following the discussion on #10 this PR will address the implementation for the Authentication Layer of the framework. This is still in an early stage, so I'll be updating the description or adding more comments as I go. I created it just to get initial feedback if I'm going in the right direction.
Can you take a look and see if I should change anything on this initial approach @brendt?
Pull Request Test Coverage Report for Build 9053148147
Details
- 2 of 22 (9.09%) changed or added relevant lines in 6 files are covered.
- No unchanged relevant lines lost coverage.
- Overall coverage decreased (-1.1%) to 84.964%
| Changes Missing Coverage | Covered Lines | Changed/Added Lines | % |
|---|---|---|---|
| src/Auth/Authenticators/GenericAuthenticator.php | 0 | 1 | 0.0% |
| src/Auth/Exceptions/InvalidLoginException.php | 0 | 1 | 0.0% |
| src/Auth/AuthenticatorInitializer.php | 0 | 2 | 0.0% |
| src/Auth/Authenticators/DatabaseAuthenticator.php | 0 | 16 | 0.0% |
| <!-- | Total: | 2 | 22 |
| Totals | |
|---|---|
| Change from base Build 9027153256: | -1.1% |
| Covered Lines: | 1294 |
| Relevant Lines: | 1523 |
💛 - Coveralls
@brendt I worked on the changes from the previous comments and created a really simple and minimalist implementation for the DatabaseAuthenticator, can you take a look and let me know other things that you want the auth layer to provide? Also I created a HasIdentity trait that can be used in a model, for example in a User class:
final class User implements Identifiable, Model
{
use HasIdentity;
}
That would already set this as the model used for authentication and by default it would use the email and password fields for the authentication.
To customize this, it would need to simply provide new values for the IDENTIFIER and SECRET constants:
final class User implements Identifiable, Model
{
use HasIdentity;
protected const string IDENTIFIER = 'username';
protected const string SECRET = 'secret';
}
I'm not encrypting anything here, do you think we should offer encrypting the password to be transparent for the devs?
I'm not encrypting anything here, do you think we should offer encrypting the password to be transparent for the devs?
Definitely, passwords should always be encrypted and devs shouldn't worry about it
I'm not encrypting anything here, do you think we should offer encrypting the password to be transparent for the devs?
Definitely, passwords should always be encrypted and devs shouldn't worry about it
To clarify, passwords should be hashed + salted, not just encrypted. 🙂
This is probably worth a separate PR for that functionality.
@brendt I worked on the things that you mentioned and also added an example of how the usage of the Auth layer would look with the current implementation in the app folder.
Please let me know if things are looking good and what should be the next steps!
To clarify, passwords should be hashed + salted, not just encrypted. 🙂
The password_hash function already uses a salt.
To clarify, passwords should be hashed + salted, not just encrypted. 🙂
This is probably worth a separate PR for that functionality.
True, sorry for the confusion!
I think we can use PHP's hashing library? I don't see a need to add a layer on top of it for now tbh
Oh damn... I knew I had typed the wrong alias for some git commands I screwed the git history for this branch 😢
@brendt can you take a look and check if the implementation is going in the right direction now and what should we improve? I can fix the git history before the merge if needed.
Hey @brendt I worked on some changes, please let me know which approach you want to follow for the AuthenticationCall interface. The other observations that you made I think I addressed all of them.
Regarding the Git History, when this looks good and no other changes are needed I'll create a new PR with it fixed.
Gonna merge this in a feature branch, and finetune it on there 👍 Thanks for the help, and sorry it took so long to get merged