medusa icon indicating copy to clipboard operation
medusa copied to clipboard

`/admin/auth` with api key returning 400 Bad Request

Open ScuroGuardiano opened this issue 3 years ago • 1 comments

Issue

Hi, when I try to get session with api key via /admin/auth I get 400 Bad Request.

Cause

After digging a little I found that error appears here get-session.ts#L25

  try {
    const userService: UserService = req.scope.resolve("userService")
    const user = await userService.retrieve(req.user.userId) // <---- here, userId is undefined

    const cleanRes = _.omit(user, ["password_hash"])
    res.status(200).json({ user: cleanRes })
  } catch (err) {
    res.sendStatus(400)
  }

The problem is that when we send request with access_token the property req.user.userId is not set. Instead user id is in req.user.id.

Solutions

I could PR solution but I don't know if my ideas isn't a little bit silly We could just take from both properties like this

  try {
    const userService: UserService = req.scope.resolve("userService")
    const user = await userService.retrieve(req.user.userId || req.user.id)

    const cleanRes = _.omit(user, ["password_hash"])
    res.status(200).json({ user: cleanRes })
  } catch (err) {
    res.sendStatus(400)
  }

Or change here passport.js#L52 like this:

  if (auth.success) {
    done(null, { ...auth.user, userId: auth.user.id })
  }

Btw

Couldn't we log this err from try...catch? Coz currently it's failing, telling user it's their mistake and not showing any details at all...

ScuroGuardiano avatar Apr 07 '22 18:04 ScuroGuardiano

const user = await userService.retrieve(req.user.userId || req.user.id)

I find this to be pretty clean. Would be great to see a PR for it, if you don't mind :)

olivermrbl avatar May 08 '22 14:05 olivermrbl

@ScuroGuardiano Curious, what's the use case? Why would you need to fetch an authenticated session using the API key?

olivermrbl avatar Sep 25 '22 17:09 olivermrbl

Hi there. We highly appreciate you filing an issue and showing an interest in improving Medusa.

I apologize for the delayed response.

Moving forward, we aim to do better. But we would like to start fresh. Therefore, we are considering all older issues as stale and closing them, even though they might still be relevant.

Please don’t hesitate to re-open the issue (or create a new one) if you still need a resolution or an answer.

Thanks ❤️

olivermrbl avatar Dec 06 '23 08:12 olivermrbl