misuse of API … call with `true` deletes this object's data

[ergohack]

Description (*)

misuse of API, ... When true, the resultant getData('before_auth_url',true) call shifts the context to the specified field, expecting it to be an array. The previous Data context is replaced. In this case, the field is not an array and all of the previous data on this object now returns NULL. I don't believe loss of this data, and all of the other fields was the intended behavior.

In my case, all subsequent requests for the redirect URL failed, returning a NULL redirect.

diff ../magento2/app/code/Magento/Customer/Model/Account/Redirect.php  ./html/vendor/magento/module-customer/Model/Account/Redirect.php
147c147
<             $result->setUrl($this->session->getBeforeAuthUrl(true));
---
>             $result->setUrl($this->session->getBeforeAuthUrl());

Fixed Issues (if relevant)

unknown -- I needed this change to fix my deployment

Manual testing scenarios (*)

I would encounter this by signing out and then signing in, ... The account would sign in correctly, but the configured redirect page was incorrectly cleared to NULL.

Contribution checklist (*)

• [√] Pull request has a meaningful description of its purpose
• [√] All commits are accompanied by meaningful commit messages
• [n/a] All new or changed code is covered with unit/integration tests (if applicable)
• [n/a] README.md files for modified modules are updated and included in the pull request if any README.md predefined sections require an update
• [unknown] All automated tests passed successfully (all builds are green)

Resolved issues:

1. [x] resolves magento/magento2#38802: misuse of API … call with true deletes this object's data

[m2-assistant[bot]]

Hi @ergohack. Thank you for your contribution! Here are some useful tips on how you can test your changes using Magento test environment.

Add the comment under your pull request to deploy test or vanilla Magento instance:

• @magento give me test instance - deploy test instance based on PR changes
• @magento give me 2.4-develop instance - deploy vanilla Magento instance

:exclamation: Automated tests can be triggered manually with an appropriate comment:

• @magento run all tests - run or re-run all required tests against the PR changes
• @magento run <test-build(s)> - run or re-run specific test build(s) For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names.

Allowed build names are:

1. Database Compare
2. Functional Tests CE
3. Functional Tests EE
4. Functional Tests B2B
5. Integration Tests
6. Magento Health Index
7. Sample Data Tests CE
8. Sample Data Tests EE
9. Sample Data Tests B2B
10. Static Tests
11. Unit Tests
12. WebAPI Tests
13. Semantic Version Checker

You can find more information about the builds here :information_source: Run only required test builds during development. Run all test builds before sending your pull request for review.

---

For more details, review the Code Contributions documentation. Join Magento Community Engineering Slack and ask your questions in #github channel.

[engcom-Hotel]

@magento create issue

[ihor-sviziev]

Hi @ergohack, From what I see, the method returns a redirect result object that redirects the client to the previous page where he/she was. After the redirect, it's logical that this information should be removed.

Could you please provide some steps to reproduce or a use case explaining why it's working incorrectly in your case?

[ergohack]

What I am seeing, without the proposed change, is that when a customer logs in, the redirect URL, for a successful login is cleared and returns NULL. With the proposed change, the redirect URL, after a successful login is preserved and the customer is taken to the correct post‑login page.

While I was debugging to find out why the post‑login page was NULL I found the following:

    app/code/Magento/Customer/Model/Account/Redirect.php:147 in function getRedirect()
    $result->setUrl($this->session->getBeforeAuthUrl(true));

became

    lib/internal/Magento/Framework/DataObject.php:395 in function __call($method, $args)
    return $this->getData($key = 'before_auth_url', $index = true);

inside the DataObject::getData($key, $index) function (starting at line 139), with $index === true, a boolean value and not null or an expected string|int value, the the function can only return NULL. $data[(boolean)true] is never set. It is nonsensical to pass a boolean value of true as the $index parameter. The commented intention of the getData($key, $index) function is that if a value for $index is provided; it is expecting an addressable named sub‑array within the data array to return.

        if ($index !== null) {
            if ($data === (array)$data) {
                $data = isset($data[$index]) ? $data[$index] : null;
            } elseif (is_string($data)) {
                $data = explode(PHP_EOL, $data);
                $data = isset($data[$index]) ? $data[$index] : null;
            } elseif ($data instanceof \Magento\Framework\DataObject) {
                $data = $data->getData($index);
            } else {
                $data = null;
            }
        }

The overt objective of the call to getBeforAuthUrl() is to get the underlying value from the data (e.g. $this‑>$data['before_auth_url']), which is a string value. With the $index parameter passed, the lookup of the string value is completely overlooked, in an attempt to return a non‑existent array node (e.g. $this‑>$data['before_auth_url'][(boolean)true]). Thus the higher call to $result‑>setUrl() always clears the redirect URL even though the underlying data has an intended value.

BTW, if the above analysis is correct, then app/code/Magento/Customer/Controller/Account/Confirm.php:253 should also be changed from: $successUrl = $this->session->getBeforeAuthUrl(true); to: $successUrl = null; If not the intended behavior, this is the effective behavior.

[ihor-sviziev]

@ergohack, It looks like you didn't get exactly how it works. The session objects have another structure for getters. It extends DataObject and changes the signature, which leads to confusion. Unfortunately, due to backward compatibility reasons, I don't think we can change that behavior.

In sessions, the first is key, 2nd one if we need to clean the value: https://github.com/magento/magento2/blob/7a9d77f8e657ba70fa1b6a3a04d67ecd7119580b/lib/internal/Magento/Framework/Session/Storage.php#L70-L77

Do you have some specific use case that we can reproduce on a clean Magento installation?

[ergohack]

No, I think I am in a non-standard state with my deployment. If my debugging is correct, I do not know why the session override for getData() is not being called.

I'll close this pull request.