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

Add back in Cookie Domain support #2432

Merged
merged 3 commits into from
Jun 21, 2018
Merged

Add back in Cookie Domain support #2432

merged 3 commits into from
Jun 21, 2018

Conversation

nwmac
Copy link
Contributor

@nwmac nwmac commented Jun 19, 2018

Env var COOKIE_DOMAIN can restrict the domain used for the session cookie.

This PR:

  • Adds back in the COOKIE_DOMAIN in the Helm chart
  • Adds a new helm value console.cookieDomain that overrides env.DOMAIN
  • Allows this new value to be set to "-" to say not to set it (event if env.DOMAIN is set)
  • Adds a view that is shown if the configured Cookie Domain does not match that of the requesting URL - since the session cookie won't work and therefore the user can not login

@cfdreddbot
Copy link

Hey nwmac!

Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you and the commit authors have already signed the CLA.

@codecov
Copy link

codecov bot commented Jun 19, 2018

Codecov Report

Merging #2432 into v2-master will increase coverage by 0.01%.
The diff coverage is 82.14%.

@@              Coverage Diff              @@
##           v2-master    #2432      +/-   ##
=============================================
+ Coverage      71.03%   71.04%   +0.01%     
=============================================
  Files            585      586       +1     
  Lines          24536    24567      +31     
  Branches        5509     5520      +11     
=============================================
+ Hits           17429    17454      +25     
- Misses          7107     7113       +6

@nwmac nwmac self-assigned this Jun 19, 2018
Copy link
Contributor

@irfanhabib irfanhabib left a comment

Choose a reason for hiding this comment

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

LGTM, minor tweaks

@@ -182,8 +182,12 @@ spec:
- name: SKIP_SSL_VALIDATION
value: {{default "true" .Values.uaa.skipSSLValidation | quote}}
{{- end }}
{{- if .Values.env.DOMAIN }}
- name: X_COOKIE_DOMAIN
{{- if .Values.console.cookieDomain }}{{ if ne .Values.console.cookieDomain "-" }}
Copy link
Contributor

Choose a reason for hiding this comment

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

both ifs can be merged

{{- if and .Vales.console.cookieDomain (ne .Values.console.cookieDomain "-") }}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@irfanhabib - The logic I want is: if cookieDomain is set, use it, unless it is set to '-' in which case do not set it, otherwise default to env.DOMAIN - I don't think what you propose will do that.

- name: COOKIE_DOMAIN
value: {{.Values.console.cookieDomain}}
{{- end }}
{{- else if .Values.env.DOMAIN }}
Copy link
Contributor

Choose a reason for hiding this comment

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

{{end}} can be removed if the first edit is made

@@ -13,6 +13,7 @@ dbProvider: mysql
useLb: false
console:
port: 443
cookieDomain:
# externalIP: 127.0.0.1
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not set this to '-'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because setting it to -would mean we don't pick up from env.DOMAIN

@irfanhabib irfanhabib added needs attention This PR needs attention and removed ready for review labels Jun 20, 2018
@nwmac nwmac added comments-addressed and removed needs attention This PR needs attention labels Jun 20, 2018
@irfanhabib irfanhabib merged commit 263b8fb into v2-master Jun 21, 2018
@irfanhabib irfanhabib deleted the cookie-domain branch June 21, 2018 08:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants