Problem/Motivation

Access to user/{user}/security/tfa is denied depending on user entity or database types.

The $is_self = $account->id() === $target_user->id(); line in \Drupal\tfa\Controller\TfaLoginController::accessSelfOrAdmin expects id calls on both sides to be the exact same type, however depending on the storage backend, or if entity class is overridden with strict types, either side may not be the same type.

An ID call may return a numeric string or strict integer.

Steps to reproduce

Use a database backend which returns true ints.

Or, create a bundle class with id(): int method override

Proposed resolution

The equality operator should either be less strict ==, or preferably each side coerce to integer before comparison.

Remaining tasks

Implement

User interface changes

Nil

API changes

Nil

Data model changes

Nil

Issue fork tfa-3318453

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

dpi created an issue. See original summary.

dpi’s picture

Assigned: dpi » Unassigned
Status: Active » Needs review
jcnventura’s picture

Status: Needs review » Needs work

Test-only also passed, so those tests don't seem to be doing much at all. I'm OK with just the 1-line change (i.e. "The fix").

dpi’s picture

Might be because I pushed them too close together. I think DrupalCI only pulls latest, not specific commits.

It does fail locally when I checkout only the tests.

Behat\Mink\Exception\ExpectationException: Current response status code is 403, but 200 expected.

Mind pushing the tests also? theres no config only essentials test in the repo afaict. Plus the return type in test entity is important.

The critical line is tests/modules/tfa_test_user/src/Entity/TfaTestUser.php:18

jcnventura’s picture

Status: Needs work » Reviewed & tested by the community

OK. That indeed failed. Setting it to RTBC, as I've seen already what the tests are going to do, and the fix is trivial to check anyway.

  • cf5c582 committed on 2.x
    Issue #3318453 by dpi, jcnventura: Access to TFA page is denied...

  • 519e0fb committed on 8.x-1.x
    Issue #3318453 by dpi, jcnventura: Access to TFA page is denied...
jcnventura’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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