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

Cookie consent#45 #47

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Cookie consent#45 #47

wants to merge 14 commits into from

Conversation

justin-b-yee
Copy link

@justin-b-yee justin-b-yee commented Aug 14, 2024

This change adds a mongo-collections model for storing user consent data, handles displaying service options to consent to, and running the code when consented.

@justin-b-yee justin-b-yee linked an issue Aug 14, 2024 that may be closed by this pull request
6 tasks
@justin-b-yee
Copy link
Author

Hi @ddfridley:

This is what I have on the consent model's schema so far:

  1. Requires at least 1 "who" identifier to create.
  2. Consent doc is updated by passing a "who" key and value to lookup, along with new consent info.
  3. New consent info to update is passed as a structured list of { category, isGranted, terms }.
  4. Each consent category has it's own history. The old value is pushed every time it's updated via the functions via updateConsent().
Consent = {
    who: { 
        userId = USER_ID, // must have at least one of these 
        ipAddress: IP_ADDRESS,
        ...otherIds,
    },
    what: {
      category1: {
        isGranted: true,
        consentDate: date1,
        terms: "This is a term you've agreed to."
        history: [{
         ...previous data
        }]
      },
      category2: {
        isGranted: false,
        consentDate: date2,
        term: "This is another term you've also agreed to."
        history: []
      }
      ... // more categories
    }
}

For this task I see the Enciv Home branch changes already includes the Google Analytics code.

  • app/server/routes/react-server-render will need to be modified so that the things that require cookies are not run, but that code is incorporated into cookie-consent

I was trying to understand what's left to be done - I read in the docs onConsent() runs on every subsequent page load, so does this already functionally do what's done in react-server-render, and would the change there be to remove the hardcoded push of the Google Analytics code?

if (process.env.GOOGLE_ANALYTICS) {
window.dataLayer = window.dataLayer || []
window.gtag = function () {
dataLayer.push(arguments)
}
gtag('js', new Date())
gtag('config', `${process.env.GOOGLE_ANALYTICS}`)
const script = document.createElement('script') // create a script DOM node
script.src = `https://www.googletagmanager.com/gtag/js?id=${process.env.GOOGLE_ANALYTICS}`
script.id = 'googletagmanager' // so we can find it and delete it if needed
document.head.appendChild(script)
}
} else {

serverReactRender.head.push(googleAnalytics)

@ddfridley
Copy link
Contributor

The code for google analytics above should only run if the user has given the appropriate consent. The cookie consent code has some hooks for executing code based on what the user has agreed to.

See https://cookieconsent.orestbida.com/advanced/manage-scripts.html#using-callbacks-events

CookieConsent.run({
    onConsent: function(){
        if(CookieConsent.acceptedCategory('analytics')){
            // Analytics category enabled
        }

        if(CookieConsent.acceptedService('Google Analytics', 'analytics')){
            // Google Analytics enabled
        }
    }
});

@justin-b-yee
Copy link
Author

justin-b-yee commented Aug 16, 2024

I found documentation in the CookieConsent repo that I couldn't find on the site to display individual service toggles.

It seems to work without using onConsent to loop through the changes - the GA code is running on every load only when I accept in the modal.

[x] think about, but not required, how a new module could add additional things that require consent, without having to edit an existing module.

I was wondering if we could do something similar to how iotas.json works to update the services. I'll mark this for review in the meantime.

@justin-b-yee justin-b-yee marked this pull request as ready for review August 16, 2024 16:33
@justin-b-yee justin-b-yee requested a review from ddfridley August 16, 2024 16:33
Copy link
Contributor

@ddfridley ddfridley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am very impressed by all the stuff you figured out to do in this server, and all the validation in the consent model.

One problem I see is that when I run dev, and then go to the localhost:3011/join and accept the cookies, nothing shows up on the database. The consent collection isn't there.
I use MongoDB Compass to manually look at what's in the DB.

The other consideration is mentioned in the code comments, we need to make a React Component out of this, that can be included in App in the other repos.

Looking good. Thanks.

app/components/app.jsx Outdated Show resolved Hide resolved
app/models/consent.js Outdated Show resolved Hide resolved
@@ -129,6 +117,7 @@ function serverReactRender(App, req, res, next) {
})()
)})</script>
${helmet.script.toString()}
<link rel="stylesheet" href="https://cdn.jsdelivr.net/gh/orestbida/[email protected]/dist/cookieconsent.css">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be taken out of here done using ReactHelmet in the EnCivCookies component.

@justin-b-yee
Copy link
Author

One problem I see is that when I run dev, and then go to the localhost:3011/join and accept the cookies, nothing shows up on the database. The consent collection isn't there. I use MongoDB Compass to manually look at what's in the DB.

I'll get the callbacks connected to the DB logging function next!

I did have a question about identifying users - where do we actually get the ID data from like userId, IP address, etc to save? I looked around but had difficulty determining where to retrieve them from within app.jsx's (and soon-to-be encivcookies.js's) scope.

@ddfridley
Copy link
Contributor

Its not well documented, but if the user is logged in App will receive the user prop,, with email, id, and tempId

user: { email: "", id: "", tempId:""}

tempId is like an id but the users hasn't left an email address yet. But for consent you could use id || tempId.

For the IP address, that's a bit more of a challenge. seems it's hard to get on the client side.

But on the server side, this is probably going to make a socket-api call to tell the server to log the consent.
At the api, this.synuser use the id.

For the IP address, app/server/routes/iotas.js creates a browserConfig props that is also passed to App, that has the ip address. It gets it from express req.headers['x-forwarded-for'] || req.connection.remoteAddress, // Get IP - allow for proxy

In the socketApi function, this has a lot of stuff in it. I wonder if req is there. In the ideal it would be best not to trust the client, and just get the user id and IP address on the server side. Try setting a breakpoint in an api function, maybe socketlogger, and see what's in this.

@justin-b-yee
Copy link
Author

I formatted and tried to save the data but am a bit confused with the actual API call - I was wondering if I'm missing something?

I created another file under socket-apis - however the "this" variable only had synuser, whereas socketlogger's "this" had everything including the IP. It also seems I can't use any of the mongo/self-defined functions like findOne() as Consent was just equal to {}.

I saw in client-side-model.js there's an explanation and export - Consent had data again once I added to it, but the functions still weren't defined. I also thought calling the save consent API would be performed on the server side but I saw the comment mentions "for code that runs on the client side" - so I'm a bit unclear on that as well.

@ddfridley
Copy link
Contributor

The Consent model will not work on the browser side. It will only run on the server side -- in the socket-api directory. clicent-side-models is there to make webpack happy when building the client side code.

So there will have to be a an api call from the client side code to the server side.

To call an api from the browser side do window.socket.emit('socket-api-file', arg1, arg2, ..., callback)

Then in the socket-apis directory, create a file with the corresponding name, with a function to accept the call.

I can't see why this would be different for one api vs another, could it be the difference between running dev vs running jest? If its there for the socket-logger then we should be able to make it work for a new api. -- But this that I'm talking about is only for things in the socket-apis directory.

Let me know if you want a more details.

@justin-b-yee
Copy link
Author

justin-b-yee commented Aug 24, 2024

I recall trying window.socket.emit() but I couldn't access window, so I switched to saveConsent.call() at one point. I can use emit() now though - maybe it was a "turn it off and on" type situation. The callback I pass doesn't show anything though, nor do the logs, it seems like saveConsent isn't running? Not sure if this warning is related too.

  window.socket.emit('saveConsent', synuser, formattedConsentData , () => {
    console.log('DONE')
  })
 WARNING: Unhandled Socket API Event: 20["saveConsent",{"synuser":{"id":"anonymous"}},.other consent data..})

Before I checked the this variable for both APIs with npm run dev, with socketlogger triggered on run and saveconsent triggered when I Accept/Reject All. I noticed socketlogger printed to the VSCode console, while saveConsent logged to the browser's console.

I defined saveConsent in the same socket-apis/ folder, so that's why I was confused why it couldn't access the Consent model when I did saveConsent.call(). I thought it was almost as if the code I expected to run server-side was being treated as if it were client-side.

I switched back to socket.emit() and am trying to resolve the present not-called issue - but I'd definitely be curious to learn why these occurred!

@ddfridley
Copy link
Contributor

If you can't access window, that means the code is running on the server. Pages are first rendered on the server then passed to the browser and react-rehydrated where the code will run subsequently. Calls to the api generally need to be inside a useEffect method. Then they will only run on the browser, and they won't get called repeatedly (like on rerender).

emit('saveConsent' would need to be 'save-consent' and the file name would need to be save-consent.js

different file systems do different things with mixed case names - so we are always using lower case file names, and the emit handle needs to match the first part of the file name. the name of the function inside the file can be whatever.

I hope this helps. This is all interesting stuff - server vs client side and in this project some of the code runs on both.

@ddfridley
Copy link
Contributor

This diagram might be helpful. I go though it on dev calls sometimes, but probably not recently. https://www.figma.com/design/dYNc6vBmKXY1mZWiSV8qrI/civil-server-render-and-api?node-id=10-73&t=hSwc10R4SUpISqpD-4

I should find a way to get linked to from github.

@justin-b-yee
Copy link
Author

Thanks for the insight! This makes sense to me now - I forgot about the server rendering the code first.

I finished separating the cookies as a component and added a Helmet with the link, I've just been figuring why helmet.link seems to be empty even after this to finish up.

Copy link
Contributor

@ddfridley ddfridley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great. AFter you fix the react-helmet thing I still have more studying to do, and probably rewording some text and stuff. But this is really great!

@@ -0,0 +1,172 @@
import React, { useEffect, useState, useRef } from 'react'
import Helmet from 'helmet'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be from 'react-helmet'. there is also a helmet - which is security for node. I bet this is why helmet.link is empty.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that fixed it. Thanks for catching that!

Copy link
Contributor

@ddfridley ddfridley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This look great. I made one fix - see the inline comment. I'm going to approve this, but not merge the PR yet. I added an issue for myself to review the text in enciv-cookies, and another issue to suppress the popup if funning within an iframe.

I feel the need to proceed carefully with this change because because it crosses a lot of places.

Thanks!

{
label: 'Google Analytics',
onAccept: () => {
if (window.process.env.GOOGLE_ANALYTICS) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed this from process.env to window.process.env because it was getting a different value. I added a an issue to civil-client to fix the code in main to use process.env rather then global.env - but this fixes the immediate problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GDPR Cookie Consent
2 participants