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

EZP-30797: Implemented configuration for user password expiration #2742

Merged
merged 1 commit into from
Sep 20, 2019

Conversation

adamwojs
Copy link
Member

@adamwojs adamwojs commented Aug 16, 2019

Question Answer
JIRA issue EZP-30797
Required by
Bug/Improvement no
New feature yes
Target version 7.x, master
BC breaks no
Tests pass yes
Doc needed yes

Added new settings to ezuser field type:

  • Number of days after which password will expire
  • Number of days before password expiration when the user should be notified about password expiration

For more details see the specification attached to JIRA issue.

Significant changes in the database schema

  • Added password_updated_at column to ez_user table which stores the timestamp of last password update.

Why to store timestamp of password update instead of password expiry date?

Column password_updated_at in opposite to password_expiry_at doesn't need to be updated after changing password expiry settings.

Significant PAPI changes

  • Added \eZ\Publish\API\Repository\UserService::getPasswordInfo method
interface UserService
{
    # ...


    /**
     * Returns information about password for a given user.
     *
     * @param \eZ\Publish\API\Repository\Values\User\User $user
     *
     * @return \eZ\Publish\API\Repository\Values\User\PasswordInfo
     */
    public function getPasswordInfo(User $user): PasswordInfo;
}

and \eZ\Publish\API\Repository\Values\User\PasswordInfo structure: https://github.com/ezsystems/ezpublish-kernel/pull/2742/files#diff-8a78ba0adc00ac7ed0fe7d268b6b4e84R15-R57

  • Added \eZ\Publish\API\Repository\Values\User\User::$passwordUpdatedAt property
namespace eZ\Publish\API\Repository\Values\User;

abstract class User extends Content implements UserReference
{
    # ...

    /**
     * Datetime of last password update.
     *
     * @var DateTimeInterface|null
     */
    protected $passwordUpdatedAt;
)

TODO:

  • Implement feature / fix a bug.
  • Implement tests.
  • Fix new code according to Coding Standards ($ composer fix-cs).
  • Ask for Code Review.

@adamwojs adamwojs self-assigned this Aug 16, 2019
@adamwojs adamwojs force-pushed the ezp_30797 branch 4 times, most recently from 96c6de4 to 7e6d233 Compare August 21, 2019 12:41
@adamwojs adamwojs changed the title [WIP] EZP-30797: As an administrator, I want to configure a password expiration for users EZP-30797: As an administrator, I want to configure a password expiration for users Aug 22, 2019
Copy link
Contributor

@mikadamczyk mikadamczyk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adamwojs adamwojs force-pushed the ezp_30797 branch 2 times, most recently from 5896375 to 4ced959 Compare August 22, 2019 11:54
@adamwojs
Copy link
Member Author

Done @mikadamczyk

eZ/Publish/API/Repository/Tests/UserServiceTest.php Outdated Show resolved Hide resolved
eZ/Publish/API/Repository/Tests/UserServiceTest.php Outdated Show resolved Hide resolved
eZ/Publish/API/Repository/Tests/UserServiceTest.php Outdated Show resolved Hide resolved
eZ/Publish/API/Repository/Values/User/User.php Outdated Show resolved Hide resolved
eZ/Publish/API/Repository/Values/User/User.php Outdated Show resolved Hide resolved
'id' => $loadedUser->id,
'login' => $loadedUser->login,
'email' => $userUpdateStruct->email ?: $loadedUser->email,
'isEnabled' => $userUpdateStruct->enabled !== null ? $userUpdateStruct->enabled : $loadedUser->enabled,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'isEnabled' => $userUpdateStruct->enabled !== null ? $userUpdateStruct->enabled : $loadedUser->enabled,
'isEnabled' => $userUpdateStruct->enabled ?? $loadedUser->enabled,

'login' => $loadedUser->login,
'email' => $userUpdateStruct->email ?: $loadedUser->email,
'isEnabled' => $userUpdateStruct->enabled !== null ? $userUpdateStruct->enabled : $loadedUser->enabled,
'maxLogin' => $userUpdateStruct->maxLogin !== null ? (int)$userUpdateStruct->maxLogin : $loadedUser->maxLogin,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'maxLogin' => $userUpdateStruct->maxLogin !== null ? (int)$userUpdateStruct->maxLogin : $loadedUser->maxLogin,
'maxLogin' => $userUpdateStruct->maxLogin ?? $loadedUser->maxLogin,
``` if we could avoid casting to `int`

@mikadamczyk
Copy link
Contributor

As we discussed, method names in API could be improved and we could get rid of -1

eZ/Publish/API/Repository/Tests/UserServiceTest.php Outdated Show resolved Hide resolved
eZ/Publish/Core/FieldType/User/Type.php Outdated Show resolved Hide resolved
eZ/Publish/Core/FieldType/User/Value.php Outdated Show resolved Hide resolved
eZ/Publish/Core/Persistence/Legacy/User/Mapper.php Outdated Show resolved Hide resolved
eZ/Publish/Core/Repository/UserService.php Outdated Show resolved Hide resolved
eZ/Publish/Core/Repository/UserService.php Outdated Show resolved Hide resolved
eZ/Publish/Core/MVC/Symfony/Security/User/Provider.php Outdated Show resolved Hide resolved
@adamwojs adamwojs force-pushed the ezp_30797 branch 3 times, most recently from 1e476a3 to c9135ee Compare August 26, 2019 11:10
@adamwojs adamwojs force-pushed the ezp_30797 branch 2 times, most recently from f79ad21 to 4dc7b7c Compare August 29, 2019 09:39
@adamwojs adamwojs changed the title EZP-30797: As an administrator, I want to configure a password expiration for users EZP-30797: Implemented configuration for user password expiration Aug 29, 2019
@adamwojs adamwojs force-pushed the ezp_30797 branch 2 times, most recently from 89e6621 to 11baaed Compare August 29, 2019 11:50
@adamwojs
Copy link
Member Author

PR updated according to code review suggestions.

@barbaragr barbaragr self-assigned this Sep 6, 2019
@barbaragr
Copy link

Rebase is needed here, I suppose.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants