frontend icon indicating copy to clipboard operation
frontend copied to clipboard

Removed headers filter from loopbackApi

Open Junjiequan opened this issue 2 years ago • 7 comments

Description

For better compatibility, visibility, and ease of use headers filter are removed and will no longer be supported by scicat backend

Motivation

Couple of reasons to remove header filters as following:

  • Limited Visibility and Debugging: Headers are less visible than query parameters or request bodies, complicating debugging and logging.
  • Convention and Consistency: In RESTful APIs, headers are conventionally used for control-related metadata like content type or authentication, not for business logic like filter parameters. Using them for filters goes against standard practices.
  • Security Concerns: Using headers for business logic can expose APIs to overlooked security vulnerabilities.

Tests included/Docs Updated?

  • [ ] Included for each change/fix?
  • [ ] Passing? (Merge will not be approved unless this is checked)
  • [ ] Docs updated?
  • [ ] New packages used/requires npm install?
  • [ ] Toggle added for new features?
  • [ ] Requires update of SciCat backend API?

Junjiequan avatar Nov 24 '23 12:11 Junjiequan

For E2E test to pass, #1328 needs to be merged

Junjiequan avatar Nov 24 '23 12:11 Junjiequan

I am pretty sure our ingestors use for historical reasons the headers in many places rather than the query params. Could the BE change wait until all facilities have rollout the new be, or at least a couple of weeks after the JOBs migration will be over? In the end all the concerns for the moment could be overcome simply by not using the headers. Happy, otherwise, to change the FE

minottic avatar Nov 28 '23 08:11 minottic

I am pretty sure our ingestors use for historical reasons the headers in many places rather than the query params. Could the BE change wait until all facilities have rollout the new be, or at least a couple of weeks after the JOBs migration will be over? In the end all the concerns for the moment could be overcome simply by not using the headers. Happy, otherwise, to change the FE

Hmm, as this does not remove the header support in the backend, this change should not affect ingesters running standalone. Only if a proxy does header injection (e.g. for sec. reasons) it could affect the frontend.

I think this is a good point to discuss for the meeting.

bpedersen2 avatar Nov 28 '23 08:11 bpedersen2

yes, indeed. I have nothing against changing the FE, but the PR description mentions a change in the BE

minottic avatar Nov 28 '23 08:11 minottic

A PR https://github.com/SciCatProject/scicat-backend-next/pull/911 in the new BE removed header filter usage(in datasets, policies, and user-identities controllers). If some facilities are still using header filters to send requests in datasets, policies and user identities endpoints, then I will make the change postponed

Junjiequan avatar Nov 28 '23 09:11 Junjiequan

ah ok, thanks, I must have missed that. I support Bjorn's point, let's discuss it in the meeting if time allows

minottic avatar Nov 28 '23 09:11 minottic

I will reopen this PR when it is needed.

Junjiequan avatar Mar 24 '25 12:03 Junjiequan