Skip to content

Add referrer to pageview event#2288

Merged
tdonohue merged 20 commits intoDSpace:mainfrom
atmire:add-referrer-to-pageview-event-7.6.0-next
Jun 20, 2023
Merged

Add referrer to pageview event#2288
tdonohue merged 20 commits intoDSpace:mainfrom
atmire:add-referrer-to-pageview-event-7.6.0-next

Conversation

@artlowel
Copy link
Copy Markdown
Member

@artlowel artlowel commented Jun 1, 2023

References

Description

This PR sends a referrer param along with statistics view events, the referrer that lead the user to reach the UI page can be tracked, rather than the referrer of the rest call, which will always be the UI page you're currently on

It also removes the rel="noopener noreferrer" param from all our listableObject links if target isn't _blank. Otherwise we still wouldn't have a referrer if a user got to a page using one of them. The attribute is left on for target="_blank" links for security reasons

Instructions for Reviewers

Visit the various pages that track views (home page, DSO pages) and verify in the browser's dev tools that the referrer param is sent, and that its value is correct. Either locally or when coming from a 3rd party site.

Keep in mind when using links from a 3rd party site, that the default referrer policy for all modern browsers is strict-origin-when-cross-origin, so you'd only expect a referrer header when coming from another https site, and even then you'd only get the origin, not the exact path. Sites can specify exceptions to this using <meta> tags though

Checklist

  • 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 yarn lint
  • My PR doesn't introduce circular dependencies (verified via yarn 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.
  • 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.

@artlowel artlowel added bug component: statistics claimed: Atmire Atmire team is working on this issue & will contribute back labels Jun 1, 2023
@artlowel artlowel added this to the 7.6 milestone Jun 1, 2023
@artlowel artlowel self-assigned this Jun 1, 2023
@artlowel artlowel changed the title Add referrer to pageview event 7.6.0 next Add referrer to pageview event Jun 1, 2023
@tdonohue tdonohue added the 1 APPROVAL pull request only requires a single approval to merge label Jun 1, 2023
@tdonohue tdonohue requested a review from atarix83 June 1, 2023 14:15
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jun 5, 2023

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

Copy link
Copy Markdown
Contributor

@atarix83 atarix83 left a comment

Choose a reason for hiding this comment

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

@artlowel

I can't test this PR since i got the blank page error mentioned on slack https://dspace-org.slack.com/archives/CA15G9UE4/p1686132638587179

could you try to update both angular and rest PR

@artlowel
Copy link
Copy Markdown
Member Author

artlowel commented Jun 8, 2023

@atarix83 Can you try again please? We've updated the backend PR, and they should be compatible again

Copy link
Copy Markdown
Contributor

@atarix83 atarix83 left a comment

Choose a reason for hiding this comment

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

@artlowel

I'm testing it, but i'm not sure the result i got it's correct.

if i navigate to an item page following an internal application link, for example from the search page, i have as referrer the value "http://localhost:4000/".
Maybe i understood wrong, but should it contains also the page route? so in my test "http://localhost:4000/search"?

@artlowel
Copy link
Copy Markdown
Member Author

artlowel commented Jun 9, 2023

I tested it again, and I can't reproduce the problem you're having. I suspect your browser or an extension may be enforcing a referrer policy that prevents anything but the origin to be shared, for privacy reasons

@tdonohue
Copy link
Copy Markdown
Member

tdonohue commented Jun 9, 2023

@artlowel : I think I've figured out the possible confusion. The testing instructions are slightly unclear, as I initially thought I had the same issue as @atarix83 .

Here's what I was trying to test... depending on what is meant by "referrer", there's two different behaviors. Let me explain...

  1. First, I'm running both the frontend and backend locally (both on localhost). Frontend is in prod mode
  2. Open up browser DevTools to watch for /statistics/viewevents requests to the REST API
  3. From the homepage, perform a search for "test".
  4. I see a POST /statistics/viewevents request corresponding that that search.
    • In the Referer HTTP Header for the request, I see Referer: http://localhost:4000/. (I suspect this is what @atarix83 was looking at, because I looked there first as well)
    • But, in the Payload (body) of the POST, I see this:
      {
       "targetId":"82b9dfd6-038e-4472-8749-0a1ce87aa869",
       "targetType":"item",
       "referrer":"http://localhost:4000/search?query=test"
      }
      

Assuming I understand this PR properly, we should be looking at this referrer param/property in the Payload, correct? It's just confusing, as I initially looked at the Referer HTTP Header as that's what I naturally assumed you were talking about.

In any case, I ran a few basic tests today and can confirm that the referrer param/property in the payload is correct, but the Referer HTTP Header is not correct. I'll test more on Monday, but I assume that's likely "expected behavior"?

(I'm also assuming there may be a reason we want to send the referrer param instead of using the Referer HTTP Header?) Re-reading the original ticket, I see there is a reason why you chose to use a referrer param instead of the Referer header: DSpace/DSpace#8817 And it sounds like the behavior I'm seeing is the expected behavior.

@artlowel
Copy link
Copy Markdown
Member Author

artlowel commented Jun 10, 2023

@tdonohue indeed: I mean the referrer parameter in the viewevents request, not the Referer header. The reason we can't use the Referer header, is because for a rest request made by the UI, it will always contain the current page, not the page the user navigated from. That's the issue this PR intends to solve

@artlowel artlowel requested a review from atarix83 June 10, 2023 08:49
Copy link
Copy Markdown
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

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

👍 Thanks @artlowel . Verified this is working as described. I'm seeing the referrer param now passed to POST /api/statistics/viewevents calls. It looks to be correct in all examples that I can find. For instance, here's what the body looks like after clicking a search result after running a search on "cameron":

{
"targetId":"d10c32e6-2b14-41f7-be3e-46a41bbe1cb4",
"targetType":"item",
"referrer":"http://localhost:4000/search?query=cameron"
}

I've also verified that this results in the Solr document having a correct referrer:

{
        "ip":"[redacted]",
        "referrer":"http://localhost:4000/search?query=cameron",
        ...
        "isBot":false,
        "id":"d10c32e6-2b14-41f7-be3e-46a41bbe1cb4",
        "type":2,
        "owningColl":["282164f5-d325-4740-8dd1-fa4d6d3e7200"],
        "owningComm":["0958c910-2037-42a9-81c7-dca80e3892b4"],
        "time":"2023-06-12T15:55:00.172Z",
        "statistics_type":"view",
        "uid":"09ad75db-a17d-4e2e-b9ae-892898cf2720"},

This will be ready to merge as soon as a small bug in the backend PR is fixed.

@atarix83
Copy link
Copy Markdown
Contributor

@artlowel @tdonohue

I've tested it again, and having the same result.

The POST request contains the correct referrer, but the solr record created contains always "http://localhost:4000/"

{
  "targetId":"664f4f47-7d6f-41c9-97a5-04082015c535",
  "targetType":"item",
  "referrer":"http://localhost:4000/search?query=&f.has_content_in_original_bundle=true,equals&spc.page=1"
}

solr

      {
        "ip":"0:0:0:0:0:0:0:1",
        "referrer":"http://localhost:4000/",
        "dns":"ip6-localhost",
        "userAgent":"Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/113.0.0.0 Safari/537.36",
        "isBot":false,
        "id":"664f4f47-7d6f-41c9-97a5-04082015c535",
        "type":2,
        "owningColl":["282164f5-d325-4740-8dd1-fa4d6d3e7200"],
        "owningComm":["0958c910-2037-42a9-81c7-dca80e3892b4"],
        "time":"2023-06-15T09:59:47.925Z",
        "statistics_type":"search_result",
        "rpp":0,
        "sortBy":"score",
        "sortOrder":"desc",
        "page":1,
        "uid":"35c8a76d-eff0-4cab-b5cf-a658f2f3795b"
}

Is probably a REST issue??

Copy link
Copy Markdown
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

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

👍 @artlowel : I retested this today (with the updated backend PR) and it's working for me.

I also realized that @atarix83 seems to be accidentally looking at the wrong Solr document in his tests. He was looking at the statistics_type:search_result. This PR instead is a fix for statistics_type:view .

Here's my testing process:

  1. Install both the frontend and backend PR. Running frontend in production mode
  2. Perform a search from the homepage for "testing".
  3. Now, click on an Item in the search results. This seems to generate two documents in Solr:
    • First, it will generate a statistics_type:search_result that looks like what @atarix83 was seeing. In this situation the "referrer" does appear to always be wrong.:
    {
        "ip":"[redacted]",
        "referrer":"http://localhost:4000/",
        ...
        "type":2,
        "owningColl":["c2aecee3-d59a-4e9c-9659-34283a719cda"],
        "owningComm":["c0e4de93-f506-4990-a840-d406f6f2ada7"],
        "time":"2023-06-16T15:21:31.668Z",
        "query":["testing"],
        "statistics_type":"search_result",
        "rpp":0,
        "sortBy":"score",
        "sortOrder":"desc",
        "page":1,
        "uid":"3673c2a4-7108-4c91-bbf2-d3382662ff7a"},
    
    • Second, this generates a statistics_type:view, and the referrer should reference the search that was performed (this is the bug fixed by this PR):
    {
        "ip":"[redacted]",
        "referrer":"http://localhost:4000/search?query=testing",
        ...
        "id":"16228f28-2f7e-44ec-9aa5-5898f89e8993",
        "type":2,
        "owningColl":["c2aecee3-d59a-4e9c-9659-34283a719cda"],
        "owningComm":["c0e4de93-f506-4990-a840-d406f6f2ada7"],
        "time":"2023-06-16T15:21:32.135Z",
        "statistics_type":"view",
        "uid":"8cd6bd9d-cbfb-4e99-b691-11ad872d1bea"},
    

So, as you can see above... the referrer is now correct for view entries. But, it still is incorrect for search_result entries. I'm assuming that's expected behavior though, since this PR only claims to fix the view statistics. (Maybe we need a separate ticket for the search_result entries though? It's unclear to me how search_result is used though, and whether that's necessary)

Copy link
Copy Markdown
Contributor

@atarix83 atarix83 left a comment

Choose a reason for hiding this comment

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

@artlowel @tdonohue

tested it again and now it works fine. LGTM

@tdonohue tdonohue merged commit 157eeda into DSpace:main Jun 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

1 APPROVAL pull request only requires a single approval to merge bug claimed: Atmire Atmire team is working on this issue & will contribute back component: statistics

Projects

No open projects
Status: ✅ Done

Development

Successfully merging this pull request may close these issues.

The referrer for a statistics view event is almost always wrong

3 participants