IdentityPython / SATOSA

Proxy translating between different authentication protocols (SAML2, OpenID Connect and OAuth2)
https://idpy.org
Apache License 2.0
200 stars 123 forks source link

feat: cookie parameters in proxy configuration #363

Closed peppelinux closed 1 year ago

peppelinux commented 3 years ago

This PR fixes https://github.com/IdentityPython/SATOSA/issues/344 introducing the following paramenters to be optionally handled in the proxy global configuration.

Secure Now can be disabled via COOKIE_SECURE: no in proxy_conf.yaml. Default: True that's only for dev purpose!

HttpOnly To avoid cross-site scripting (XSS) attacks, cookies set with the HttpOnly directive are inaccessible to the JavaScript Document.cookie API. For example, session cookies don't need to be accessed by JavaScript and should therefore be set with the HttpOnly flag. Default: True parameter name eg: COOKIE_HTTPONLY: no

Domain, COOKIE_DOMAIN Max-Age, COOKIE_MAX_AGE

it come also with some minor code linting and f-strings, few things.

All Submissions:

peppelinux commented 3 years ago

It seems that here we have a collision https://github.com/IdentityPython/SATOSA/blob/1def9986e5c32baa5f95be835cf5843d9f9ab5d8/src/satosa/frontends/saml2.py#L558

anyway, with my PR I tested in the user-agent debugger and it seems that SATOSA cookie respects the global configuration parameter, instead of the frontend ones.

I think that we should have to decide if keep cookie paramenters in the plugins or move them in the global configuration and refactor/cleanup/reduce all those hardcodings

c00kiemon5ter commented 3 years ago

@peppelinux we will be merging this PR.

The SimpleCookie created by the frontend is a separate one, it has a different purpose and should not conflict the cookie that the proxy is using to store state.

c00kiemon5ter commented 1 year ago

closed by 1206ea5

peppelinux commented 1 year ago

This PR should have been taken

Since the code committed Is equal or similar

There are some cases where a PR Is not perfect but it's 80% good

In these cases the mantainers can add their commits Upon the PR made by the community

This preserves the contribution in the history of the commits, giving a gratification to contributions

If this doesn't happen, what would be the value to contribute in a project?

This Is not project management or product design, this Is social Exchange and I think that our community needs this awareness, then we should be able to apply these mechanisms to gather contributions and value in the projects

c00kiemon5ter commented 1 year ago

@peppelinux you are right and I will try to do that in the future ❤️

peppelinux commented 1 year ago

Even if the priority Is the release (output, the "what") the process to achieve this is important (the how)

I'm Happy of this brand new release, at the same time we may had a pull request to understand the "what" and the "how"

We should also take in account that a release that introduces these features needs testing, because i dont know which Will be the impact of this release on the systems in production and there might be the possibility to test these in a PR/branch, before a release

This Is just for sharing, I look forward with you

Ad maiora