shopify-api-js icon indicating copy to clipboard operation
shopify-api-js copied to clipboard

Shopify.Utils.loadCurrentSession does not always call the loadCallback and returns undefined, Oauth loop

Open jt274 opened this issue 3 years ago • 29 comments

Issue summary

On the server side, when calling await Shopify.Utils.loadCurrentSession(ctx.req, ctx.res), the function is supposed to call the loadCallback for the Shopify.Session.CustomSessionStorage. However, it does not always call this function when invoked, and immediately returns undefined, which causes the app to re-auth and get stuck in an infinite Oauth loop.

Expected behavior

await Shopify.Utils.loadCurrentSession(ctx.req, ctx.res) should always call the loadCallback for the Shopify.Session.CustomSessionStorage. Its value should be undefined only if loadCallback returns undefined, but should otherwise contain a Session object.

Actual behavior

During auth and app loading, Shopify.Utils.loadCurrentSession calls the loadCallback function. However, after auth when handleRequest is called, and the Shopify.Utils.loadCurrentSession is called again, it immediately (less than 1ms) returns undefined and loadCallback is NOT run.

Example code from server.js

Shopify.Context.initialize({
  API_KEY: API_KEY,
  API_SECRET_KEY: SHOPIFY_API_SECRET,
  SCOPES: SHOPIFY_API_SCOPES.split(","),
  HOST_NAME: SHOPIFY_APP_URL.replace(/https:\/\//, ""),
  API_VERSION: ApiVersion.April21,
  IS_EMBEDDED_APP: true,
  SESSION_STORAGE: new Shopify.Session.CustomSessionStorage(
    storeCallback,
    loadCallback,
    deleteCallback,
  ),
});

async function loadCallback(id) {
  //This is NOT called after handleRequest()
  //Load session from db
  session = await getFromDb();
  if(session) {
    return session;
  } else {
    return undefined;
  }
});

  router.get("/", async (ctx) => {
    const shop = ctx.query.shop;

    const currentSession = await Shopify.Utils.loadCurrentSession(ctx.req, ctx.res);
    if(!currentSession) {
      ctx.redirect(`/auth?shop=${shop}`);
    }  else {
      await handleRequest(ctx);
    }
  });

App Oauth flow

Shopify.Utils.loadCurrentSession: undefined
Redirect to /auth
storeCallback: Store new temp session #1 (random id, with partial data)
Shopify.Utils.loadCurrentSession (loadCallback runs): Load temp session #1
storeCallback: Store shop session (with myshopify url in id)
storeCallback: Store temp session #1 again (with full data)
Shopify.Utils.loadCurrentSession (loadCallback runs): Load temp session #1 again
Redirect to /
Shopify.Utils.loadCurrentSession (loadCallback runs): Load temp session #1 again
Load app (handleRequest())
Shopify.Utils.loadCurrentSession (**loadCallback does NOT run**): Attempt to load current online session (but immediately returns undefined)
Redirect to /auth
Process repeats until app throws error: "The app couldn’t be loaded This app can’t load due to an issue with browser cookies."

jt274 avatar Apr 14 '21 15:04 jt274

There are two things I noticed:

1. Is the storeCallback storing the session correctly?

your database record to be like this

id session
4ac3a57c-7dc3-4350-b8e7-f328a560f68f {"id":"4ac3a57c-7dc3-4350-b8e7-f328a560f68f","shop":"yourshop.myshopify.com","state":"842171815096913",...}

2. Is the loadCallback loading the session correctly?

You haven't passed the argument id to getFromDb, so it's unclear which session you can load.

async function loadCallback(id) {
    session = await getFromDb();
...

aside from that.

I have also suffered from similar behavior. In my case, when I tried to run it with lambda@AWS-APIGateway,the first loadSession call succeeded or failed, about half and half. I don't know the reason, but it can be explained as the behavior that loadSession is called before Shopify.Context.initialize is completed. The order of processing was fixed by doing the following:

deleted. (it was wrong way.) ~~app.use( Shopify.Context.initialize({ API_KEY: 'Your API_KEY', API_SECRET_KEY: 'Your API_SECRET_KEY', SCOPES: ['Your scopes'], HOST_NAME: 'Your HOST_NAME (omit the https:// part)', API_VERSION: ApiVersion.October20, IS_EMBEDDED_APP: true, SESSION_STORAGE: new Shopify.Session.MemorySessionStorage(), }); shopifyAuth({ ... );~~

kato-takaomi-ams avatar Apr 15 '21 00:04 kato-takaomi-ams

Hmm, interesting thought, I will look into that.

Yes, I have confirmed storeCallback and loadCallback are functioning properly. (My auth was working previously, but I had some package update break it. After reverting and not seeming like anything changed, I now have this Oauth loop.)

jt274 avatar Apr 15 '21 02:04 jt274

@kato-takaomi-ams doing that means you have the initialization twice? One outside app.use and another inside app.use?

@jt274 have you tried the proposed solution, if so, did it fix the problem?

ilugobayo avatar Apr 15 '21 19:04 ilugobayo

@ilugobayo I have only one initialization. The initialization moved from outside app.use to inside app.use.

kato-takaomi-ams avatar Apr 15 '21 23:04 kato-takaomi-ams

@kato-takaomi-ams My bad, I was referring to app.prepare().then(), but anyway, with this you fixed the OAuth loop then? Do you use the both access modes, or only the online access mode? Haven't you noticed any issues with that change?

ilugobayo avatar Apr 15 '21 23:04 ilugobayo

@kato-takaomi-ams I've tried putting the context initialization inside the server.use(). I recently converted the server to Typescript, and it gives the error Argument of type 'void' is not assignable to parameter of type 'Middleware<DefaultState, DefaultContext, any>'. So, I'm not sure it's supposed to go there. Placing it inside the app.prepare() had no effect.

jt274 avatar Apr 16 '21 05:04 jt274

@jt274 Sorry. I was wrong. I ended up separating the initialization into a separate ts file.

app.ts

import Shopify, { AuthQuery } from "./shopify";

shopify.ts

require("dotenv").config();
import Shopify, { ApiVersion } from "@shopify/shopify-api";

const { API_KEY, API_SECRET_KEY, SCOPES, HOST } = process.env;
const { sessionStorage } = require("session_storage");
const config = {
  API_KEY,
  API_SECRET_KEY,
  SCOPES: [SCOPES],
  HOST_NAME: HOST,
  IS_EMBEDDED_APP: true,
  API_VERSION: ApiVersion.January21,
  SESSION_STORAGE: sessionStorage,
};

Shopify.Context.initialize(config);
console.log("Shopify.Context.initialize", config);

export default Shopify;
export * from "@shopify/shopify-api";

kato-takaomi-ams avatar Apr 16 '21 05:04 kato-takaomi-ams

@kato-takaomi-ams & @jt274 why did you decide to change it to Typescript?

By the way, I tried your solution but it didn't work, got this error:

(node:171296) UnhandledPromiseRejectionWarning: TypeError: middleware must be a function!
    at Application.use (/home/ilugo/Documents/bayonet-shopify/node_modules/koa/lib/application.js:123:41)
    at /home/ilugo/Documents/bayonet-shopify/server/server.js:79:10
(Use `node --trace-warnings ...` to show where the warning was created)
(node:171296) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 1)
(node:171296) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

So I still can't get the session token consistently with loadCurrentSession for the required validations in "/".

ilugobayo avatar Apr 16 '21 11:04 ilugobayo

same problem! after registration app i have valid session object, with which I can make requests, for example Assets API, but when I uninstall the app, I try to retrieve session in '/webhook' by Shopify.Utils.loadCurrentSession, but i got undefined

valorloff avatar Apr 16 '21 13:04 valorloff

Since my error seems to be occurring inside router.get("/"), I checked to see if the Shopify.Context had been initialized in that route before calling handleRequest() (which causes the session to be lost and re-auth). I checked by printing to console the Shopify.Context.API_VERSION. It indeed IS initialized at that point, so I'm not sure the issue is with the Context initialization.

jt274 avatar Apr 16 '21 15:04 jt274

Also, to clarify:

  • Shop goes through auth successfully, redirects to /?shop=shop_name
  • Current session loads successfully by the Shopify.Utils.loadCurrentSession calling the loadCallback function, and await handleRequest(ctx) runs
  • Server shows it serving a bunch of /_next/static/ files with a 204 response
  • After all files served, about two seconds later, a 302 redirect is sent to /?hmac=LONGIDHERE&locale=en&new_design_language=true&session=LONGIDHERE&shop=shop_name&timestamp=1234567890
  • router.get("/") runs again, this time the current online session does NOT load, Shopify.Utils.loadCurrentSession does NOT call loadCallback function for session, and session returned is undefined and app goes to re-auth loop

jt274 avatar Apr 16 '21 16:04 jt274

@jt274 yes, this is exactly the behavior I have, at some point Shopify.Utils.loadCurrentSession DOES retrieve a session but when the main page is about to load, it redirects me to '/auth' again. I "got rid" of the OAuth loop with this:

    if (installedStatus === 0 || installedStatus === 2) {
      ctx.redirect(`/install/auth?shop=${shop}`);
    } else {
      if (sessionRetrieved !== undefined) {
        if (new Date(sessionRetrieved.expires) < Date.now()) {
          ctx.redirect(`/auth?shop=${shop}`);
        }
      } else {
        console.log(`Session couldn't be retrieved`);
        await handleRequest(ctx);
      }
    }

Now I have an error that says "Not found"; I debugged the code and checked the session that sometimes retrieves, that session is the temporary one with random ID, the one that has shop_user as ID is never retrieved, oddly enough, the expires value of that temporary session is always less than the present time, therefore, it would always try to redirect to '/auth'.

I still don't know how to retrieve the session I need (shop_user as ID), is the last piece I'm missing to complete the session token implementation.

I was thinking on maybe create a function that directly checks the database, without using Shopify.Utils.loadCurrentSession or the callback functions; since ctx.query contains a session value, I think it would be possible to "manually" check if the session value of the session stored in the database (or whatever session storage you're using) is different, if so, redirect to '/auth'?

That is just a random idea, the only issue I found is that I can't actually know which session to check, I know it's a session for a given shop (ctx.query.shop) but ctx doesn't include the user as far as I know, if there was a way to get the user as well, then maybe my idea could work.

ilugobayo avatar Apr 17 '21 14:04 ilugobayo

I was thinking on maybe create a function that directly checks the database

I've tried it already (with accessToken). If session is undefined, its values from the database are no longer valid

valorloff avatar Apr 17 '21 14:04 valorloff

@ilugobayo I don't believe you can just check the session yourself. It actually has to be loaded by the library. If it's not loaded (and able to be retrieved with loadCurrentSession), the info will not be sent with graphql requests and they will fail.

My app definitely retrieves the session with the shop name in the id and a proper expiration date. But it still disappears at the app loading and enters the re-auth loop. I'm also not getting the headers returned from the graphql requests to tell me when the session is invalid, despite specifying them to be returned.

jt274 avatar Apr 17 '21 14:04 jt274

There is also this issue which seems related, but I'm not clear on the conclusion that was made: https://github.com/Shopify/shopify-node-api/issues/150

jt274 avatar Apr 17 '21 15:04 jt274

@jt274 it's really odd, because the loadCallback execution actually loads a session, but why loadCurrentSession would never return anything since it should execute those functions, only sessions with random ID, do you think I'm doing something wrong with my session storage?

class SessionHandler {
  
  async storeCallback(session) {
    //console.log('store callback');
    try {
      return await dbSetHelper.saveSessionForShop(session.id, JSON.stringify(session));
    } catch (err) {
      // throw errors, and handle them gracefully in your application
      throw new Error(err)
    }
  }
  
  async loadCallback(id) {
    console.log('load callback');
    try {
      var reply = await dbGetHelper.getSessionForShop(id);
      
      const parsedReply = JSON.parse(reply);

      const newSession = new Session(parsedReply['id']);

      newSession.shop = parsedReply['shop'];
      newSession.state = parsedReply['state'];
      newSession.isOnline = parsedReply['isOnline'];
      newSession.accessToken = parsedReply['accessToken'];

      newSession.expires = parsedReply['expires'] ? new Date(parsedReply['expires']) : null;
      newSession.scope = parsedReply['scope'];
      newSession.onlineAccessInfo = parsedReply['onlineAccessInfo'] ? parsedReply['onlineAccessInfo'] : null;

        //const parsedJson = JSON.parse(reply);
        //var newSession = new Session(parsedJson['id']);
        //Object.entries(parsedJson).forEach(([key, value]) => newSession[key] = value);

      console.log(newSession);
      
      return newSession;
      // } else {
      //   return undefined;
      // }
    } catch (err) {
      // throw errors, and handle them gracefully in your application
      return undefined;
    }
  }
  
  async deleteCallback(id) {
    //console.log('delete callback');
    try {
      return dbSetHelper.deleteSessionForShop(id);
    } catch (err) {
      // throw errors, and handle them gracefully in your application
      throw new Error(err)
    }
  }
}

// Export the class
module.exports = SessionHandler

This is pretty much it, do you see anything wrong?

ilugobayo avatar Apr 17 '21 15:04 ilugobayo

@ilugobayo The problem I'm encountering is that the loadCallback does not even run every time it should. I place a log in the function to see when it runs:

async function loadCallback(id) {
  console.log('SESSION LOAD REQUEST: ', id);
  //rest of function
}

The function seems to work perfectly when called. But when the server calls handleRequest after successfully validating the session, router.get("/") gets called again (where all the verification and session loading takes place), but this time Shopify.Utils.loadCurrentSession does not fire loadCallback (as verified by the console log), so the session becomes undefined and then the app goes to re-auth over and over again.

It almost feels to me that the loadCurrentSession isn't completing, like the async part of the function isn't working or something and it gets executed and immediately proceeds after the handleRequest.

jt274 avatar Apr 17 '21 15:04 jt274

@jt274 I think that's actually the case, maybe it's just too fast; it would be really helpful to find someone that already solved this or someone that could guide us in case we're doing something wrong. I honestly don't know what else to try, feel like I'm in a dead end.

ilugobayo avatar Apr 17 '21 15:04 ilugobayo

I am tracking down the path for loading the session, and I've discovered that loadCurrentSession calls the getCurrentSessionId function. This function takes in the request and response arguments. It then reads the authorization header:

https://github.com/Shopify/shopify-node-api/blob/0c16494755f4507bca01dcef3e9243bcc0478ff8/src/auth/oauth/oauth.ts#L240

The problem is that for me this header is not present when my server.js file loads the / route like so:

router.get("/", async (ctx) => { //function here });

ctx.req does not contain auth headers. If you go through the rest of the getCurrentSessionId function, you'll see that it then causes it to be treated like an offline session and tries to read the session from a cookie, which is also undefined. It then returns the undefined session. And with no session, we cannot do anything other than go back to auth again.

jt274 avatar Apr 17 '21 16:04 jt274

@jt274 In this case, the issue is on our side? Or can be considered a bug?

ilugobayo avatar Apr 18 '21 12:04 ilugobayo

I am not sure, I am currently trying to find what is supposed to supply those headers and why they are not there.

jt274 avatar Apr 18 '21 18:04 jt274

Well, my app works now. Literally changed nothing since last night when it was completely broken! Using the exact same deployment as before. Not sure what to say. I'll keep digging and see what I can do to have the auth break again.

jt274 avatar Apr 20 '21 04:04 jt274

I am having a similar issue.

leesander1 avatar Jun 03 '21 03:06 leesander1

I am also facing the same issue. Any findings?

@jt274 The Authorization header is added only when authenticatedFetch from @shopify/app-bridge-utilities is used to make a fetch request from the frontend.

mkamalkayani avatar Jun 11 '21 22:06 mkamalkayani

Hi guys, the issue occurs only when a user is on the "You are about to install #APP_NAME" page and they wait for more than 1 minute to click "Install app".

Perhaps this bug is difficult to recreate as installing the app on a test store, you typically don't go through the App store. This means the "You are about to install #APP_NAME" is skipped as it "Install app" is auto-clicked/auto-selected.

As noted above, the loadCallback function isn't even called and results in the following error Cannot complete OAuth process. Could not find an OAuth cookie for shop url: #SHOP_URL even though there is a session in the storage.

This issue occurs when using the Shopify.Session.MemorySessionStorage and Shopify.Session.CustomSessionStorage.

My relevant dependencies are:

"@shopify/app-bridge": "^2.0.3",
"@shopify/app-bridge-react": "^2.0.3",
"@shopify/app-bridge-utils": "^2.0.3",
"@shopify/koa-shopify-auth": "^4.1.3",
"koa": "^2.13.1",
"koa-bodyparser": "^4.3.0",
"koa-ignore": "^1.0.1",
"koa-logger": "^3.2.1",
"koa-router": "^8.0.8",
"shopify-api-node": "^3.6.12",

The relevant session code:

class Store {
  session: any;

  constructor() {}

  storeCallback = async (session: Session) => {
    this.session = session;
    return true;
  };

  loadCallback = async (id: string) => {
    // Never gets here if there is more than 1 minute
    // between /auth and /auth/callback
    return this.session;
  };

  deleteCallback = async (id: string) => {
    return true;
  };
}

const sessionStorage = new Store();

Shopify.Context.initialize({
  API_KEY: process.env.SHOPIFY_API_KEY || "",
  API_SECRET_KEY: process.env.SHOPIFY_API_SECRET || "",
  SCOPES: process.env.SHOPIFY_API_SCOPES?.split(",") || [""],
  HOST_NAME: APP_URL.replace(/https:\/\//, ""),
  API_VERSION: ApiVersion.October20,
  IS_EMBEDDED_APP: true,
  SESSION_STORAGE: new Shopify.Session.CustomSessionStorage(
    sessionStorage.storeCallback,
    sessionStorage.loadCallback,
    sessionStorage.deleteCallback
  ),
});

louiscollarsmith avatar Jun 15 '21 21:06 louiscollarsmith

@jt274 In this case, the issue is on our side? Or can be considered a bug?

I think this should be considered a bug. We are in the process of onboarding new clients and have told them to try and zoom through the install page, but this isn't ideal as most clients like to read about what they're installing!

louiscollarsmith avatar Jun 15 '21 21:06 louiscollarsmith

Hi guys, the issue occurs only when a user is on the "You are about to install #APP_NAME" page and they wait for more than 1 minute to click "Install app".

I might see this as a different issue if it's happening during install. For me, when it was happening, it was after install and happened regularly.

I have a couple areas of the app auth that feel a bit like they're being held together with tape. Definitely think the whole auth situation could be improved. It can make for some long loading times.

jt274 avatar Jun 16 '21 01:06 jt274

Opened new issue https://github.com/Shopify/shopify-node-api/issues/202

louiscollarsmith avatar Jun 17 '21 11:06 louiscollarsmith

Same issue here. Any updates on that?

zirkelc avatar Aug 04 '21 11:08 zirkelc

This issue is stale because it has been open for 90 days with no activity. It will be closed if no further action occurs in 14 days.

github-actions[bot] avatar Oct 06 '22 02:10 github-actions[bot]