Add new orejime cookie for correlation-id header

[AndreaBarbasso]

References

• Fixes part of Discussion: How does the correlation id work? Do we need to change it? #3335

Description

I added a new cookie to orejime's settings. This cookie relates to the X-CORRELATION-ID request header. If the cookie is not accepted, no X-CORRELATION-ID header will be set on requests.
The cookie title and description are not final - I'm open to suggestions on how to make them more clear for non-technical users.

Instructions for Reviewers

You can check any request done while the cookie has not been accepted, and it won't have the X-CORRELATION-ID header.

If you then accept the cookie, you will see the X-CORRELATION-ID cookie back again.

List of changes in this PR:

• Added a new cookie to orejime's settings;
• Added a check for that cookie before setting up the X-CORRELATION-ID header in the LogInterceptor.

Checklist

• My PR is created against the main branch of code (unless it is a backport or is fixing an issue specific to an older branch).
• My PR is small in size (e.g. less than 1,000 lines of code, not including comments & specs/tests), or I have provided reasons as to why that's not possible.
• My PR passes ESLint validation using npm run lint
• My PR doesn't introduce circular dependencies (verified via npm run check-circ-deps)
• My PR includes TypeDoc comments for all new (or modified) public methods and classes. It also includes TypeDoc for large or complex private methods.
• My PR passes all specs/tests and includes new/updated specs or tests based on the Code Testing Guide.
• My PR aligns with Accessibility guidelines if it makes changes to the user interface.
• My PR uses i18n (internationalization) keys instead of hardcoded English text, to allow for translations.
• My PR includes details on how to test it. I've provided clear instructions to reviewers on how to successfully test this fix or feature.
• If my PR includes new libraries/dependencies (in package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.
• If my PR includes new features or configurations, I've provided basic technical documentation in the PR itself.
• If my PR fixes an issue ticket, I've linked them together.

[github-actions]

Hi @AndreaBarbasso,
Conflicts have been detected against the base branch.
Please resolve these conflicts as soon as you can. Thanks!

[AndreaBarbasso]

I'm fixing merge conflicts, then this issue should be ready (it was more or less blocked by #3963 - the functionality was there, but it could not be tested correctly because of the issues with the optOut property.).

[AndreaBarbasso]

I also added a little piece of code to disable orejime during e2e testing:

        if (this._window?.nativeWindow?.Cypress) {
          this.orejimeConfig.apps = [];
        } else {
          this.orejimeConfig.apps = this.filterConfigApps(appsToHide);
        }

This removes the need to update the cookie object in cypress' e2e.ts file, and also makes it work with cookies that are not yet approved for the test user (the correlation-id is one of them).

[tdonohue]

@AndreaBarbasso : I tested this and reviewed this today. Overall, it looks good, but I noticed a few things that need fixing:

• First, if I change my Cookie Settings to disable the Correlation ID, the CORRELATION-ID cookie is still created. I expected the cookie to be gone if I don't allow for Correlation ID tracking. I can verify though that the value in the CORRELATION-ID cookie is correctly not sent to the backend.
• Second, I'm not a fan of adding checks into our main codebase for whether Cypress is running. I feel this is "messy code". In the backend codebase we don't embed checks for whether code is running as part of an integration test or unit test. Similarly, I don't think we should embed checks to see if Cypress is running. A few possible ideas inline below.

Overall, this looks good though & works at the basic level (other than the cookie wrongly being created)

[tdonohue]

I'm not a fan of adding Cypress specific code into our main codebase (i.e. the ./src/ folder). Is there no way to do this in the Cypress code under ./cypress/?

I just feel this makes our code messy if we start to check whether Cypress is active or not in our codebase -- it also complicates our ability to move to a different e2e testing system in the future (if we ever need to do so), because we'll have to find everywhere that Cypress is now referenced in our code. In other words, I'd hate to see this sort of if (Cypress) check repeated all over our codebase.

An alternative might be to find a way to just disable Orejime altogether in Cypress (maybe via a passed in param or a configuration that we could set from Cypress).

[tdonohue]

@AndreaBarbasso and @atarix83 : I've just realized this same issue with Orejime in e2e tests may be appearing in #3613, where some solutions are offered. So, we may want to consider merging #3613 first (if one of you could help review/test it that'd help), as it may allow for a better way to solve this issue with Cypress e2e tests not working well with Orejime.

[AndreaBarbasso]

@tdonohue, I think the suggestion made on #3613 (adding all necessary accepted cookies to the test user in the e2e docker db) is the best solution, even if it will need continuous maintenance (i.e. adding each new non-mandatory cookie to the ones the user accepted). This way, there is no need for the action created in #3613 to accept orejime cookies, and we can fix #4106 with no backside.
If then we want to test the orejime cookies banner, we might want to create a new test user that has no cookies accepted.

[tdonohue]

@AndreaBarbasso : As clarified in today's Developers meeting, adding information to the e2e docker database is not a simple task (it requires updating the database dump in the demo-entites-data test data set) While it's possible to do so, I feel that approach is painful for the long term, as it would require pre-updating the e2e docker database anytime a new PR needs to add information to the orejime cookie.

I'd recommend we look at potentially merging #3613 first as it is currently passing e2e tests without having to update the e2e docker database. If we are lucky, maybe we'll find the approach taken in #3613 will also work for this PR... if so, that would mean there's no need to update the e2e docker database.

[AndreaBarbasso]

@tdonohue, the problem with merging #3613 first is that, as soon as #4106 is fixed, e2e tests will no longer work as expected. The fix used in #3613 to make tests pass is actually a workaround that relies on the fact that we have the #4106 bug.

What we have now is:

• Cookies are no longer stored in user metadata when changed. #4106, yet to be fixed, which is (IMHO) a critical bug, even more critical as soon as we introduce non-required cookies;
• Accessibility settings page #3613, which implemented a workaround for another issue (i.e. we have cookies that are not accepted on the Test User profile, and so the Orejime popup shows up after logging in) by clicking on the Orejime consent banner after each login. This is working now because Cookies are no longer stored in user metadata when changed. #4106 is yet to be fixed, but as soon as it is fixed it will result in e2e tests failing, because after the first click on Orejime's banner we are going to save the cookie preferences on the User profile, and so the banner will no longer appear. This will make the cy.get of the banner fail consistently. @artlowel suggested to implement a way to check if the banner is rendered and - only if it is - to click on it, but this is not easy to do properly and, above all, strongly not recommended by Cypress;
• Add new orejime cookie for correlation-id header #3889 (this issue), that tried another approach on solving the Orejime banner issue, which I see why is not considered as the best possible approach.

The problem I see in merging #3613 first is that we are merging a workaround that will fail as soon as #4106 is fixed.
So, I think that the correct line of fixes and merges is:

1. Fix and merge Cookies are no longer stored in user metadata when changed. #4106 first (since it does not change how tests should be handled);
2. Then, find a way to solve the banner issue here, in Add new orejime cookie for correlation-id header #3889, and merge it;
3. Finally, revert the workaround on Accessibility settings page #3613 and merge it.

---

Now, about "how do we solve the banner issue", I can see a few options:

• We keep the check for window.Cypress. I see that this is kinda getting the code dirtier, but it's an approach suggested by Cypress and I think it will not be used a lot around the code;
• We can mimic the solution above and keep it cleaner by checking for an environment variable (e.g. environment.isTestEnvironment) instead of window.Cypress. The problem is that we should either use the current test environment configuration file or create a new one, and make sure that current e2e tests will not fail, since they are now performed after a yarn run serve:ssr, which uses the default environment configuration file;
• Finally, we can change the Test User configuration on the e2e docker db, but this is time-consuming and it is not a solution that can be easily maintained.

@tdonohue, @artlowel, what are your thoughts on this?

[tdonohue]

@AndreaBarbasso : thanks for providing more information here. I'm not against your solutions to solve the banner issues, but I'd prefer the second one (checking for an environment variable) rather than hardcoding references to window.Cypress in our code.

Regarding the order of merger, do we have a solution for #4106 yet? It would be good to move that forward quickly if you feel that is the highest priority.. as I would hate to "block" the merger of these other related PRs for very long

[AndreaBarbasso]

@tdonohue, I opened a PR for #4106. If we manage to move that forward, I can make the requested changes on this PR and move it forward too!

[tdonohue]

I'd recommend a slightly difference wording for this text in order to make it clear that this is for backend logs. Maybe something like this:

"Allow us to track your session in backend logs for support/debugging purposes"

[github-actions]

Hi @AndreaBarbasso,
Conflicts have been detected against the base branch.
Please resolve these conflicts as soon as you can. Thanks!

[tdonohue]

@AndreaBarbasso : When you have a chance, please rebase this PR now that #4225 is merged. Thanks!

[tdonohue]

@AndreaBarbasso : Reminder that I need your help getting this PR ready for review again. Currently, I was waiting on this PR before I review #3613...but, if we cannot more it forward, then I may need to move forward #3613 first. Please get this rebased & conflicts resolved as soon as you are able.

[FrancescoMolinaro]

Hi @tdonohue, Andrea is currently away and will be back on Thusday, he asked me to take a look to this so I did the rebasing in his stead, the PR should be aligned now.

[pilasou]

HI @AndreaBarbasso, hi @tdonohue, thanks. I have tested the new cookie for Correlation ID using Docker, it worked as discribed. The new set up page for tracking contains a section to enable/disable Correlation ID.

If enable, the Correlation ID is generated

If disable, it is not:

[tdonohue]

@AndreaBarbasso : Thanks for the updates. Overall, this looks good & it works as described.

But, in looking at the way you've set the "isE2E" variable/flag (to disable orejime for e2e tests), I cannot help but feel there may be a much easier solution. I'm not against your approach, but I wanted to suggest something that may be much easier to maintain. See the note inline below.

[tdonohue]

I could be wrong, but I feel like we could remove all of this new environment.production-e2e.ts stuff and simply set this flag via an environment variable in package.json like this:

"e2e" : "cross-env NODE_ENV=production DSPACE_E2ETEST=true ng e2e"

Then, check for if (environment.e2eTest) { in the browser-orejime.service.ts. It should be "true" if that environment variable is set. This should work because of how environment variables can be used to override our settings.

Assuming that works, it'd have a few distinct advantages. We'd no longer need to duplicate all the production settings in package.json and angular.json. That means it'd also be much easier to maintain because we would not have to remember to keep the settings for production & production-e2e "in sync" in angular.json.

[AndreaBarbasso]

@tdonohue, I tried to do as you suggested, but it does not seem to work the way it should.

I applied that strategy to the usual build:prod in order to test it easily:
"build:prod": "cross-env NODE_ENV=production DSPACE_E2ETEST=true npm run build:ssr",

but then. in the config.json file generated in the dist folder I can't find the environment variable set up via the script in the package.json file. This is confirmed after starting it up in SSR - that configuration is nowhere to be found.

I also tried to set a default value of false and override it via the script, to no avail. Lastly, I changed its name to DSPACE_CYPRESSTEST (maybe the number in E2ETEST could spell trouble), and again, nothing happens.

I might be missing something, though...

[tdonohue]

@AndreaBarbasso : I was looking around for how we've done this before, and I think I've stumbled on something that will work. We already override the environment variables used in e2e tests in our build.yml here: https://github.com/DSpace/dspace-angular/blob/main/.github/workflows/build.yml#L17-L29

So, I'd propose we create a new boolean configuration called test, at the same level as the boolean production configuration here: https://github.com/DSpace/dspace-angular/blob/main/src/config/app-config.interface.ts#L43

We might need to default it to false in all our src/environments/environment.*.ts files (not sure if this is required).

Then, in browser-orejime.service.ts, you should be able to add a simple check for if (environment.test) {

Finally, to set this to true for our e2e tests, we just need to modify that build.yml list of environment variables to include:

env:
   # Enable "test" mode, which sets "environment.test" to true and disables some features (e.g. orejime).
   DSPACE_TEST: true

I believe this should work, because (as you can see in that build.yml) we already override some DSpace UI environment variables in order to ensure tests have other features disabled by default.

Finally, I should add, we could even make this setting specific to Orejime, and just call it enableCookieConsent under the info section like this:

info:
  enableCookieConsent: true

In that scenario, we'd default it to true though and set DSPACE_INFO_ENABLECOOKIECONSENT: false in the build.yml.

[tdonohue]

@AndreaBarbasso : Just a reminder to take a look at my feedback from Friday, when you find the chance. I think it would really simplify this PR, and let us get this finally merged. (But, please make sure to try it on your end to verify)

[artlowel]

Thanks @AndreaBarbasso!

While the correlation headers aren't sent when you disagree to the cookie, currently the cookie itself isn't removed. It looks like the reason is that the cookie name configured for orejime is not the one we're actually using. I expect that if we configure the correct cookie name, orejime will take care of removing the cookie.

However, before setting it again, it would be good to check if the user has agreed to it, otherwise I expect you'll see the cookie appear, and quickly being removed by orejime again every time you go to a new page.

In general it would also be a good idea to use the constant for the cookie name everywhere it's needed.

As for the e2e issue. I prefer @tdonohue's last suggestion to add a info.enableCookieConsentPopup config property that defaults to true. That’s not only useful for our tests, but people who aren’t beholden to the GDPR might want to use it too

[tdonohue]

@AndreaBarbasso : If you could quickly update this PR, it sounds like we have some general agreement on the best approach to fix the e2e tests. Please keep in mind we have a "code freeze" coming up at the end of this week, so I'd like to get this updated as soon as possible so that we can still include it in 9.0 (and also find time to include #3613, which is waiting on this PR)

[AndreaBarbasso]

@tdonohue, @artlowel, I implemented the suggestions you gave me!

Now the cookie is set only if orejime's consent has been given, and I'm trying to use an environment variable via the build.yml file. Let's see if the pipeline is successful!

EDIT: oops, circular dependencies are here, my bad.

[github-actions]

Hi @AndreaBarbasso,
Conflicts have been detected against the base branch.
Please resolve these conflicts as soon as you can. Thanks!

[artlowel]

Thanks @AndreaBarbasso. That works, and the e2e tests pass! 🎉

I found one more issue though. If you enable the popup, and open it for the first time with cleared cookies (or in an incognito window) you get an error in the console. I added an inline comment where it happens:

[artlowel]

Thanks @AndreaBarbasso!

[tdonohue]

👍 Thanks @AndreaBarbasso ! This looks great to me now too, and all my testing is successful. I've again verified that the X-CORRELATION-ID is not stored when the user turns it off. I've also verified it works when turned on. Finally, I verified that the new enableCookieConsentPopup can be turned off to disable the popup altogether (and remove the footer link)

One note: After today's Developer Meeting, I did realize we might still want to backport the ability to manage the X-CORRELATION-ID to 8.x and 7.x. That would involve a partial backport of this PR though, because it doesn't require the ability to turn off orejime (which didn't exist in 8.x and 7.x anyways). But, there's not a rush to backport this immediately, and I'd consider it optional at this time.