-
Notifications
You must be signed in to change notification settings - Fork 324
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
Leave domain empty in cookies. #1076
Conversation
@tiago-loureiro you asked for this, now you have to review it! ;-) |
hum, this looks like it could be related:
|
These tests pass locally, so the problem is something specific to the setup in which tests are run on concourse. My bet would be network setup. This would mean that either the concourse network setup needs to be fixed to accomodate implicit cookie domains, or it is right and catches an error here. Or is it something about bilge, which we use as the client lib we use for integration tests? |
05a7ccc fixes one test. the legal hold variant is using an internal end-point to login which can't just go through nginz. i'll take a break now and figure this out later. |
|
testNginzLegalHold = do | ||
nginz <- view tsNginz | ||
(owner, tid) <- createBindingTeam' | ||
(c, t) <- withDummyTestServiceForTeam (Brig.userId owner) tid $ \_chan -> do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious, trying to understand the code: What do you need the withDummyTestServiceForTeam
for? Wouldn't (c, t) <- legalHoldLoginViaNginz ...
work? (if not, why not?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in the test before this PR, we called an internal end-point in brig directly to get the credentials. that circumvented the integrity tests that (presumably) galley does before approving a LH device, including talking to the device and checking that its certificate and URL are valid. Now that I need to go through nginz so that the cookie is issued by the same host that will get it back further down in the test, these integrity tests are happening, and fail.
i should have probably double-checked this theory, but that's my mental model, based on an error message to that end that i saw at some point today.
The problem is that
|
cool, thanks! the next one feels more like something i got wrong during the heavy refactoring.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good now!
This makes UAs default to server host (plus sub-domains).
Fixes https://github.com/zinfra/backend-issues/issues/953