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

add PasswordApi #3289

Merged
merged 14 commits into from
Dec 12, 2016
Merged

add PasswordApi #3289

merged 14 commits into from
Dec 12, 2016

Conversation

craigh
Copy link
Member

@craigh craigh commented Dec 11, 2016

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? yes (old Utils)
Fixed tickets #3175
Refs tickets -
License MIT
Changelog updated yes

Description

add PasswordApi. fixes #3175

Todos

  • Tests
  • Documentation
  • Changelog

@craigh craigh added this to the 1.4.5 milestone Dec 11, 2016
@craigh craigh self-assigned this Dec 11, 2016
@craigh craigh requested a review from Guite December 11, 2016 14:33
Copy link
Member

@Guite Guite left a comment

Choose a reason for hiding this comment

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

Looks good so far. One aspect which came to my mind: maybe it would be advantageous to add some information to the hash methods about whether they are considered secure or not? This way we could query that information (like if (isMethodConsideredSecure($hashMethod)) and take further actions, like re-persisting the password using another method after a login happened.

@craigh
Copy link
Member Author

craigh commented Dec 11, 2016

none of the available methods are considered secure any longer. This is one reason to completely get out of the authentication business entirely (use OAuth instead). We need to upgrade all passwords to php 5.5+ methods which use bcrypt() I think. There is another ticket on this.

@craigh
Copy link
Member Author

craigh commented Dec 11, 2016

refs #2842

@Guite
Copy link
Member

Guite commented Dec 11, 2016

completely get out of the authentication business entirely

is this possible when using the internal user/password login only?

@craigh
Copy link
Member Author

craigh commented Dec 11, 2016

is this possible

no

@craigh craigh changed the title [WIP] add PasswordApi add PasswordApi Dec 12, 2016
@craigh craigh requested a review from Guite December 12, 2016 21:26
@@ -373,7 +375,7 @@ public function changePasswordAction(Request $request)
$form->handleRequest($request);
if ($form->isSubmitted() && $form->isValid()) {
$data = $form->getData();
$mapping->setPass(\UserUtil::getHashedPassword($data['pass']));
$mapping->setPass($this->container->get('zikula_zauth_module.api.password')->getHashedPassword($data['pass']));
Copy link
Member

Choose a reason for hiding this comment

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

Sometimes you use $this->get, sometimes $this->container->get. Please use $this->get only in controllers for consistency.

@@ -84,7 +84,7 @@ public function verifyAction(Request $request, $uname = null, $verifycode = null
$userEntity = $this->get('zikula_users_module.user_repository')->findOneBy(['uname' => $data['uname']]);
$mapping = $this->get('zikula_zauth_module.authentication_mapping_repository')->getByZikulaId($userEntity->getUid());
if (isset($data['pass'])) {
$mapping->setPass(\UserUtil::getHashedPassword($data['pass']));
$mapping->setPass($this->container->get('zikula_zauth_module.api.password')->getHashedPassword($data['pass']));
Copy link
Member

Choose a reason for hiding this comment

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

Same as above.

@@ -230,7 +230,7 @@ public function modifyAction(Request $request, AuthenticationMappingEntity $mapp
/** @var AuthenticationMappingEntity $mapping */
$mapping = $form->getData();
if ($form->get('setpass')->getData()) {
$mapping->setPass(\UserUtil::getHashedPassword($mapping->getPass()));
$mapping->setPass($this->container->get('zikula_zauth_module.api.password')->getHashedPassword($mapping->getPass()));
Copy link
Member

Choose a reason for hiding this comment

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

Same as above.


$hashedPass = $this->api->getHashedPassword('mybirthdayplusabunchofchanracters%&*&^53', 5); // 5 = sha1
$this->assertEquals(48, strlen($hashedPass));
$this->assertRegExp(';[0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ~@#$%^*()_+-={}|\][];', $hashedPass);
Copy link
Member

Choose a reason for hiding this comment

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

maybe put ;[0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ~@#$%^*()_+-={}|\][]; into a variable for easier reuse (avoiding duplicate strings).

Copy link
Member Author

Choose a reason for hiding this comment

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

it is only used once?

Copy link
Member

Choose a reason for hiding this comment

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

This string is used in lines 38, 43 and 48.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed!

@craigh craigh merged commit 3c54b16 into 1.4 Dec 12, 2016
@craigh craigh deleted the passwordApi branch December 12, 2016 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants