Updated: Comment #N

Problem/Motivation

Now that #2036259: Move $drupal_hash_salt to settings() is in. We can remove the call to drupal_get_hash_salt() in CsrfTokenGenerator and just inject settings instead. Added bonus; we can also remove the if (function_exists(...)) hack in CsrfTokenGeneratorTest. Nice.

Proposed resolution

Inject settings into the csrf_token service, and call settings->get('hash_salt') in the get() method instead.

Remaining tasks

Patch

User interface changes

None

API changes

None

CommentFileSizeAuthor
#7 interdiff-2201919-7.txt2.79 KBdamiankloip
#7 2201919-7.patch4.88 KBdamiankloip
#1 2201919.patch3.39 KBdamiankloip
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
FileSize
2201919.patch3.39 KB
damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Status: Active » Needs review
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community

Perfect cleanup!

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented

drupal_get_hash_salt() throws an exception on an empty hash salt, which increases security as it does not allow misconfiguration. Are we fine with dumping that?

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Fixed » Needs work

Er, no we're not. Reverted.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Status: Needs work » Needs review
FileSize
2201919-7.patch4.88 KB
interdiff-2201919-7.txt2.79 KB

Spoke to catch on IRC; Let's add this logic to the CsrfTokenGenerator for now.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Related issues: +#2207585: Find a new OO home for drupal_get_hash_salt()

Created that: #2207585: Find a new OO home for drupal_get_hash_salt()

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Status: Needs review » Closed (duplicate)

Most of this is now obsolete, an the one small change can be rolled into #2207585: Find a new OO home for drupal_get_hash_salt(). As the usage example with the conversion.