Skip to content
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

Enable cookie support after the SDK has been initialized #1091

Closed
wham opened this issue Oct 17, 2019 · 22 comments
Closed

Enable cookie support after the SDK has been initialized #1091

wham opened this issue Oct 17, 2019 · 22 comments
Assignees
Milestone

Comments

@wham
Copy link

wham commented Oct 17, 2019

We have a home page / with a cookie banner. We are not allowed to write cookies until the user consents. We initialize the SDK with isCookieUseDisabled=true. Once the user consents, we would like to immediately write the cookies so when they continue to the next page /sign-in, same session ID is used there.

We can call appInsights.properties.context.sessionManager.update() but there seems to be no way to set CoreUtils._canUseCookies to true once the SDK has been initialized.

@markwolff markwolff added the bug label Oct 17, 2019
@markwolff
Copy link
Contributor

Seems like a bug. Will get this in to next release (2.3.1)

@Schmaga
Copy link

Schmaga commented Nov 16, 2019

The fix did not make it into 2.3.1 as far as I can see. In the meantime, did anyone find a workaround for this, yet? Tried for two hours but failed miserably :)

@Schmaga
Copy link

Schmaga commented Dec 1, 2019

I managed to create a very hacky workaround. When using the following code, we seem to be able to activate cookies even after the SDK has been initialized. It only works by recreating a completely new app insights instance. like this:

activateCookies() {
    CoreUtils._canUseCookies = true;
    this.appInsights = new ApplicationInsights({
      config: {
        instrumentationKey: environment.appInsights.instrumentationKey,
        isCookieUseDisabled: false,
        enableAutoRouteTracking: true
      }
    });
    this.appInsights.loadAppInsights();
    this.appInsights.trackPageView();
}

@MSNev
Copy link
Collaborator

MSNev commented Dec 2, 2019

Note: This workaround will break in the near future as we are planning on hiding all of the properties that start with "_" as they are considered to be private and should not be set directly.

To fix this properly we will need to add a setter method to be able to set/reset this value.

@Schmaga
Copy link

Schmaga commented Dec 2, 2019

Hopefully you will not hide it without fixing the bug itself. But thanks for the heads up :) Maybe a reloadConfig method or so might help, in case other settings are also affected by that "set-once, change-never"-behavior.

@MSNev
Copy link
Collaborator

MSNev commented Dec 3, 2019

Noted.

Also while looking at some other indirectly related code, I just tripped over another issue with the workaround, because the _canUseCookies is actual an internal global (on the CoreUtils). if someone else creates a new instance of AppInsights and also sets the isCookieUseDisabled to true this will reset the _canUseCookies back to false (via calling CoreUtils.disableCookies()).

This means that all of the cookie functions become no-op...

So this also means that by your instance setting this any other instances included on your app/page are also affected, even if they are only using the Utils.setCookie(), Utils.getCookie(), Utils.deleteCookie() and Utils.canUseCookies().

So in your case if you only have 1 AppInsights instance and you are not including any other components that create their own app insights instance then this workaround will currently work (until we fix the issue). But if not then there will be a race (initialization order) condition which could cause cookies to be unexpectedly disabled for any other instances.

Notes for possible fix.
I'm thinking the real fix here is probably the following or a combination

  • remove the CoreUtils._canUseCookies and associated CoreUtils.disableCookies() static methods, keeping the Utils are cookies available settings detection.
  • perhaps also have "global" Utils settings for backward compatibility for now (we already have disableCookies() methods).
  • change the logic for the "isCookieUseDisabled" config to be AppInsights instance specific only
  • likewise in the ChannelController change the logic for "isCookieUseDisabled" config to be AppInsights instance specific only.
  • add setter (or enable/disable) function(s) to enable setting / disabling the instance to cookie setting.

@MSNev MSNev mentioned this issue Dec 3, 2019
@Schmaga
Copy link

Schmaga commented Dec 5, 2019

I think it is a good idea to remove global statics in general, except for maybe global defaults that can be changed on instance-basis. So +1 for removing those.

I also think an enable/disableCookie Function makes it explicit that you can actually do that on an existing instance. I would probably prefer that. But it would just somehow make sense to avoid confusion with the existing isCookieUseDisabled flag on the config object. Because having both the config property and the enable/disable methods could confuse users.

Maybe rename that config property as well to "enableCookies" and make it default to true? Just thinking out loud here...

@hiraldesai
Copy link

I just came across this as I have exactly the same problem with "cookie consent" scenario in a react application. Is the recommendation to load AI with isCookieUseDisabled = false first, then reload it with isCookieUseDisabled = true after getting user consent? I tried just changing the config upon consent directly but that doesn't seem to work.

@Schmaga
Copy link

Schmaga commented Jun 12, 2020

@hiraldesai, there is no way to re-enable cookies after you have initialized AI with cookies disabled. You have to use the workaround I described above by completely re-initializing AI after getting consent.

@wham
Copy link
Author

wham commented Sep 1, 2020

We worked with our privacy team and decided that we don't need this feature any more. Feel free to close if there are no other customers asking for it.

@omarsourour
Copy link

omarsourour commented Sep 7, 2020

Hello @MSNev ,

Due to the new Microsoft WCP Cookie Compliance banner, we are required to give the user the option to change their cookie preferences even after they had already consented to them. For example, the user might first consent to Telemetry cookies, but change their mind afterwards whilst using the website.

In order to support that, we need to:

  • Clean up the cookies created by the application insights library when the user chooses to no longer consent to telemetry cookies.
  • Re-initialize the application insights client object with cookies disabled.

My questions are:

  • Is there a native way in the API to do that cleanup?
  • Will simply reassigning the this.appInsights to a new instance of ApplicationInsights be enough?
  • Is there a way to update (disable/enable) the cookie flag without having to reinitialize the entire object?

@omarsourour
Copy link

Hello @MSNev ,

Any updates on this? There is a compliance deadline date across the company.

@MSNev
Copy link
Collaborator

MSNev commented Sep 9, 2020

Sorry, was off yesterday, quick answers

  • Is there an API to cleanup -- No
  • Will simply reassigning this.appInsights to a new instance -- depends, but probably not. If the initial instance hooks XHR etc, these will remain in effect.
  • Currently, you can disable Cookies via the Utils.disableCookies() or CoreUtils.disableCookies() functions, however, we don't have a "clean" way to re-enable them that is what we are going to look at as part of this issue

@omarsourour
Copy link

Thank you for your replies!

Is it ok to resort to manually delete the cookies? Would that break the calls to app insights?

@MSNev
Copy link
Collaborator

MSNev commented Sep 9, 2020

If you call disableCookies() first and then delete them that should not cause any issues

@MSNev
Copy link
Collaborator

MSNev commented Jan 22, 2021

Note: as per comment in #1463 investigate adding a new "cleanup cookies" function to remove any pre-existing cookies (even if cookie usage is disabled) to better support GDPR.

MSNev added a commit to MSNev/ApplicationInsights-JS that referenced this issue Mar 6, 2021
Refactor code to provide better tree shaking and minification of generated code microsoft#1076
Also addresses
- Disable Cookies microsoft#1125
- Ability to specify cookie Path so that AI works behind App Gateway microsoft#1434
MSNev added a commit to MSNev/ApplicationInsights-JS that referenced this issue Mar 6, 2021
Utils - Tree-Shaking enhancements
Refactor code to provide better tree shaking and minification of generated code microsoft#1076
Also addresses
- Disable Cookies microsoft#1125
- Ability to specify cookie Path so that AI works behind App Gateway microsoft#1434
MSNev added a commit to MSNev/ApplicationInsights-JS that referenced this issue Mar 8, 2021
Utils - Tree-Shaking enhancements
Refactor code to provide better tree shaking and minification of generated code microsoft#1076
Also addresses
- Disable Cookies microsoft#1125
- Ability to specify cookie Path so that AI works behind App Gateway microsoft#1434
MSNev added a commit to MSNev/ApplicationInsights-JS that referenced this issue Mar 8, 2021
Utils - Tree-Shaking enhancements
Refactor code to provide better tree shaking and minification of generated code microsoft#1076
Also addresses
- Disable Cookies microsoft#1125
- Ability to specify cookie Path so that AI works behind App Gateway microsoft#1434
MSNev added a commit to MSNev/ApplicationInsights-JS that referenced this issue Mar 9, 2021
Utils - Tree-Shaking enhancements
Refactor code to provide better tree shaking and minification of generated code microsoft#1076
Also addresses
- Disable Cookies microsoft#1125
- Ability to specify cookie Path so that AI works behind App Gateway microsoft#1434
MSNev added a commit to MSNev/ApplicationInsights-JS that referenced this issue Mar 10, 2021
Utils - Tree-Shaking enhancements
Refactor code to provide better tree shaking and minification of generated code microsoft#1076
Also addresses
- Disable Cookies microsoft#1125
- Ability to specify cookie Path so that AI works behind App Gateway microsoft#1434
MSNev added a commit to MSNev/ApplicationInsights-JS that referenced this issue Mar 10, 2021
Utils - Tree-Shaking enhancements
Refactor code to provide better tree shaking and minification of generated code microsoft#1076
Also addresses
- Disable Cookies microsoft#1125
- Ability to specify cookie Path so that AI works behind App Gateway microsoft#1434
MSNev added a commit to MSNev/ApplicationInsights-JS that referenced this issue Mar 11, 2021
Utils - Tree-Shaking enhancements
Refactor code to provide better tree shaking and minification of generated code microsoft#1076
Also addresses
- Disable Cookies microsoft#1125
- Ability to specify cookie Path so that AI works behind App Gateway microsoft#1434
MSNev added a commit to MSNev/ApplicationInsights-JS that referenced this issue Mar 12, 2021
Utils - Tree-Shaking enhancements
Refactor code to provide better tree shaking and minification of generated code microsoft#1076
Also addresses
- Disable Cookies microsoft#1125
- Ability to specify cookie Path so that AI works behind App Gateway microsoft#1434
MSNev added a commit to MSNev/ApplicationInsights-JS that referenced this issue Mar 12, 2021
Utils - Tree-Shaking enhancements
Refactor code to provide better tree shaking and minification of generated code microsoft#1076
Also addresses
- Disable Cookies microsoft#1125
- Ability to specify cookie Path so that AI works behind App Gateway microsoft#1434
MSNev added a commit to MSNev/ApplicationInsights-JS that referenced this issue Mar 12, 2021
Utils - Tree-Shaking enhancements
Refactor code to provide better tree shaking and minification of generated code microsoft#1076
Also addresses
- Disable Cookies microsoft#1125
- Ability to specify cookie Path so that AI works behind App Gateway microsoft#1434
MSNev added a commit that referenced this issue Mar 17, 2021
* Enable cookie support after the SDK has been initialized #1091
Utils - Tree-Shaking enhancements
Refactor code to provide better tree shaking and minification of generated code #1076
Also addresses
- Disable Cookies #1125
- Ability to specify cookie Path so that AI works behind App Gateway #1434

* Update Documentation and Core Perf Tests
- address circular dependencies

* Add missing api exports
@MSNev MSNev added the fixed - waiting release PR Committed and waiting deployment label Mar 17, 2021
@MSNev
Copy link
Collaborator

MSNev commented Mar 17, 2021

As part of the change that was just checked in, I've added some documentation on the readme but I thought here would also be a good location.
Moving forward, cookie support will be based on the appInsights instance so enabling disabling after initialization will be simply a case of using.
appInsights.getCookieMgr().setEnabled(value: boolean)

I've added support for the existing _canUseCookies, but usage is problematic in ES3 environments (won't always work) and it's still also global, so if you share an environment with another instance using this will affect other users. And for backward compatibility, disabling via this will still disable globally -- even for instance based cookies

@TimoWilhelm
Copy link

if anyone else is looking for this method, it's located under the core property at appInsights.core.getCookieMgr().setEnabled(value: boolean) in version 2.6.0

@MSNev
Copy link
Collaborator

MSNev commented Mar 25, 2021

Oh, nice catch...
I'll look to expose this on both the web analytics object and the snippet proxy (the actual sources of the global appInights object) as part of simple initialization (Id been working on the underlying support), this will be in the next release.

I'll update the documentation (hopefully today) to hightlight this.

@MSNev
Copy link
Collaborator

MSNev commented Mar 25, 2021

OK added new issue #1512 to address this gap as this issue will get closed once the release is fully deployed to all CDN endpoints

@MSNev MSNev removed the fixed - waiting release PR Committed and waiting deployment label Mar 29, 2021
@MSNev
Copy link
Collaborator

MSNev commented Mar 29, 2021

v2.6.0 is now fully deployed to all CDN endpoints

@MSNev MSNev closed this as completed Mar 29, 2021
@github-actions
Copy link

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants