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

NEXT-8593 - Customer impersonation #3713

Open
wants to merge 28 commits into
base: trunk
Choose a base branch
from

Conversation

sobyte
Copy link
Contributor

@sobyte sobyte commented May 14, 2024

1. Why is this change necessary?

This PR continues the work of #3278.

2. What does this change do, exactly?

See #3278

3. Describe each step to reproduce the issue or behaviour.

See #3278

4. Please link to the relevant issues (if any).

5. Checklist

  • I have rebased my changes to remove merge conflicts
  • I have written tests and verified that they fail without my change
  • I have created a changelog file with all necessary information about my changes
  • I have written or adjusted the documentation according to my changes
  • This change has comments for package types, values, functions, and non-obvious lines of code
  • I have read the contribution requirements and fulfil them.

ugurkankya and others added 23 commits September 22, 2022 20:37
@CLAassistant
Copy link

CLAassistant commented May 14, 2024

CLA assistant check
All committers have signed the CLA.

Copy link

github-actions bot commented May 14, 2024

Fails
🚫 You should not use the same commit message multiple times
Warnings
⚠️ Please be kind and add unit tests for your new code in these files:
src/Core/Checkout/Customer/Exception/InvalidLoginAsCustomerTokenException.php
If you are sure everything is fine with your changes, you can resolve this warning.
You can run `composer make:coverage` to generate dummy unit tests for files that are not covered

@sobyte sobyte mentioned this pull request May 14, 2024
6 tasks
@ugurkankya
Copy link

@sobyte Looks good to me thanks

@ugurkankya
Copy link

@shyim Can you have a look at this? Would be happy to see this merged 💯

@AydinHassan
Copy link
Contributor

@sobyte thanks for the PR. Is it ready for review? We will most likely do that next week as much of the team is away.

@sobyte
Copy link
Contributor Author

sobyte commented May 16, 2024

@sobyte thanks for the PR. Is it ready for review? We will most likely do that next week as much of the team is away.

Yes, I think so. I've implemented the requested changes from @shyim of #3278 and added additional changes. I will try to fix the remaining errors in the pipeline as soon as possible, but that shouldn't block your review, does it?

@AydinHassan
Copy link
Contributor

@sobyte yep that's fine, just didn't want to prematurely review a large piece of work if it was still a wip :)


$newToken = $restoredCart->getToken();

$event = new CustomerLoginEvent($context, $customer, $newToken);
Copy link
Contributor

@akf-bw akf-bw May 17, 2024

Choose a reason for hiding this comment

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

@sobyte @AydinHassan
Do you think we should/could also add a new "AsCustomerLoginEvent" here so that we can differentiate/track whether the login is triggered by a standard customer login or an admin customer login?
This would help create custom logic like an “Admin-Customer Login Log” to log when a user accesses a customer account.

To do this, it would also be nice to somehow get the user for whom the token was generated here to pass the admin user to this new event so that we can identify which user has logged into this customer account

Copy link
Member

Choose a reason for hiding this comment

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

or a flag inside the event?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe also the AccountService::loginById method should be used to login the customer: https://github.com/shopware/shopware/blob/trunk/src/Core/Checkout/Customer/SalesChannel/AccountService.php#L93

As otherwise it would be possible to login to a customer, which is for example bound to a different sales channel. But in general I think it would be useful to have different events here (not sure if it would be sufficient to only have the customer id in that event).

#[Route(path: '/store-api/account/login/customer', name: 'store-api.account.login-as-customer', methods: ['POST'])]
public function loginAsCustomer(RequestDataBag $data, SalesChannelContext $context): ContextTokenResponse
{
// TODO: find better way to handle this
Copy link
Member

Choose a reason for hiding this comment

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

You can use here the DataValidator which uses under the hood Symfony Validator

$definition = new DataValidationDefinition('newsletter_recipient.opt_out');
$definition->add('email', new NotBlank(), new Email());

Copy link
Member

Choose a reason for hiding this comment

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

This would also unify the error message when something is missing

Copy link
Contributor

@akf-bw akf-bw left a comment

Choose a reason for hiding this comment

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

We can pass the userId from the initial request to the final CustomerLoginEvent.
In the CustomerLoginEvent & CustomerBeforeLoginEvent we can add a new:
?string $userId = null

@akf-bw
Copy link
Contributor

akf-bw commented May 23, 2024

@sobyte to continue as fast as possible i extended your PR into #3727.
I added all the additional comments from @shyim & @aragon999 to the new PR & reworked some parts of it.
Also all tests are now fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants