Skip to content

Dynamic cookie domain per request#46726

Merged
byroot merged 1 commit intorails:mainfrom
braindeaf:dynamic_cookie_domain_per_request
Dec 22, 2022
Merged

Dynamic cookie domain per request#46726
byroot merged 1 commit intorails:mainfrom
braindeaf:dynamic_cookie_domain_per_request

Conversation

@braindeaf
Copy link

@braindeaf braindeaf commented Dec 14, 2022

Motivation / Background

We've recently been looking at authentication across multiple applications where a multi-tenanted application needs to set a specific cookie domain e.g .tenant1.company.name for the application. Since this needs to work across multiple applications whether it be auth.tenant1.company.name, jam.tenant1.company.name we need just a little more flexibility and the :tld_length option won't cut it. Hence using a proc to set the cookie domain.

This pull request allows option[:domain] to be a proc

Detail

This is a small change to actionpack/lib/action_dispatch/middleware/cookies.rb, coping with that additional option.

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

@rails-bot rails-bot bot added the actionpack label Dec 14, 2022
@ghiculescu
Copy link
Member

Thanks for the PR. Could you please update the changelog, and squash your commits down to one?

@braindeaf
Copy link
Author

braindeaf commented Dec 14, 2022

@ghiculescu Added the CHANGELOG, just tried to squash into a single commit and now I've got 5 commits total :S. I don't think I've had to squash within the same branch before, only at the point of merging a PR from one branch to another.

@braindeaf braindeaf force-pushed the dynamic_cookie_domain_per_request branch 3 times, most recently from de7438d to 3ad0252 Compare December 14, 2022 20:32
@braindeaf
Copy link
Author

braindeaf commented Dec 14, 2022

Scratch that, I've sorted it :)

@ghiculescu ghiculescu added the ready PRs ready to merge label Dec 14, 2022
Copy link
Member

@byroot byroot left a comment

Choose a reason for hiding this comment

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

LGTM. Can you rebase to fix the conflict and let me know?

Per-request cookie domain set through proc
@braindeaf braindeaf force-pushed the dynamic_cookie_domain_per_request branch from 3ad0252 to 83da2e9 Compare December 22, 2022 19:49
@braindeaf
Copy link
Author

Sorry @byroot busy day. Rebased now :)

@byroot byroot merged commit 9589ef3 into rails:main Dec 22, 2022
@byroot
Copy link
Member

byroot commented Dec 22, 2022

No worries, there was no rush. Thanks for the PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

actionpack ready PRs ready to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants