Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug?]: v7.0.0 RC: dbAuth immediately invalidates cookie if using an id field other than "id" #10005

Closed
1 task done
will-ks opened this issue Feb 13, 2024 · 2 comments · Fixed by #10013
Closed
1 task done
Assignees
Labels
bug/confirmed We have confirmed this is a bug topic/auth

Comments

@will-ks
Copy link
Contributor

will-ks commented Feb 13, 2024

What's not working?

I upgraded my RW project to the v7.0.0 RC and encountered the following issue with dbAuth. I am unable to sign up using the dbAuth signup function, which was working fine before the upgrade.

What happens when I use the signup function is:
First off, this request is made:

Request URL:
http://localhost:8910/.redwood/functions/auth
Request Method:
POST
Status Code:
201 Created

the response headers for this request returned successfully sets the cookie as expected:

HTTP/1.1 201 Created
Access-Control-Allow-Origin: *
content-type: application/json; charset=utf-8
csrf-token: 7d3e8226-56a1-44ab-a580-49509eaad1a7
set-cookie: redwood-saas-starter_session_8911=6U88Fq1jYTY9EoNywJu3j4+VUCnNc/Lq8ZDrlIFTzffRnF1Xa6x1URiNkpZNuW9lV5GLi0SBmDv8iWZro3CX116leje+yADMEXZnm3il4H1LygOcPTd7NKzs63M4zY2MF/y76Nu5kvY+ttbGC+2ww4l84qMe8SgVrZfVdhHXq1o=|uMryi9dy0Hz3msm/puJ4Lg==;HttpOnly;Path=/;SameSite=Strict;Expires=Fri, 10 Feb 2034 21:37:28 GMT
content-length: 83
date: Tue, 13 Feb 2024 21:37:28 GMT
connection: close

Then immediately following this, this request is made:

Request URL:
http://localhost:8910/.redwood/functions/auth?method=getToken
Request Method:
GET
Status Code:
200 OK

however, the response headers for this request now clear the cookie by setting an expire date in the past:

HTTP/1.1 200 OK
Access-Control-Allow-Origin: *
content-type: application/json; charset=utf-8
set-cookie: redwood-saas-starter_session_8911=;HttpOnly;Path=/;SameSite=Strict;Expires=Thu, 01 Jan 1970 00:00:00 GMT
content-length: 0
date: Tue, 13 Feb 2024 21:37:28 GMT
connection: close

I dug in to the dbAuth code and found that the issue is with these two lines:


where: { [this.options.authFields.id]: this.session?.id },

These fail in my case because this.session.id is undefined. I use a different id field on my user object called userId, not id (which I have set in the dbAuth options options.authFields.id). In my case this.session is:

{
	userId: '0d94a3ac-2b51-40f9-9c59-8be0da2e979c',
	email: 'foo@bar.com'
}

Changing the two lines to:

    if (!this.session?.[this.options.authFields.id]) {

and

        where: { [this.options.authFields.id]: this.session?.[this.options.authFields.id] },

fixes the issue. If this sounds like a sensible fix I am glad to submit a PR.

I'm not really sure why this issue didn't show itself before upgrading to v7 🤔

How do we reproduce the bug?

No response

What's your environment? (If it applies)

System:
    OS: macOS 13.4.1
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 20.11.0 - /private/var/folders/b6/pqr079j13t16f80pyr5qjf2r0000gn/T/xfs-89f6020b/node
    Yarn: 3.2.1 - /private/var/folders/b6/pqr079j13t16f80pyr5qjf2r0000gn/T/xfs-89f6020b/yarn
  Databases:
    SQLite: 3.39.5 - /usr/bin/sqlite3
  Browsers:
    Chrome: 119.0.6045.105
    Safari: 16.5.1
  npmPackages:
    @redwoodjs/auth-dbauth-setup: 7.0.0-rc.970 => 7.0.0-rc.970+2b404570e 
    @redwoodjs/cli-storybook: 7.0.0-rc.970 => 7.0.0-rc.970+2b404570e 
    @redwoodjs/core: 7.0.0-rc.970 => 7.0.0-rc.970+2b404570e

Are you interested in working on this?

  • I'm interested in working on this
@will-ks will-ks added the bug/needs-info More information is needed for reproduction label Feb 13, 2024
@cannikin cannikin self-assigned this Feb 14, 2024
@cannikin
Copy link
Member

Hello! Thanks so much for the issue! I think I realized why this wasn't an issue prior to this 7.0 release:

In 7.0 we introduce the allowedUserFields option which makes sure that only the fields you define are returned by any of the auth functions. If you don't include that option at all (any app created prior to 7.0 wouldn't have it) then it defaults to ["id", "email"], so by default a field named userId wouldn't be available to some functions any more. You can read about setting this in the PR introduction.

But, even if you did set allowedUserFields it would still fail because of the lines of code you identified above! Previous to 7.0, session.id was considered "internal" and so would always be called id. Prior to v7.0 we would set it like this:

session.id = user[this.options.authFields.id]

So no matter what you called the column in your own database, as far as dbAuth and the cookies are concerned, it's now called id. And then dbAuth reads the cookie to find out what user was logged in, it would do the reverse and map session.id to this.options.authFields.id

But now, we sanitize the user data (all keys not present in allowedUserFields are removed) and then anything left over is encrypted into the session cookie. So as long as userId is in the allow list, it would be available in the cookie and dbAuth could look up the user with it: except that those two lines are still hardcoded to session.id!

So yes, we'd love a PR that fixes this! When I wrote this code originally, all of my testing involved the default id column name, so I didn't run into this. If you could include a test that proves everything works with an id column of a different name, that would be amazing! :)

@cannikin
Copy link
Member

We were hoping to do an official release of 7.0 on Thursday or Friday of this week, so if that's too short of a timetable for a PR we totally understand—just say the word and we'll fix it real quick! We'll make sure to give you credit, you already did all the work. ;)

@jtoar jtoar added bug/confirmed We have confirmed this is a bug topic/auth and removed bug/needs-info More information is needed for reproduction labels Feb 14, 2024
cannikin added a commit that referenced this issue Feb 14, 2024
…ny user data defined by `allowedUserFields` not only `id`

Closes #10005
jtoar pushed a commit that referenced this issue Feb 14, 2024
Fixes bug when User table had a primary key other than `id`. Shout out
to @will-ks for finding this!

### Impact 

For apps which had a primary key other than `id`, all users will be
logged out on their next request after this is deployed.

Not sure if we consider that breaking? But it is 7.0 so anything goes!

Closes #10005
jtoar pushed a commit that referenced this issue Feb 15, 2024
Fixes bug when User table had a primary key other than `id`. Shout out
to @will-ks for finding this!

### Impact 

For apps which had a primary key other than `id`, all users will be
logged out on their next request after this is deployed.

Not sure if we consider that breaking? But it is 7.0 so anything goes!

Closes #10005
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug/confirmed We have confirmed this is a bug topic/auth
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants