Encode all RequestParam values

[alexandrevryghem]

References

• Fixes Vocabulary displayed-values aren't retrieved using the encoded stored-value in submission form #2724

Description

This PR encodes all the RequestParams values. I added an optional parameter to still make it possible to provide already encoded values if needed. This has also an added advantage that it will throw errors for all values that are not created by using the new RequestParam constructor, since the third parameter will be missing.

Instructions for Reviewers

List of changes in this PR:

• Encode the value received in RequestParam by default
• Removed the encodeURIComponent() of values that were given as parameters to RequestParam to prevent values from being encoded twice

See #2724 & #2727 for how to test this PR (#2727 is only reproducible in production mode)

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.

[tdonohue]

@alexandrevryghem : Tried to test this today (just cause it looked small & simple). Unfortunately, it's not working for me. I was able to reproduce the bug.

But, with this PR installed, all of the <value-pairs> fields no longer show any values. Here's a screenshot showing the "Type" field. All fields that use <value-pairs> act like this. Maybe this requires a backend fix as well?

There's no rush in fixing this issue (as the currently priority for 8.0 is on cleaning up feature PRs). I'm simply reporting what I found, so that you can revisit this after you've finished up with any feature PRs you are working with.

[alexandrevryghem]

@tdonohue: Like discussed in #2727 I refactored this PR to encode all RequestParams, since this fixes both issues at the same time

[tdonohue]

@alexandrevryghem : I retested this today, and it doesn't seem to have solved both of the issues that it claims to fix:

• First, I can verify that this fixes Vocabulary displayed-values aren't retrieved using the encoded stored-value in submission form #2724 That is no longer reproducible with this PR
• However, it does NOT seem to fix Bitstream download URLs based on the handle of the item and the bitstream filename don't work with accented characters #2727. I no longer get the 500 error that used to occur, but now you are given a 403 error (even after logging in as an Admin) when downloading bitstreams with accented characters.

Here's how to reproduce the issues with #2727. I'm running the UI in production mode via yarn build:prod; yarn serve:ssr

1. First, create an item with two files with no access restrictions. Make sure one has zero accented characters and one has at least one. Here's an example:
   • One bitstream named test.doc
   • One bitstream named test_ć.pdf
2. Determine the Item's handle (e.g. 123456789/10). Make sure that a basic redirect works properly like this:
   • Visit http://localhost:4000/handle/123456789/10 and make sure you are redirected to the Item
   • Or run curl --head http://localhost:4000/handle/123456789/10 and make sure it returns a 301 redirect.
3. Now, try the "handle" style redirect for the first bitstream (test.doc). It should work.
   • Visit http://localhost:4000/bitstream/handle/123456789/10/test.doc and verify the file is downloaded.
   • Or, run curl --head http://localhost:4000/bitstream/handle/123456789/10/test.doc and make sure it returns a 302 redirect. (The 302 redirect is slightly wrong, but that's a different bug ticket Old 6.x Bitstream URL paths are redirecting with a 302 (temporary) instead of 301 (permanent) #2963 )
4. Finally, try the same redirect for the accented bitstream (test_ć.pdf) It will NOT work
   • Visit http://localhost:4000/bitstream/handle/123456789/10/test_ć.pdf You will be prompted to login. If you login as an Admin, you'll still get a 403 exception
   • Or, run curl --head http://localhost:4000/bitstream/handle/123456789/10/test_ć.pdf and you'll see it returns a 200 OK instead of a 302 redirect.

If you'd rather split this into two PRs, that's another option. We could merge this as a proper fix for #2724, if we want to treat that other bug separately.

UPDATE: I noticed that when the curl --head http://localhost:4000/bitstream/handle/123456789/10/test_ć.pdf is run, I see this in the SSR logs:

HEAD /bitstream/handle/123456789/10/test_pdf_Ä.pdf 200 869.406 ms - 398724

Notice how the accented character now looks incorrect? It might be that this PR is partially working, but there's now a new bug in how the redirect occurs. It might be that the redirect itself is mangling the accented character.

[tdonohue]

@alexandrevryghem : Any chance of still moving this forward for 8.0? It appears the feedback in my review wasn't addressed yet. If this is turning out to be too difficult, we can reschedule or separate out the fix for #2727. Just wanted to remind you that this is waiting on attention.

[alexandrevryghem]

@tdonohue: So if I remember correctly 😅 this PR did work before the latest Angular upgrade, so this fix should fix both issues on 7.6.2. So if you want to already merge this PR in order to fix #2724 on main this can already be done, and then I could try and take a closer look later into which changes caused this fix to break on main. I'm also going to try and take a closer look at this next week

[tdonohue]

👍 Thanks @alexandrevryghem ! Re-tested today and this fixes #2724. However, since it does NOT fix #2727, I've removed the link to that issue. We'll need to fix that issue separately. Code looks good to me & I like that this forces all RequestParams to be encoded by default.

[dspace-bot]

Successfully created backport PR for dspace-7_x:

• [Port dspace-7_x] Encode all RequestParam values #3032

[tdonohue]

@alexandrevryghem : I've realized this PR appears to have had a side effect of increasing these statements in the UI logs:

The response for 'http://localhost:8080/server/api/authz/authorizations/search/object?uri=http%3A%2F%2Flocalhost%3A8080%2Fserver%2Fapi%2Fcore%2Fsites%2F6d65c6a2-3fe7-44dd-bacb-79271257c35d&feature=canSubmit'
has the self link 'http://localhost:8080/server/api/authz/authorizations/search/object?uri=http://localhost:8080/server/api/core/sites/6d65c6a2-3fe7-44dd-bacb-79271257c35d&feature=canSubmit'.
These don't match. This could mean there's an issue with the REST endpoint

The reason is that the UI is comparing one URL that has the RequestParam values all encoded, to one which does NOT have those params encoded. If you look closely, in the statement above, the URLs are identical except for the encoded params.

Any thoughts on how we might solve this? In the meantime, I'll pull this into a new ticket