ampproject / amphtml

The AMP web component framework.
https://amp.dev
Apache License 2.0
14.89k stars 3.89k forks source link

[amp-analytics] missing Support for Session cookies #34937

Open Gott50 opened 3 years ago

Gott50 commented 3 years ago

Description

Currently it is not possible to create Session cookies, because cookieMaxAge is only implemented to handle Number

Example:

I would like to create a Session cookie. That could look like this:

Reproduction Steps

Try creating a Session cookie. For Example like this:

Relevant Logs

No response

Browser(s) Affected

No response

OS(s) Affected

No response

Device(s) Affected

No response

AMP Version Affected

No response

micajuine-ho commented 3 years ago

@Gott50 Are you proposing to make the session expiration time indefinite? What is the use case?

Gott50 commented 3 years ago

@Gott50 Are you proposing to make the session expiration time indefinite? What is the use case?

@micajuine-ho Yes, I would like to set a cookie without an expires value. This is called a session cookie. https://www.cookiepro.com/knowledge/what-is-a-session-cookie/

Usually I would just set it with JS, like this: document.cookie = "cname=cvalue" but that is not possible in AMP :/

micajuine-ho commented 3 years ago

Is using the default 1year expiration period not desirable?

Gott50 commented 3 years ago

Is using the default 1year expiration period not desirable?

unfortunately only Session cookies are allowed without asking the user, because of GDPR https://en.wikipedia.org/wiki/General_Data_Protection_Regulation

micajuine-ho commented 3 years ago

I'm sorry, I'm a little confused. Are you only supposed to set the session cookie after you have consent from the user? Are you using amp-consent?

Gott50 commented 3 years ago

I'm sorry, I'm a little confused. Are you only supposed to set the session cookie after you have consent from the user? Are you using amp-consent?

No, if the user has not consented, yet, we can only use session cookies, because they are not persistent. After we have the consent of the user, we can use "regular" cookies that last longer than the session (e.g. 1 year)

micajuine-ho commented 3 years ago

No, if the user has not consented, yet, we can only use session cookies, because they are not persistent.

So you want to set a session cookie that is reset every time a user that has not consented enters a page?

What information do you want to store in the cookie?

samouri commented 3 years ago

@Gott50: supporting a Session cookie sounds reasonable to me. @micajuine-ho, do you think this FR should require an I2I or is it small enough to not need one?

Gott50 commented 3 years ago

@Gott50: supporting a Session cookie sounds reasonable to me. @micajuine-ho, do you think this FR should require an I2I or is it small enough to not need one?

This use case is really necessary in the EU because of GDPR and we need that feature ASAP. That's why I have also implemented the solution already. It is a really simple and short fix. https://github.com/ampproject/amphtml/pull/34938

Please let me know, if there is anything else I can do to help with the Release of this Feature.

micajuine-ho commented 3 years ago

@Gott50 I'm still confused about what this feature entails, looking at your PR, you're setting a cookie with no expiration, and that doesn't really make sense to me, within the context of the CookieWriter module and your use case.

Can you please explain what information do you want to store in the cookie?

@samouri It would at least need to be taken to design review, since we're dealing with storage of data.

samouri commented 3 years ago

@micajuine-ho, according to mdn docs on cookie.set, a cookie without an expirationDate is a "session cookie".

I agree that this should be brought to design review, even if just to formalize/agree upon the API.

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.