Leave domain empty in cookies, but account for clients with old cookies. #1102

fisx · 2020-05-18T15:16:16Z

Fixes https://github.com/zinfra/backend-issues/issues/953

#1076 breaks UX: clients attempting to get new tokens from /access with cookies they got before this change will receive an error, and will have to login again.

This PR does it right.

When multiple cookies are presented to the backend, all of them will be tested against the database. If any of them is valid, then we pick the "first" one and use that for auth purposes.

fisx · 2020-05-18T15:16:58Z

I've opened this with the changes from the earlier PR. Now we just need to figure out how to do it right.

fisx · 2020-05-18T15:20:58Z

@mythsunwind writes:

The current issues with our QA environments and former-used accounts are happening, because when /login is accessed, the browsers do not override the old cookie with ".zinfra.io" with the new cookie. Instead they then have two cookie. One cookie with a working token and Domain:"staging-nginz-https.zinfra.io" and one with an outdated cookie and Domain:.zinfra.io. Unfortunately it seems both Firefox and Chrome then send two zuid tokens which results in a 403 on backend.

So is it enough to just allow for the old cookie to be present, but ignore it?

mythsunwind · 2020-05-18T16:27:27Z

So is it enough to just allow for the old cookie to be present, but ignore it?

Here is an example of the Cookie header of a request headers done by Firefox when the staging backend had the issue:

zuid=O49aSGfTwRWCjOlIUzJSSI0S2OqQRx0qtj2wjo6YExDrWDKrCN02dd97Pux7PQCmm8bWYoRWTBe_rs1S0mQOBA==.v=1.k=1.d=1593608983.t=u.l=.u=a7adc996-f698-4275-b0e4-1980f7105fba.r=8ac45588; zuid=qqme0VEmQkgR1l6ZIw-o3BO-OBbYsujB5PgTWOXNAFs69ffHrt1sPIiN3ajC1DfBut7l86qi4XHvsgGaP77UDg==.v=1.k=1.d=1594647026.t=u.l=.u=26a193b9-c4de-4671-9140-71470651bf6d.r=1bd7e045

I hope this helps you.

fisx · 2020-05-19T15:09:03Z

106816c does this:

            test/integration/API/User/Auth.hs:213:
            every login must issue a new cookie

I'm probably missing something obvious?

fisx · 2020-09-04T05:09:21Z

services/brig/test/integration/API/User/Auth.hs

+  let Just email = userEmail u
+      dologin :: HasCallStack => Http ResponseLBS
+      dologin = login n (defEmailLogin email) PersistentCookie <!! const 200 === statusCode
+  outdatedCookie <- decodeCookie <$> dologin


we need to make the outdated cookie look like a legacy cooikie! ie., different domain.

We just need to send 2 cookies

This reverts commit 14fb65f.

fisx · 2020-09-04T05:12:49Z

services/spar/src/Spar/API.hs

@@ -69,7 +69,6 @@ import Spar.Scim
 import Spar.Scim.Swagger ()
 import Spar.Types
 import qualified URI.ByteString as URI
-import qualified Web.Cookie as Cky


also test that this behaves as expected!

fisx · 2020-09-04T05:31:00Z

services/brig/brig.integration.yaml

@@ -135,7 +135,6 @@ optSettings:
  setNexmo: test/resources/nexmo-credentials.yaml
  # setStomp: test/resources/stomp-credentials.yaml
  setUserMaxConnections: 16
-  setCookieDomain: 127.0.0.1


move this to integration.yaml

tiago-loureiro · 2020-09-08T20:21:30Z

This is now ready for review. Could someone please run ormolu against it?

tiago-loureiro · 2020-09-10T07:20:25Z

This is now ready for review. Could someone please run ormolu against it?

Nevermind, all good

arianvp · 2020-09-10T09:08:30Z

services/brig/src/Brig/Options.hs

@@ -411,9 +411,6 @@ data Settings = Settings
    -- | Max. number of permanent clients per user
    setUserMaxPermClients :: !(Maybe Int),
    -- | The domain to restrict cookies to
-    setCookieDomain :: !Text,
-    -- | Whether to allow plain HTTP transmission
-    --   of cookies (for testing purposes only)


You removed the wrong comment

arianvp · 2020-09-10T09:08:56Z

services/brig/src/Brig/Provider/API.hs

@@ -1045,7 +1045,7 @@ setProviderCookie t r = do
      Cookie.def
        { Cookie.setCookieName = "zprovider",
          Cookie.setCookieValue = toByteString' t,
-          Cookie.setCookieDomain = Just $ Text.encodeUtf8 . setCookieDomain $ s,
+          Cookie.setCookieDomain = Nothing,


can just leave this line; Cookie.def already contains Cookie.setCookieDomain = Nothing

services/brig/src/Brig/User/API/Auth.hs

services/brig/src/Brig/User/Auth.hs

arianvp · 2020-09-10T09:23:11Z

services/brig/src/Brig/User/Auth.hs

+validateTokens [] _ = throwE ZAuth.Invalid
+validateTokens (ut : []) at = validateToken ut at
+validateTokens uts at = do
+  tokens <- forM uts $ \ut -> lift $ runExceptT (validateToken ut at)


Suggested change

tokens <- forM uts $ \ut -> lift $ runExceptT (validateToken ut at)

tokens <- forM uts $ \ut -> (validateToken ut at)

lift . runExceptT is just id I think? No need to runExceptT and lift if validateToken and validateTokens are both already an ExceptT ?

We discussed it f2f; the reason is that we don't want to fail right away so the logic is correct. Will try to tackle the previous point as a cleanup.

services/brig/src/Brig/User/Auth/Cookie.hs

services/brig/test/integration/API/User/Auth.hs

services/brig/src/Brig/User/Auth.hs

tiago-loureiro · 2020-09-14T21:45:15Z

This is good to go from my end, all PR feedback was addressed now.

mheinzel

Just one small formatting nit, not a blocker. Looks good to me! :)

jschaul

Looks good to me, too.

fisx changed the title ~~Leave domain empty in cookies, but account for clients with old cookies.~~ [WIP] Leave domain empty in cookies, but account for clients with old cookies. May 19, 2020

fisx force-pushed the fisx/cookies-the-2nd branch from 0cd5079 to 106816c Compare May 19, 2020 15:07

fisx force-pushed the fisx/cookies-the-2nd branch 2 times, most recently from cf34bd0 to 5cf5d6c Compare May 25, 2020 14:37

fisx self-assigned this May 25, 2020

fisx force-pushed the fisx/cookies-the-2nd branch 3 times, most recently from ca23b06 to 46a6f79 Compare August 3, 2020 09:41

fisx commented Sep 4, 2020

View reviewed changes

fisx added 3 commits September 4, 2020 07:11

Recover "Leave domain empty in cookies. (#1076)"

7aefaa3

This reverts commit 14fb65f.

Cleanup.

b19fe6a

Failing test case. (unfortunately it is not failing)

531cda3

fisx commented Sep 4, 2020

View reviewed changes

...

860b2dc

fisx commented Sep 4, 2020

View reviewed changes

fisx force-pushed the fisx/cookies-the-2nd branch from 46a6f79 to 860b2dc Compare September 4, 2020 05:32

tiago-loureiro added 4 commits September 8, 2020 01:40

Add failing test when using 2 cookies

3d47172

Added comment

e17422e

Added support for multiple cookies

e7667ad

Comments and clean up

a28e83e

tiago-loureiro changed the title ~~[WIP] Leave domain empty in cookies, but account for clients with old cookies.~~ Leave domain empty in cookies, but account for clients with old cookies. Sep 8, 2020

tiago-loureiro marked this pull request as ready for review September 8, 2020 20:20

tiago-loureiro assigned arianvp Sep 9, 2020

Comments and clean up

f2aaa21

tiago-loureiro force-pushed the fisx/cookies-the-2nd branch from 9042b0d to f2aaa21 Compare September 10, 2020 05:19

Ran ormolu

649cecc

arianvp reviewed Sep 10, 2020

View reviewed changes

tiago-loureiro added 2 commits September 11, 2020 00:49

Do not fail so early when parsing cookies

6841dff

Address PR feedback

b94c80e

arianvp reviewed Sep 11, 2020

View reviewed changes

services/brig/src/Brig/User/Auth.hs Show resolved Hide resolved

Address PR feedback

b05e536

tiago-loureiro assigned akshaymankar and jschaul Sep 14, 2020

One more assertion

d2b5d65

mheinzel approved these changes Sep 15, 2020

View reviewed changes

Auth should always go through nginz

23d29b8

jschaul approved these changes Sep 15, 2020

View reviewed changes

tiago-loureiro merged commit 4acab3b into develop Sep 16, 2020

tiago-loureiro deleted the fisx/cookies-the-2nd branch September 16, 2020 07:02

jschaul mentioned this pull request Oct 5, 2020

Release 2020 10 05 #1214

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Leave domain empty in cookies, but account for clients with old cookies. #1102

Leave domain empty in cookies, but account for clients with old cookies. #1102

fisx commented May 18, 2020 •

edited by tiago-loureiro

Loading

fisx commented May 18, 2020

fisx commented May 18, 2020

mythsunwind commented May 18, 2020

fisx commented May 19, 2020

fisx Sep 4, 2020

tiago-loureiro Sep 8, 2020

fisx Sep 4, 2020

fisx Sep 4, 2020

tiago-loureiro Sep 8, 2020

tiago-loureiro commented Sep 8, 2020

tiago-loureiro commented Sep 10, 2020

arianvp Sep 10, 2020

tiago-loureiro Sep 10, 2020

arianvp Sep 10, 2020

tiago-loureiro Sep 14, 2020

arianvp Sep 10, 2020

tiago-loureiro Sep 10, 2020

tiago-loureiro commented Sep 14, 2020

mheinzel left a comment

jschaul left a comment

	tokens <- forM uts $ \ut -> lift $ runExceptT (validateToken ut at)
	tokens <- forM uts $ \ut -> (validateToken ut at)

Leave domain empty in cookies, but account for clients with old cookies. #1102

Leave domain empty in cookies, but account for clients with old cookies. #1102

Conversation

fisx commented May 18, 2020 • edited by tiago-loureiro Loading

fisx commented May 18, 2020

fisx commented May 18, 2020

mythsunwind commented May 18, 2020

fisx commented May 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiago-loureiro commented Sep 8, 2020

tiago-loureiro commented Sep 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiago-loureiro commented Sep 14, 2020

mheinzel left a comment

Choose a reason for hiding this comment

jschaul left a comment

Choose a reason for hiding this comment

fisx commented May 18, 2020 •

edited by tiago-loureiro

Loading