Problem/Motivation

While #1845004: Replace custom password hashing library with PHP password_hash() is postponed, at least fix the numerous documentation code style problems in PhpassHashedPassword.

Proposed resolution

Remaining tasks

Review.

User interface changes

None.

API changes

None.

Data model changes

None.

Comments

znerol’s picture

Status: Active » Needs review
StatusFileSize
new4.14 KB
jhodgdon’s picture

Status: Needs review » Needs work

Thanks! This almost all looks like improvements to the docs. One minor problem:

+++ b/core/lib/Drupal/Core/Password/PhpassHashedPassword.php
@@ -95,7 +95,8 @@ protected function base64Encode($input, $count) {
   /**
-   * Generates a random base 64-encoded salt prefixed with settings for the hash.
+   * Generates a random base 64-encoded salt prefixed with settings for the
+   * hash.
    *

The beginning of a doc block is supposed to be a one line sentence, so this needs to be shortened rather than wrapped.

I suggest changing the end wording to "... with hash settings" instead of "with settings for the hash"?

neclimdul’s picture

Issue tags: +Novice
neetu morwani’s picture

Assigned: Unassigned » neetu morwani
neetu morwani’s picture

StatusFileSize
new2.01 KB
neetu morwani’s picture

Assigned: neetu morwani » Unassigned
Status: Needs work » Needs review
hitesh-jain’s picture

Status: Needs review » Reviewed & tested by the community

Looks good ! Thanks

deepakaryan1988’s picture

Looking good to me too!!

znerol’s picture

Commit credit should go to claudiu.cristea, not to me. The only thing I did was extracting the relevant parts.

jhodgdon’s picture

+1 for RTBC, thanks all!

neetu morwani’s picture

xjm’s picture

Status: Reviewed & tested by the community » Needs work

We seem to have lost several cleanups from the original patch in #1 to #5 when making the change @jhodgdon suggested:

diff --git a/core/lib/Drupal/Core/Password/PhpassHashedPassword.php b/core/lib/Drupal/Core/Password/PhpassHashedPassword.php
index dc3aad5..2deaa24 100644
--- a/core/lib/Drupal/Core/Password/PhpassHashedPassword.php
+++ b/core/lib/Drupal/Core/Password/PhpassHashedPassword.php
@@ -44,7 +44,7 @@ class PhpassHashedPassword implements PasswordInterface {
   protected $countLog2;
 
   /**
-   * Constructs a new password hashing instance.
+   * Constructs a new phpass password hashing instance.
    *
    * @param int $countLog2
    *   Password stretching iteration count. Specifies the number of times the
@@ -62,11 +62,11 @@ function __construct($countLog2) {
    *
    * @param string $input
    *   The string containing bytes to encode.
-   * @param int $count
+   * @param Integer $count
    *   The number of characters (bytes) to encode.
    *
    * @return string
-   *   Encoded string.
+   *   Encoded string
    */
   protected function base64Encode($input, $count) {
     $output = '';
@@ -95,8 +95,7 @@ protected function base64Encode($input, $count) {
   }
 
   /**
-   * Generates a random base 64-encoded salt prefixed with settings for the
-   * hash.
+   * Generates a random base 64-encoded salt prefixed with hash settings.
    *
    * Proper use of salts may defeat a number of attacks, including:
    *  - The ability to try candidate passwords against multiple hashes at once.
@@ -119,11 +118,11 @@ protected function generateSalt() {
   /**
    * Ensures that $count_log2 is within set bounds.
    *
-   * @param int $count_log2
+   * @param integer $count_log2
    *   Integer that determines the number of iterations used in the hashing
    *   process. A larger value is more secure, but takes more time to complete.
    *
-   * @return int
+   * @return integer
    *   Integer within set bounds that is closest to $count_log2.
    */
   protected function enforceLog2Boundaries($count_log2) {
@@ -145,16 +144,16 @@ protected function enforceLog2Boundaries($count_log2) {
    * for an attacker to try to break the hash by brute-force computation of the
    * hashes of a large number of plain-text words or strings to find a match.
    *
-   * @param string $algo
+   * @param String $algo
    *   The string name of a hashing algorithm usable by hash(), like 'sha256'.
-   * @param string $password
+   * @param String $password
    *   Plain-text password up to 512 bytes (128 to 512 UTF-8 characters) to
    *   hash.
-   * @param string $setting
-   *   An existing hash or the output of $this->generateSalt(). Must be at least
-   *   12 characters (the settings and salt).
+   * @param String $setting
+   *   An existing hash or the output of $this->generateSalt().  Must be
+   *   at least 12 characters (the settings and salt).
    *
-   * @return string
+   * @return String
    *   A string containing the hashed password (and salt) or FALSE on failure.
    *   The return string will be truncated at HASH_LENGTH characters max.
    */
@@ -201,14 +200,11 @@ protected function crypt($algo, $password, $setting) {
   }
 
   /**
-   * Parses the log2 iteration count from a stored hash or setting string.
-   *
-   * @param string $setting
-   *   An existing hash or the output of $this->generateSalt(). Must be at least
-   *   12 characters (the settings and salt).
+   * Parse the log2 iteration count from a stored hash or setting string.
    *
-   * @return int
-   *   The log2 iteration count.
+   * @param String $setting
+   *   An existing hash or the output of $this->generateSalt().  Must be
+   *   at least 12 characters (the settings and salt).
    */
   public function getCountLog2($setting) {
     return strpos(static::$ITOA64, $setting[3]);

Let's add back those other cleanups? Thanks!

znerol’s picture

Oh right, patch in #5 is half the size as #1.

sumitmadan’s picture

Status: Needs work » Needs review
StatusFileSize
new4.14 KB
new679 bytes

Interdiff to #1.

znerol’s picture

#14 seems to be identical to #1, did you maybe upload the wrong patch?

sumitmadan’s picture

StatusFileSize
new4.13 KB

Oops!! My Bad!!!

znerol’s picture

Status: Needs review » Reviewed & tested by the community

Perfect, thanks! Note to committers: interdiff in #14 is for the patch in #16 and applies to #1.

xjm’s picture

Status: Reviewed & tested by the community » Fixed

Great, thanks for the quick update!

This issue only changes documentation, so per https://www.drupal.org/core/beta-changes, this can be completed any time during the Drupal 8 beta phase. Committed and pushed to 8.0.x. I added claudiu.cristea to the commit message as per #9.

  • xjm committed 2f3c868 on 8.0.x
    Issue #2511806 by claudiu.cristea, sumitmadan, znerol, neetu morwani,...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.