Signup form domain validation #399
Conversation
supun-io
changed the title
supun-io
left a comment
supun-io
left a comment
There was a problem hiding this comment.
NOTE: This is not fully reviewed. I have one pending question: what if the user does not want to embed the form on one of the sending domain websites? There would be no way to block that domain since all sending domains are implicitly allowed. It might have been a bad idea on our part to allow all sending domains. Needs to discuss. So, don't work further on this PR until this is resolved
| throw new UnprocessableEntityHttpException('Newsletter not found'); | ||
| } | ||
|
|
||
| $referer = $request->headers->get('referer'); |
There was a problem hiding this comment.
referer is unreliable. We have to send location.origin manually from the frontend.
| $validationResult = $this->domainValidationService->validateDomain($newsletter, $domain); | ||
|
|
||
| if (!$validationResult->isAllowed) { | ||
| $settingsUrl = $this->appConfig->getUrlApp() . '/console/' . $newsletter->getId() . '/settings'; |
There was a problem hiding this comment.
console URL uses the slug, not the ID
| name VARCHAR(255) NOT NULL, | ||
| test_sent_emails JSONB | ||
| test_sent_emails JSONB, | ||
| allowed_domains JSONB |
There was a problem hiding this comment.
I think messing with null everywhere would be a mistake. Make it not-null and set the default to an empty array
| /** | ||
| * @var string[]|null | ||
| */ | ||
| public ?array $allowed_domains; |
| /** | ||
| * @var string[]|null | ||
| */ | ||
| public ?array $allowed_domains; |
| $this->subdomain = $newsletter->getSubdomain(); | ||
| $this->created_at = $newsletter->getCreatedAt()->getTimestamp(); | ||
| $this->name = $newsletter->getName(); | ||
| $this->allowed_domains = $newsletter->getAllowedDomains() ?? []; |
There was a problem hiding this comment.
here null coalescing would not be needed
| * @var string[]|null | ||
| */ | ||
| #[ORM\Column(type: 'json')] | ||
| private ?array $allowed_domains = null; |
| // But we need the inverse: check if embedDomain is a subdomain of user's domain | ||
| // | ||
| // We fetch active domains and check in PHP for reliability | ||
| $domains = $this->createQueryBuilder('d') |
There was a problem hiding this comment.
Bad solution: can't load all domains into memory. Some accounts may have thousands of domains.
|
|
||
| namespace App\Service\Domain; | ||
|
|
||
| readonly class DomainValidationResult |
There was a problem hiding this comment.
I think the class name must be changed, since this is only about the form domain, not the sending domains
Close #317