dspace-angular icon indicating copy to clipboard operation
dspace-angular copied to clipboard

Encode all `RequestParam` values

Open alexandrevryghem opened this issue 1 year ago • 1 comments

References

  • Fixes #2724
  • Fixes #2727

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

  • [x] 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.
  • [x] My PR passes ESLint validation using yarn lint
  • [x] My PR doesn't introduce circular dependencies (verified via yarn check-circ-deps)
  • [x] My PR includes TypeDoc comments for all new (or modified) public methods and classes. It also includes TypeDoc for large or complex private methods.
  • [x] My PR passes all specs/tests and includes new/updated specs or tests based on the Code Testing Guide.
  • [x] 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.
  • [x] If my PR includes new features or configurations, I've provided basic technical documentation in the PR itself.
  • [x] If my PR fixes an issue ticket, I've linked them together.

alexandrevryghem avatar Dec 28 '23 15:12 alexandrevryghem

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

alexandrevryghem avatar Mar 24 '24 15:03 alexandrevryghem

@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.

tdonohue avatar May 09 '24 20:05 tdonohue

@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

alexandrevryghem avatar May 10 '24 08:05 alexandrevryghem

Successfully created backport PR for dspace-7_x:

  • #3032

dspace-bot avatar May 10 '24 15:05 dspace-bot

@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

tdonohue avatar May 14 '24 19:05 tdonohue