1. Addd scope, expires, access token, and online access info to session storage #11

MuhammadFarag · 2021-05-05T15:08:12Z

Add missing fields to session storage. These fields are required for the callback flow and online session tokens

MuhammadFarag · 2021-05-05T15:11:36Z

app/Models/DbSessionStorage.php

+        $dbSession->expires_at = $session->getExpires();
+        $dbSession->scope = $session->getScope();
+        if(!empty($session->getOnlineAccessInfo())){
+            $dbSession->online_access_info = serialize($session->getOnlineAccessInfo());


I have a bitter feeling about this...
Other options:

Serialize as JSON and handle potential exception

Create a column for each field in the online access info

Go the other way around and just serialize the entire Session object against the key

The disadvantage of serializing it as JSON is that we'd have to rebuild the object when coming back (which could be a static method in the info class). I agree that the serialize call is less than ideal, but this is a shallow value object, so it's not necessarily a big problem.

If we want to do things 'the right way', we'd probably go with option 2, which is more work but gives us fully structured data that can be queried for the purposes of an app - like say an app that needs to know which user is doing things in a context that does not use one of the lib's sessions.

Spread it into separate columns 👍

Paulinakhew

Maybe we should add a linter 🤔😂 other than that this PR makes sense! I don't really have an opinion on the serialize thing other than the fact that we should do either option 1 or 3. I'm leaning more towards option 1, for no particular reason

Paulinakhew · 2021-05-06T01:29:43Z

app/Models/DbSessionStorage.php

+            if($dbSession->expires_at){
+                $session->setExpires($dbSession->expires_at);
+            }
+            if($dbSession->access_token){
+                $session->setAccessToken($dbSession->access_token);
+            }
+            if($dbSession->scope){
+                $session->setScope($dbSession->scope);
+            }
+            if($dbSession->online_access_info){
+                $session->setOnlineAccessInfo(unserialize($dbSession->online_access_info));
+            }
+            return $session;


Suggested change

if($dbSession->expires_at){

$session->setExpires($dbSession->expires_at);

}

if($dbSession->access_token){

$session->setAccessToken($dbSession->access_token);

}

if($dbSession->scope){

$session->setScope($dbSession->scope);

}

if($dbSession->online_access_info){

$session->setOnlineAccessInfo(unserialize($dbSession->online_access_info));

}

return $session;

if ($dbSession->expires_at){

$session->setExpires($dbSession->expires_at);

}

if ($dbSession->access_token){

$session->setAccessToken($dbSession->access_token);

}

if ($dbSession->scope){

$session->setScope($dbSession->scope);

}

if ($dbSession->online_access_info){

$session->setOnlineAccessInfo(unserialize($dbSession->online_access_info));

}

return $session;

This tells me, we need to merge a linting PR prior to these PRs

Paulinakhew · 2021-05-06T01:32:11Z

app/Models/DbSessionStorage.php

        }
        return null;
    }

    public function storeSession(Session $session): bool
    {
-        $dbSession = new \App\Models\Session();
+        $dbSession =\App\Models\Session::where('session_id', $session->getId())->first();


Suggested change

$dbSession =\App\Models\Session::where('session_id', $session->getId())->first();

$dbSession = \App\Models\Session::where('session_id', $session->getId())->first();

Paulinakhew · 2021-05-06T01:32:21Z

app/Models/DbSessionStorage.php

        }
        return null;
    }

    public function storeSession(Session $session): bool
    {
-        $dbSession = new \App\Models\Session();
+        $dbSession =\App\Models\Session::where('session_id', $session->getId())->first();
+        if(!$dbSession){


Suggested change

if(!$dbSession){

if (!$dbSession){

Paulinakhew · 2021-05-06T01:32:49Z

app/Models/DbSessionStorage.php

+        $dbSession->access_token = $session->getAccessToken();
+        $dbSession->expires_at = $session->getExpires();
+        $dbSession->scope = $session->getScope();
+        if(!empty($session->getOnlineAccessInfo())){


Suggested change

if(!empty($session->getOnlineAccessInfo())){

if (!empty($session->getOnlineAccessInfo())){

Thank you @Paulinakhew 💯

paulomarg · 2021-05-11T14:17:29Z

app/Models/DbSessionStorage.php

+        $dbSession->expires_at = $session->getExpires();
+        $dbSession->scope = $session->getScope();
+        if(!empty($session->getOnlineAccessInfo())){
+            $dbSession->online_access_info = serialize($session->getOnlineAccessInfo());


The disadvantage of serializing it as JSON is that we'd have to rebuild the object when coming back (which could be a static method in the info class). I agree that the serialize call is less than ideal, but this is a shallow value object, so it's not necessarily a big problem.

If we want to do things 'the right way', we'd probably go with option 2, which is more work but gives us fully structured data that can be queried for the purposes of an app - like say an app that needs to know which user is doing things in a context that does not use one of the lib's sessions.

paulomarg · 2021-05-11T14:19:12Z

tests/Feature/DBSessionStorageTest.php

@@ -42,12 +60,11 @@ public function testLoadSessionReturnSessionIfItExists()
        );
    }

-    public function testStoreSessionReturnFalseIfOperationFails()
+    public function testStoreShouldUpdateSession()


I made this comment elsewhere, seems like that was a branch off of this :) - we should probably keep the old test around as it's useful, and trigger a different storage failure (like an invalid type for a column?).

I just didn't know how to trigger that failure in the tests 😅

😅 won't it fail if we give a column some bogus value? I guess the problem here is that strict typing makes our code too robust to fail 😄 .

Exactly 🕺

paulomarg

🚢

paulomarg · 2021-05-11T18:02:45Z

app/Lib/DbSessionStorage.php

+        $dbSession->expires_at = $session->getExpires();
+        $dbSession->scope = $session->getScope();
+        if (!empty($session->getOnlineAccessInfo())) {
+            $dbSession->user_id = $session->getOnlineAccessInfo()->getId();


A bit more verbose, but I like this approach better. As a separate PR, we may want to consider adding a method to Shopify\Auth\Session that takes in an object / associative array and pops out a session object so partners don't need to go through all of this (i.e. this could all be done as Session::createFromObject($dbSession).

I am not sure if we can generalize that. Because we don't know the underlying store they are using an how is it being encoded.

…torage.

MuhammadFarag requested a review from a team as a code owner May 5, 2021 15:08

MuhammadFarag requested a review from Paulinakhew May 5, 2021 15:08

MuhammadFarag force-pushed the update-sesssion-storage branch from c9d297e to b384cab Compare May 5, 2021 15:09

MuhammadFarag commented May 5, 2021

View reviewed changes

MuhammadFarag changed the title ~~Addd scope, expires, access token, and online access info to session storage~~ 1. Addd scope, expires, access token, and online access info to session storage May 6, 2021

Paulinakhew reviewed May 7, 2021

View reviewed changes

MuhammadFarag requested a review from paulomarg May 10, 2021 04:34

paulomarg reviewed May 11, 2021

View reviewed changes

MuhammadFarag requested review from paulomarg and Paulinakhew May 11, 2021 16:30

paulomarg approved these changes May 11, 2021

View reviewed changes

Add scope, expires, access token, and online access info to session s…

eda87f2

…torage.

MuhammadFarag force-pushed the update-sesssion-storage branch from 283a91d to eda87f2 Compare May 11, 2021 18:11

MuhammadFarag merged commit ddd5037 into main May 11, 2021

MuhammadFarag deleted the update-sesssion-storage branch May 11, 2021 18:11

cjohansson mentioned this pull request Aug 22, 2022

Shopify\Utils::sanitizeShopDomain(): Argument #1 ($shop) must be of type string, null given #316

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

1. Addd scope, expires, access token, and online access info to session storage #11

1. Addd scope, expires, access token, and online access info to session storage #11

MuhammadFarag commented May 5, 2021

MuhammadFarag May 5, 2021

paulomarg May 11, 2021

MuhammadFarag May 11, 2021

Paulinakhew left a comment

Paulinakhew May 6, 2021

MuhammadFarag May 10, 2021

Paulinakhew May 6, 2021

Paulinakhew May 6, 2021

Paulinakhew May 6, 2021

MuhammadFarag May 11, 2021

paulomarg May 11, 2021

paulomarg May 11, 2021

MuhammadFarag May 11, 2021

paulomarg May 11, 2021

MuhammadFarag May 11, 2021

paulomarg left a comment

paulomarg May 11, 2021

MuhammadFarag May 11, 2021

	$dbSession =\App\Models\Session::where('session_id', $session->getId())->first();
	$dbSession = \App\Models\Session::where('session_id', $session->getId())->first();

	if(!empty($session->getOnlineAccessInfo())){
	if (!empty($session->getOnlineAccessInfo())){

1. Addd scope, expires, access token, and online access info to session storage #11

1. Addd scope, expires, access token, and online access info to session storage #11

Conversation

MuhammadFarag commented May 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Paulinakhew 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

paulomarg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment