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

feat(jwt): Pass Cookie Options from jwt Middleware to getCookie/getSignedCookie #2403

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

HeyITGuyFixIt
Copy link
Contributor

@HeyITGuyFixIt HeyITGuyFixIt commented Mar 22, 2024

Fixes #2398. Adds support for getSignedCookie while also allowing for all options to getCookie/getSignedCookie to be set. Sets the cookie option to have the type:

cookie: string | { key: string, secret?: string | BufferSource, prefixOptions?: CookiePrefixOptions }

The key names reflect the parameter names from getCookie and getSignedCookie. Tests still need to be added and ran for this.

Author should do the followings, if applicable

  • Add tests
  • Run tests
  • yarn denoify to generate files for Deno

Adds support for getSignedCookie while also allowing for all options to getCookie/getSignedCookie to be set.
@HeyITGuyFixIt
Copy link
Contributor Author

I could use some help writing the tests for this.

@yusukebe
Copy link
Member

Hi @HeyITGuyFixIt

I'll check the details later!

@yusukebe yusukebe added the v4.2 label Mar 29, 2024
@yusukebe
Copy link
Member

Hi @HeyITGuyFixIt

You can use this test:

describe('Credentials in signed cookie', () => {
  let handlerExecuted: boolean

  beforeEach(() => {
    handlerExecuted = false
  })

  const app = new Hono()

  app.use(
    '/auth/*',
    jwt({
      secret: 'a-secret',
      cookie: {
        key: 'cookie_name',
        secret: 'cookie_secret',
      },
    })
  )

  app.get('/auth/*', async (c) => {
    handlerExecuted = true
    const payload = c.get('jwtPayload')
    return c.json(payload)
  })

  it('Should not authorize', async () => {
    const req = new Request('http://localhost/auth/a')
    const res = await app.request(req)
    expect(res).not.toBeNull()
    expect(res.status).toBe(401)
    expect(await res.text()).toBe('Unauthorized')
    expect(handlerExecuted).toBeFalsy()
  })

  it('Should authorize', async () => {
    const url = 'http://localhost/auth/a'
    const req = new Request(url, {
      headers: new Headers({
        Cookie:
          'cookie_name=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJtZXNzYWdlIjoiaGVsbG8gd29ybGQifQ.B54pAqIiLbu170tGQ1rY06Twv__0qSHTA0ioQPIOvFE.i2NSvtJOXOPS9NDL1u8dqTYmMrzcD4mNSws6P6qmeV0%3D; Path=/',
      }),
    })
    const res = await app.request(req)
    expect(res).not.toBeNull()
    expect(res.status).toBe(200)
    expect(await res.json()).toEqual({ message: 'hello world' })
    expect(handlerExecuted).toBeTruthy()
  })
})

@yusukebe
Copy link
Member

Hi @Code-Hex !

What do you think about this PR? If you can, could you review it? I pasted my test code above, but I'm not sure it's enough.

Copy link
Contributor

@Code-Hex Code-Hex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@yusukebe
Copy link
Member

Hi @HeyITGuyFixIt

Thank you for changing. And last things. Can you do the two following things?

  • bun run lint:fix && bun run format:fix
  • bun run denoify

@yusukebe yusukebe changed the title Pass Cookie Options from jwt Middleware to getCookie/getSignedCookie feat(jwt): Pass Cookie Options from jwt Middleware to getCookie/getSignedCookie Mar 30, 2024
@yusukebe yusukebe removed the v4.2 label Apr 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use Signed Cookies in jwt Middleware
3 participants