Login via social media #3154

sspooky13 · 2024-05-04T13:58:56Z

Q	A
Description, reason for the PR	Implementation of login via social media (Facebook, Google, Seznam)
New feature	Yes
BC breaks	Yes
Fixes issues	...
Have you read and signed our License Agreement for contributions?	Yes

🌐 Live Preview:

henzigo

Hello, here are some of my thoughts.

I'm missing one feature. On some platforms, you can see connected networks in your profile and you can remove this connection. It should be more likely addressed to Bussines, but I would love this feature.

henzigo · 2024-05-06T07:51:36Z

packages/framework/src/Model/SocialNetwork/SocialNetworkConfigFactory.php

+    public function createConfig(string $redirectUrl): array
+    {
+        return [
+            'callback' => $redirectUrl,
+            'providers' => [
+                self::PROVIDER_GOOGLE => [
+                    'enabled' => true,
+                    'keys' => [
+                        'id' => $this->googleClientId,
+                        'secret' => $this->googleClientSecret,
+                    ],
+                ],
+                self::PROVIDER_FACEBOOK => [
+                    'enabled' => true,
+                    'keys' => [
+                        'id' => $this->facebookClientId,
+                        'secret' => $this->facebookClientSecret,
+                    ],
+                ],
+                self::PROVIDER_SEZNAM => [
+                    'enabled' => true,
+                    'keys' => [
+                        'id' => $this->seznamClientId,
+                        'secret' => $this->seznamClientSecret,
+                    ],
+                    'scope' => 'identity',
+                    'adapter' => Seznam::class,
+                ],
+            ],
+        ];
+    }
+}


It would be nice to have this configuration via yaml in project-base. It should be possible to create some Configuration wrapper and then parse it to some structure.

The main purpose of this change is to give programmers an easier way to work with this feature and only one place where you can configure this feature.

fixed in 68ae66f

henzigo · 2024-05-06T07:54:38Z

project-base/app/config/packages/monolog.yaml

@@ -1,5 +1,5 @@
 monolog:
-    channels: ["cron", "queue", "slow"]
+    channels: ["cron", "queue", "slow", "socialNetwork"]


Is there any benefit to having a custom monolog channel only for social networks?

yep, you can filter your social network messages in kibana by, e.g.: message: socialNetwork.INFO or message: socialNetwork.ERROR etc. but yes, you can do it same filter by message 🤷🤷‍♂️🤷‍♀️. If you don't use filter this way I can remove it

I don't think this is necessary, but it's up to you

henzigo · 2024-05-06T07:56:16Z

project-base/app/config/routes/shopsys_front.yaml

+    defaults: { _controller: App\Controller\Front\SocialNetworkController::loginAction }
+    path: /social-network/login/{type}
+    requirements:
+        type: Google|Facebook|Seznam #copy from /packages/framework/src/Model/SocialNetwork/SocialNetworkConfigFactory.php


This is another part that you need to edit as a programmer if you want to add/remove any type of login. It would be nice to enable all types (or simply anything) here and check enabled types directly in controller

changed in 68ae66f

henzigo · 2024-05-06T07:58:58Z

project-base/app/src/Controller/Front/SocialNetworkController.php

@@ -0,0 +1,80 @@
+<?php


Can we move this controller into the Framework package? I know that this is primarily used for Frontend, but I don't see any benefit to having this class in project-base. This file will probably not be edited on projects.

henzigo · 2024-05-06T08:02:02Z

project-base/storefront/components/Blocks/Popup/LoginPopup.tsx

+    const [socialNetworkLoginFacebook, socialNetworkLoginGoogle, socialNetworkLoginSeznam] =
+        getInternationalizedStaticUrls(
+            [
+                { url: '/social-network/login/:type', param: 'Facebook' },
+                { url: '/social-network/login/:type', param: 'Google' },
+                { url: '/social-network/login/:type', param: 'Seznam' },
+            ],
+            url,
+        );


It would be nice to provide those data in SettingsQuery. Now you have to synchronize FE and BE separately. What if some project will want to have social logins only on some domains? Then you need to duplicate logic on both sides.

henzigo · 2024-05-06T08:02:57Z

project-base/app/src/Model/Security/LoginAsUserFacade.php

+
+    /**
+     * @param \App\Model\Customer\User\CustomerUser $customerUser
+     * @return array{accessToken: string, refreshToken: string}
+     */
+    public function loginAndReturnAccessAndRefreshToken(CustomerUser $customerUser): array
+    {
+        $deviceId = Uuid::uuid4()->toString();
+
+        return [
+            'accessToken' => $this->tokenFacade->createAccessTokenAsString($customerUser, $deviceId),
+            'refreshToken' => $this->tokenFacade->createRefreshTokenAsString($customerUser, $deviceId),
+        ];
+    }


This can be moved to the parent class.

henzigo · 2024-05-06T08:08:12Z

project-base/app/src/Model/Customer/User/RegistrationDataFactory.php

@@ -61,4 +64,25 @@ public function createWithArgument(Argument $argument): RegistrationData

        return $registrationData;
    }
+


It seems like you should be able to move this file into the Framework package easily. If you do that it will be easier to maintain this file for us.

henzigo · 2024-05-06T08:10:52Z

project-base/app/src/Component/SocialNetwork/Seznam.php

@@ -0,0 +1,63 @@
+<?php


This file should be in the Framework bundle as well.

vitek-rostislav · 2024-05-06T08:25:53Z

packages/framework/src/Model/SocialNetwork/Exception/SocialNetworkExceptionInterface.php

+
+namespace Shopsys\FrameworkBundle\Model\SocialNetwork\Exception;
+
+interface SocialNetworkExceptionInterface


this interface in not necessary (we do not create the interfaces anymore, unless there is a valid use-case which I do not see here)

deleted in 68ae66f

vitek-rostislav · 2024-05-06T08:26:55Z

packages/framework/src/Model/SocialNetwork/SocialNetworkConfigFactory.php

+
+class SocialNetworkConfigFactory
+{
+    public const PROVIDER_GOOGLE = 'Google';


Suggested change

public const PROVIDER_GOOGLE = 'Google';

public const string PROVIDER_GOOGLE = 'Google';

fixed in 68ae66f

vitek-rostislav · 2024-05-06T08:28:54Z

packages/framework/src/Model/SocialNetwork/SocialNetworkConfigFactory.php

+class SocialNetworkConfigFactory
+{
+    public const PROVIDER_GOOGLE = 'Google';
+    public const PROVIDER_FACEBOOK = 'Facebook';


Suggested change

public const PROVIDER_FACEBOOK = 'Facebook';

public const string PROVIDER_FACEBOOK = 'Facebook';

fixed in 68ae66f

vitek-rostislav · 2024-05-06T08:29:05Z

packages/framework/src/Model/SocialNetwork/SocialNetworkConfigFactory.php

+{
+    public const PROVIDER_GOOGLE = 'Google';
+    public const PROVIDER_FACEBOOK = 'Facebook';
+    public const PROVIDER_SEZNAM = 'Seznam';


Suggested change

public const PROVIDER_SEZNAM = 'Seznam';

public const string PROVIDER_SEZNAM = 'Seznam';

fixed in 68ae66f

vitek-rostislav · 2024-05-06T08:43:05Z

packages/framework/src/Model/SocialNetwork/SocialNetworkConfigFactory.php

+     * @param string $seznamClientId
+     * @param string $seznamClientSecret
+     */
+    public function __construct(


TBH, I am not a fan of this solution...I think it is not ideal for future extensions and adding more providers.

I have an idea that I would like to discuss - what about creating a configuration yaml in project-base that would be loaded here and processed? It could look like this:

parameters: social_network_login_config: providers: google: enabled: true id: '%env(GOOGLE_CLIENT_ID)%' secret: '%env(GOOGLE_CLIENT_SECRET)%' facebook: enabled: true id: '%env(FACEBOOK_CLIENT_ID)%' secret: '%env(FACEBOOK_CLIENT_SECRET)%' ...

Then, SocialNetworkConfigFactory construct would accept just the config array:

public function __construct(array $socialNetworkLoginConfig)

services.yaml: Shopsys\FrameworkBundle\Model\SocialNetwork\SocialNetworkConfigFactory: arguments: $socialNetworkLoginConfig: '%social_network_login_config%'

The SocialNetworkConfigFactory::createConfig() method would dynamically create the config from the input array or the factory would not be needed anymore if the array in the yaml file is defined precisely 🤔 When typing this comment, I realized this is something I would expect from a Symfony bundle so I tried to search the bundle and I found this - https://github.com/ITLized/social - it seems like it is not maintained but the idea with the config in yaml was implemented there 🤔

If config in yaml, I would rather create a true framework bundle config than use parameters and have to validate them by hand – you can see inspiration in 9636a0f

@grossmannmartin Honestly, solution from Rosta seems to me better because creating "yaml configuration" (or how to name it) is not good with this solution because with configuration you tell what can be used and what is expected will come. This library for login by social network has many solutions and I can't all of them solve + if library whatever change you have to update your configuration ++ if on project you need to do something that is not expected you have to edit this configuration too. Because I don't think creating configuration is not good solution and I accept solution of creating only yaml file where I will send only config as array as Rosta suggest.

Do you agree or do you want to call so I can better describe issue with configuration solution? 🙏

changed by discussion and suggestion by Rosta, see 68ae66f. After discussion, I take it's OK now, if not let me know

vitek-rostislav · 2024-05-06T09:40:25Z

project-base/app/src/Model/Customer/User/RegistrationDataFactory.php

+     * @param \Hybridauth\User\Profile $profile
+     * @return \App\Model\Customer\User\RegistrationData
+     */
+    public function createFromSocialNetworkProfile(Profile $profile): RegistrationData


this should be implemented in the framework package 😇

vitek-rostislav · 2024-05-06T09:43:26Z

project-base/app/src/Model/Customer/User/RegistrationDataFactory.php

+        $registrationData->street = '';
+        $registrationData->city = '';
+        $registrationData->postcode = '';
+        $registrationData->country = $countries[0];


I know this is an edge case but what if there is no enabled country on a domain?

as we discussed, It's very little edge case and it will be solved in next follow-up task

vitek-rostislav · 2024-05-06T09:46:28Z

project-base/app/src/Model/Customer/User/RegistrationDataFactory.php

+        $registrationData->city = '';
+        $registrationData->postcode = '';
+        $registrationData->country = $countries[0];
+        $registrationData->password = $this->hashGenerator->generateHashWithoutConfusingCharacters(CustomerUserPasswordFacade::MINIMUM_PASSWORD_LENGTH);


When the password policy is changed on a project (e.g. special characters are required), this implementation might be a problem, right? Even at this moment, it feels like it might lead to quite weak passwords 🤔 Let's discuss it, please

fixed in 68ae66f

vitek-rostislav · 2024-05-06T09:50:56Z

project-base/app/src/Model/Security/LoginAsUserFacade.php

+     * @param \App\Model\Customer\User\CustomerUser $customerUser
+     * @return array{accessToken: string, refreshToken: string}
+     */
+    public function loginAndReturnAccessAndRefreshToken(CustomerUser $customerUser): array


it should not be in project-base 🙂 Moreover, I do not feel like putting this functionality into the existing LoginAsUserFacade - the class is used when you need to login administrator as a user. IMHO it is worth creating a separate class for your purpose. But we can discuss it if you want

fixed in 313cf3c as we agreed

vitek-rostislav · 2024-05-06T09:53:50Z

project-base/docker/nginx/nginx.conf

@@ -42,7 +42,7 @@ server {
        fastcgi_param SCRIPT_FILENAME $realpath_root/resolveFriendlyUrl.php;
    }

-    location ~ ^\/(admin|public|content(-test)?|ckeditor|build|bundles|graphql|_profiler|_wdt|file|redirect|elfinder|efconnect|personal-overview-export|codeception-acceptance-test|_error) {
+    location ~ ^\/(admin|public|content(-test)?|ckeditor|build|bundles|graphql|_profiler|_wdt|file|redirect|elfinder|efconnect|personal-overview-export|codeception-acceptance-test|_error|social-network) {


https://github.com/shopsys/deployment/blob/main/kubernetes/configmap/nginx.yaml needs to be edited in the same manner.

Then, you need to release a new version of the deployment package and require it in your composer.json
We can discuss this as well 😉

project-base/storefront/public/icons/google.png

project-base/storefront/pages/social-login.tsx

project-base/storefront/components/Blocks/Login/LoginForm.tsx

henzigo · 2024-05-16T09:56:12Z

project-base/app/src/Controller/Front/SocialNetworkController.php

+     */
+    public function loginAction(Request $request, string $type): Response
+    {
+        if (in_array($type, SocialNetworkConfigFactory::ALLOWED_PROVIDERS, true) === false) {


Why are you checking types agains some hard defined array instead of checking if type is in social_network_login_config configuration?

I would like to see that if you want to add for example Apple ID, you can do that just by configuration (and probably you will have to add some images to SF) but thats all. I don't like the idea when you have configuration for some service and then you have same information somewhere in codebase

hmm it true, it's not necessary check type, that library will throw exception if you send something not supported or disabled or..., deleted in 7f060b7

…mework bundle

…amework bundle

…etwork

- additionally refactored login form - this relates to both the login popup and login page

…review server

…ode can be nullable

sspooky13 force-pushed the pt-login-with-social-media branch 2 times, most recently from 8e15c2b to 433672b Compare May 4, 2024 14:08

henzigo reviewed May 6, 2024

View reviewed changes

vitek-rostislav requested changes May 6, 2024

View reviewed changes

sebaholesz force-pushed the pt-login-with-social-media branch from f5bccb8 to 19f354e Compare May 10, 2024 10:53

VaniaToper reviewed May 13, 2024

View reviewed changes

sebaholesz force-pushed the pt-login-with-social-media branch from 5ebff2b to 68a3d85 Compare May 14, 2024 18:54

henzigo reviewed May 16, 2024

View reviewed changes

sspooky13 force-pushed the pt-login-with-social-media branch 7 times, most recently from 3c8f547 to 7e6490e Compare May 28, 2024 08:17

sspooky13 added 2 commits May 29, 2024 18:59

Refactoring: move LoginAsUserFacade and his cascade dependency to fra…

f117e4a

…mework bundle

Refactoring: move RegistrationFacade and his cascade dependency to fr…

a10aa03

…amework bundle

sspooky13 force-pushed the pt-login-with-social-media branch from 7e6490e to f17b28a Compare May 29, 2024 17:16

sspooky13 and others added 9 commits May 29, 2024 20:51

create page where customers will be redirected to login into social n…

4503ece

…etwork

add upgrade notes

2f28a48

implemented social login on storefront

e0c6a82

- additionally refactored login form - this relates to both the login popup and login page

visually improved add-to-cart popup

3cadca3

fixup! implemented social login on storefront

755a699

fixup! implemented social login on storefront

b85126d

Refactoring: rename method for login administrator as customer user

7defbb7

login or register via Google, Facebook and Seznam are now enabled on …

a626ab0

…review server

GraphQL: Customer user: first name, last name, street, city and postc…

c37810c

…ode can be nullable

sspooky13 force-pushed the pt-login-with-social-media branch from 2bbd21c to c37810c Compare May 29, 2024 18:52

		@@ -61,4 +64,25 @@ public function createWithArgument(Argument $argument): RegistrationData

		return $registrationData;
		}


		namespace Shopsys\FrameworkBundle\Model\SocialNetwork\Exception;

		interface SocialNetworkExceptionInterface

	public const PROVIDER_GOOGLE = 'Google';
	public const string PROVIDER_GOOGLE = 'Google';

	public const PROVIDER_FACEBOOK = 'Facebook';
	public const string PROVIDER_FACEBOOK = 'Facebook';

	public const PROVIDER_SEZNAM = 'Seznam';
	public const string PROVIDER_SEZNAM = 'Seznam';

Login via social media #3154

Are you sure you want to change the base?

Login via social media #3154

Conversation

sspooky13 commented May 4, 2024 • edited by github-actions bot

henzigo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sspooky13 commented May 4, 2024 •

edited by github-actions bot