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

SecurityResponse only accept single header values #3608

Closed
5 tasks
peternied opened this issue Oct 26, 2023 · 3 comments · Fixed by #3717
Closed
5 tasks

SecurityResponse only accept single header values #3608

peternied opened this issue Oct 26, 2023 · 3 comments · Fixed by #3717
Labels
bug Something isn't working triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable.

Comments

@peternied
Copy link
Member

As part of the abstraction [1] of SecurityRequest/SecurityResponse, changes were made for how headers were processed. OpenSearch's RestRequest object treats headers as a Map<String, List<String>> whereas in the refactor this was simplified to Map<String, String> This has caused some plugins that extend behavior of the security plugin to be broken.

SecurityResponse classes should switch to a Map<String, List<String>> representation so workarounds aren't needed in downstream consumers.

Acceptance Criteria

  • Read up on duplicate header entries and how they are handled
    • Confirm if a refactor is needed or if this can be done via another method (comma delimited?) StackOverflow
  • Update SecurityResponse from Map<String, String> headers; to Map<String, List<String>> headers;
  • Fix all impacted areas of the codebase
  • Add tests that use and verify duplicate header entries
@peternied peternied changed the title SecurityRequest & SecurityResponse only accept single header values SecurityResponse only accept single header values Oct 26, 2023
@peternied peternied added the bug Something isn't working label Oct 26, 2023
@github-actions github-actions bot added the untriaged Require the attention of the repository maintainers and may need to be prioritized label Oct 26, 2023
@kumjiten
Copy link

LGTM,
based on stackOverflow mentioned sending multiple value as comma separated in single value means whoever extends this has to write this logic of preparing the single value separated by comma for multi value header which I feel overhead.

@shikharj05
Copy link
Contributor

Thanks for creating this issue. This sounds like an appropriate change; as it would also be in-line with RestResponse class in OpenSearch core- https://github.com/opensearch-project/OpenSearch/blob/main/server/src/main/java/org/opensearch/rest/RestResponse.java#L54

@stephen-crawford
Copy link
Contributor

[Triage] Thank you for filing this issue @peternied. This looks good to go.

@stephen-crawford stephen-crawford added triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable. and removed untriaged Require the attention of the repository maintainers and may need to be prioritized labels Oct 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants