Session cookie need to be visible to OP IFrame Javascript script. #731

rohe · 2019-12-20T16:22:56Z

And even if no back-/frontchannel logout is defined the session cookie should be removed.

Any changes relevant to users are recorded in the CHANGELOG.md.
The documentation has been updated, if necessary.
New code is annotated.
Changes are covered by tests.

Even if no back-/frontchannel logout is defined the session cookie should be removed.

schlenk · 2019-12-21T09:25:09Z

src/oic/oauth2/provider.py

-    def write_session_cookie(self, value):
-        return make_cookie(self.session_cookie_name, value, self.seed, path="/")
+    def write_session_cookie(self, value, http_only=True):
+        return make_cookie(self.session_cookie_name, value, self.seed, path="/", httponly=http_only)


If IFrames are the usecase, we might want to handle SameSite settings for cookies as well.
Otherwise it will probably break with newer browsers like Chrome 80 that implement SameSite=Lax by default.

(https://chromestatus.com/features#samesite),
https://textslashplain.com/2019/09/30/same-site-cookies-by-default/

IFrames is the use case. I'll look at SameSite. Any preference as how to handle it ?

The make_cookie() method should handle and explitly set a SameSite value. This may requires monkey patching the Cookie class of Python (see Beaker/Cookie class), as it is only supported in 3.8+.

The patch is like this:
from http_cookies import Morsel Morsel._reserved[str('samesite')] = str('SameSite')

In IFrames usage the only value for SameSite that works properly is 'None'.
https://web.dev/samesite-cookie-recipes/
or mentioning it might break OpenID connect logins:
https://devblogs.microsoft.com/aspnet/upcoming-samesite-cookie-changes-in-asp-net-and-asp-net-core/

Ran isort, blacken, mypy and pylama.

…al formatting.

codecov-io · 2020-01-15T09:25:59Z

Codecov Report

Merging #731 into master will increase coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #731      +/-   ##
==========================================
+ Coverage   63.87%   63.91%   +0.03%     
==========================================
  Files          64       64              
  Lines       11688    11688              
  Branches     2060     2060              
==========================================
+ Hits         7466     7470       +4     
+ Misses       3653     3649       -4     
  Partials      569      569

Impacted Files	Coverage Δ
src/oic/oauth2/message.py	`73.72% <100%> (ø)`	⬆️
src/oic/utils/http_util.py	`69.82% <0%> (+0.57%)`	⬆️
src/oic/utils/authn/user.py	`64.22% <0%> (+0.81%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f2866d6...65873af. Read the comment docs.

rohe · 2020-01-15T09:38:47Z

If we need monkey patching we an add that later.

rohe · 2020-01-16T07:58:09Z

The CI fail has nothing to do with my PR.
Hesitate to fix it here.

tpazderka · 2020-01-16T08:25:14Z

First of all, this needs to be rebased on top of actual master.

The CI fail is caused by the added monkeypatch. Mypy doesn't like the access to internal methods. It probably needs to be ignored in this case.

Even if no back-/frontchannel logout is defined the session cookie should be removed.

Ran isort, blacken, mypy and pylama.

…al formatting.

rohe · 2020-01-20T08:16:39Z

I don't like it either but it was a request from Mikael to add SameSite support and I don't think it can be done without the monkey patch.

…into session_management

rohe · 2020-01-20T08:52:12Z

It's now rebased.

tpazderka · 2020-01-20T08:54:37Z

You probably need to add # type: ignore to the "offending" line to make MyPy happy.

rohe · 2020-01-29T08:00:54Z

Any estimate on when this will be approved ??

schlenk

LGTM now.

tpazderka · 2020-01-30T08:34:23Z

CHANGELOG.md

@@ -24,10 +24,12 @@ The format is based on the [KeepAChangeLog] project.
 - [#711] Deal with no post_logout_redirect_uri
 - [#712] Set Content-Type on BackChannel logout POST.
 - [#717] Missing OP logout metadata.
+- [#731] Session cookie need to be visible to OP IFrame.


This needs to be moved to the Unreleased part together with the link.

…into session_management

tpazderka · 2020-01-31T09:26:43Z

Here is an overview of what got changed by this pull request:

Issues
======
+ Solved 2

See the complete overview on Codacy

rohe added 2 commits December 20, 2019 17:22

Session cookie need to be visible to OP IFrame Javascript script.

3242d08

Even if no back-/frontchannel logout is defined the session cookie should be removed.

Updated CHANGELOG.md .

dcbf2fa

schlenk reviewed Dec 21, 2019

View reviewed changes

rohe added 3 commits January 10, 2020 10:13

Added SameSite.

ae34ee4

Fixed test.

5c8817c

Ran isort, blacken, mypy and pylama.

Added mapping from the lowercase variant to the appropriate tradition…

8b92bb6

…al formatting.

Set same_site to "None"

a3e858d

rohe added 2 commits January 15, 2020 12:19

So we need the monkey.

899c338

So we need the monkey. - testing variant

0a50887

rohe added 8 commits January 20, 2020 09:03

Session cookie need to be visible to OP IFrame Javascript script.

515da33

Even if no back-/frontchannel logout is defined the session cookie should be removed.

Updated CHANGELOG.md .

186978c

Added SameSite.

31422b5

Fixed test.

d999cf1

Ran isort, blacken, mypy and pylama.

Added mapping from the lowercase variant to the appropriate tradition…

942a15e

…al formatting.

Set same_site to "None"

53d9c29

So we need the monkey.

38a2c90

So we need the monkey. - testing variant

64b9181

rohe added 2 commits January 20, 2020 09:18

Merge branch 'master' into session_management

371195c

Merge branch 'session_management' of https://github.com/OpenIDC/pyoidc …

59d4137

…into session_management

To make mypy happy.

f2866d6

Merge branch 'master' into session_management

ea45547

tpazderka requested a review from schlenk January 29, 2020 08:36

schlenk approved these changes Jan 29, 2020

View reviewed changes

tpazderka requested changes Jan 30, 2020

View reviewed changes

rohe added 2 commits January 31, 2020 10:23

Change registered under unreleased.

ffe409b

Merge branch 'session_management' of https://github.com/OpenIDC/pyoidc …

65873af

…into session_management

tpazderka approved these changes Feb 1, 2020

View reviewed changes

tpazderka merged commit bac06a4 into master Feb 1, 2020

tpazderka deleted the session_management branch February 1, 2020 14:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Session cookie need to be visible to OP IFrame Javascript script. #731

Session cookie need to be visible to OP IFrame Javascript script. #731

rohe commented Dec 20, 2019

schlenk Dec 21, 2019

rohe Jan 10, 2020

schlenk Jan 10, 2020

codecov-io commented Jan 15, 2020 •

edited

rohe commented Jan 15, 2020

rohe commented Jan 16, 2020

tpazderka commented Jan 16, 2020

rohe commented Jan 20, 2020

rohe commented Jan 20, 2020

tpazderka commented Jan 20, 2020

rohe commented Jan 29, 2020

schlenk left a comment

tpazderka Jan 30, 2020

tpazderka commented Jan 31, 2020

Session cookie need to be visible to OP IFrame Javascript script. #731

Session cookie need to be visible to OP IFrame Javascript script. #731

Conversation

rohe commented Dec 20, 2019

schlenk Dec 21, 2019

Choose a reason for hiding this comment

rohe Jan 10, 2020

Choose a reason for hiding this comment

schlenk Jan 10, 2020

Choose a reason for hiding this comment

codecov-io commented Jan 15, 2020 • edited

Codecov Report

rohe commented Jan 15, 2020

rohe commented Jan 16, 2020

tpazderka commented Jan 16, 2020

rohe commented Jan 20, 2020

rohe commented Jan 20, 2020

tpazderka commented Jan 20, 2020

rohe commented Jan 29, 2020

schlenk left a comment

Choose a reason for hiding this comment

tpazderka Jan 30, 2020

Choose a reason for hiding this comment

tpazderka commented Jan 31, 2020

codecov-io commented Jan 15, 2020 •

edited