Use headers instead of cookies for authorization#771
Conversation
Test coverage90.18% line coverage reported by SimpleCov. |
abcampo-iry
force-pushed
the
issues/1256-use-headers-instead-of-cookies
branch
from
919f9c5 to
c7199ae
Compare
abcampo-iry
force-pushed
the
issues/1256-use-headers-instead-of-cookies
branch
from
c7199ae to
019d66d
Compare
There was a problem hiding this comment.
Pull request overview
This PR migrates from cookie-based authentication to header-based authentication for Scratch API endpoints. It removes the IdentifiableByCookie concern that previously read auth tokens from cookies and instead relies on the existing Identifiable concern which reads from the Authorization header. All related test files are updated to use Authorization headers instead of setting cookies.
Changes:
- Removed
IdentifiableByCookieconcern (app/controllers/concerns/identifiable_by_cookie.rb) - Removed cookie-related code from ProjectsController and ScratchController
- Updated all Scratch-related test files to use Authorization headers
- Removed tests that verified cookie-setting behavior
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| app/controllers/concerns/identifiable_by_cookie.rb | Deleted - cookie-based auth no longer needed |
| app/controllers/api/scratch/scratch_controller.rb | Removed include IdentifiableByCookie |
| app/controllers/api/projects_controller.rb | Removed cookies include and cookie-setting logic |
| spec/requests/projects/show_spec.rb | Updated tests to verify no cookie is set, simplified test structure |
| spec/features/scratch/*.rb | Updated all test files to use Authorization headers in place of cookie headers |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
spec/requests/projects/show_spec.rb
Outdated
| it 'does not set a scratch auth cookie' do | ||
| get("/api/projects/#{project.identifier}", headers:) | ||
|
|
||
| expect(response).to have_http_status(:ok) | ||
| expect(response.cookies['scratch_auth']).to be_nil | ||
| end |
There was a problem hiding this comment.
I think this test doesn't have any purpose now - I think it's very unlikely that we would unintentionally set this cookie on this action in the future
There was a problem hiding this comment.
I agree.
Moreover, my thoughts was to have a negative test and then remove the test completely after the PR is completed.
Happy to remove it in the same PR.
At least with this comment we leave record that it passed and we can remove it.
zetter-rpf
left a comment
zetter-rpf
left a comment
There was a problem hiding this comment.
Great!
One very optional suggestion.
abcampo-iry
force-pushed
the
issues/1256-use-headers-instead-of-cookies
branch
from
019d66d to
b2944b2
Compare
Status
What's changed?
Steps to perform after deploying to production
If the production environment requires any extra work after this PR has been deployed detail it here. This could be running a Rake task, a migration, or upgrading a Gem. That kind of thing.