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: Set httpOnly flag in cookie #2591

Merged
merged 5 commits into from
Dec 21, 2023
Merged

feat: Set httpOnly flag in cookie #2591

merged 5 commits into from
Dec 21, 2023

Conversation

DafyddLlyr
Copy link
Contributor

@DafyddLlyr DafyddLlyr commented Dec 20, 2023

What does this PR do?

  • Adds the httpOnly flag to our auth cookie
  • Corresponding changes to not descructure and read the cookie in the frontend
  • Some refactoring of auth methods to make things a bit easier to follow

How does auth work?

I'll move this diagram to a page in Notion as well, but this is a helpful diagram for making sense of the changes here -

sequenceDiagram
  participant Frontend as PlanX Editor
  participant API as PlanX API
  participant GoogleOAuth as Google OAuth
  participant Hasura

  Frontend ->> API: Initiate Login
  activate API
  API ->> GoogleOAuth: Proxy OAuth request

  activate GoogleOAuth
  GoogleOAuth ->> API: Redirect to callback URL
  deactivate GoogleOAuth

  API ->> API: Generate JWT

  alt Staging/Production
    API ->> Frontend: Return JWT in cookie
  else Local Dev/Pizzas
    API ->> Frontend: Return JWT in search params
    deactivate API
    Frontend ->> Frontend: Generate cookie
  end
    
    Frontend ->> API: Make request to /user/me (with cookie)
    activate API
    API ->> Hasura: Get user details
    activate Hasura
    Hasura ->> API: Return user details
    deactivate Hasura
    API ->> Frontend: Return user details
    deactivate API
    Frontend ->> Frontend: Initialise userStore
Loading

How do I test this?

  • I can log into the Pizza
  • I can repeatedly log in / log out of the Pizza
  • I can make a request to https://api.2591.planx.pizza/user/me when logged in and I get a response (request is made via cookie)
  • I can see that the Access-Control-Allow-Origin header is set to https://2591.planx.pizza when hitting /user/me as part of the auth process

(Once changes merged to staging)

  • I can repeat the above steps on staging
  • I can see the secure flag appended to my cookie
  • I can still log in on Pizzas

Copy link

github-actions bot commented Dec 20, 2023

Removed vultr server and associated DNS entries

@DafyddLlyr DafyddLlyr force-pushed the dp/http-only-v3 branch 3 times, most recently from afa1890 to c14c9d3 Compare December 21, 2023 09:44
@DafyddLlyr DafyddLlyr force-pushed the dp/http-only-v3 branch 3 times, most recently from 01c2694 to e7ffb80 Compare December 21, 2023 11:31
@@ -25,7 +25,7 @@
"@tiptap/extension-history": "^2.0.3",
"@tiptap/extension-image": "^2.0.3",
"@tiptap/extension-italic": "^2.0.3",
"@tiptap/extension-link": "^2.0.3",
"@tiptap/extension-link": "^2.1.13",
Copy link
Contributor Author

@DafyddLlyr DafyddLlyr Dec 21, 2023

Choose a reason for hiding this comment

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

This was the issue that was giving me so much grief (causing E2E tests to fail) - I managed to recreate on the Pizza whilst testing 🤯

I still really don't get the relationship here or what the real root cause is, but it was code for this extension in RichTextInput.tsx which was causing issues.

I'll pick up a PR to revert #2589 shortly.

@DafyddLlyr DafyddLlyr requested a review from a team December 21, 2023 12:49
@DafyddLlyr DafyddLlyr marked this pull request as ready for review December 21, 2023 12:49
@DafyddLlyr DafyddLlyr changed the title feat(wip): Set httpOnly flag in cookie feat: Set httpOnly flag in cookie Dec 21, 2023
Copy link
Member

@jessicamcinchak jessicamcinchak left a comment

Choose a reason for hiding this comment

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

Thanks for getting to the bottom of this one, diagram is super helpful! Testing steps all working as expected for me, happy to test again this afternoon on staging.

Copy link
Contributor

@Mike-Heneghan Mike-Heneghan left a comment

Choose a reason for hiding this comment

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

Looks great! It sounds like it was a frustrating piece of work but awesome you got to the bottom of it 🥳

@DafyddLlyr DafyddLlyr merged commit 01bcb76 into main Dec 21, 2023
12 checks passed
@DafyddLlyr DafyddLlyr deleted the dp/http-only-v3 branch December 21, 2023 14:11
DafyddLlyr added a commit that referenced this pull request Dec 21, 2023
DafyddLlyr added a commit that referenced this pull request Dec 21, 2023
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.

3 participants