Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removed headers filter from loopbackApi #1331

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Junjiequan
Copy link
Contributor

@Junjiequan Junjiequan commented Nov 24, 2023

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
Copy link
Contributor Author

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

@bpedersen2 bpedersen2 force-pushed the SWAP-3647-scicat-remove-header-filters-and-use-query-filter branch from 96ddaa6 to 7b25aec Compare November 27, 2023 07:29
Copy link
Contributor

@bpedersen2 bpedersen2 left a comment

Choose a reason for hiding this comment

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

looks good to me

But I'd like feedback from facilities that run SciCat in production: @minottic , @dapvan , @dylanmcreynolds

Copy link
Contributor

@dylanmcreynolds dylanmcreynolds left a comment

Choose a reason for hiding this comment

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

Is this code part of the autogenerated code? We have been discussing regenerating based on the new backend, in which case this PR would be removed, yes?

@minottic
Copy link
Contributor

minottic commented Nov 28, 2023

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

@bpedersen2
Copy link
Contributor

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 bpedersen2 added the meeting Should be discussed in meeting label Nov 28, 2023
@minottic
Copy link
Contributor

minottic commented Nov 28, 2023

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

@Junjiequan
Copy link
Contributor Author

Junjiequan commented Nov 28, 2023

A PR SciCatProject/scicat-backend-next#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

@minottic
Copy link
Contributor

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

Copy link

sonarcloud bot commented Jun 11, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meeting Should be discussed in meeting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants