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

[Refactor] Remove the notion of SecurityRequestChannel #3476

Open
3 tasks
peternied opened this issue Oct 6, 2023 · 3 comments
Open
3 tasks

[Refactor] Remove the notion of SecurityRequestChannel #3476

peternied opened this issue Oct 6, 2023 · 3 comments
Assignees
Labels
CCI College Contributor Initiative good first issue These are recommended starting points for newcomers looking to make their first contributions. refactoring code/test refactoring triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable.

Comments

@peternied
Copy link
Member

peternied commented Oct 6, 2023

Coming from [1] where this new interface was added.

This class is meant to send back a response through the channel that a current request has. As originally built up responses could be triggered from deep in code paths around the backend registry or authenticator implementations. As more and more refactoring was done these changes transitioned to passing back a response which would be used it if was found.

This model of checking if a response has already been created and should be sent to the client seems to be an obvious replacement for the current too permissive access to the request channel that many parts of the security plugin has.

Exit Criteria

  • Find all places where a RestChannel is being passed as parameter, invert the flow of control so that method passes back out a Optional<SecurityResponse>
  • Delete the SecurityRequestChannel class and do the same process as above, providing Optional<SecurityResponse> as a return value for functionality that was directly using the channel. All instance of SecurityRequestChannel should be replaced with SecurityRequest.
  • Make sure that usage of sendResponse() are replaced in such a way that where and when the response is triggered is from the source of the request, such as the handler wrapper in the SecurityRestFilter or the AuthenticatingVerifier.

@github-actions github-actions bot added the untriaged Require the attention of the repository maintainers and may need to be prioritized label Oct 6, 2023
@peternied peternied added the refactoring code/test refactoring label Oct 6, 2023
@stephen-crawford
Copy link
Contributor

[Triage] Hi @peternied, thank you for filing this issue. This issue has a clear goal and closure criteria.

@stephen-crawford stephen-crawford added triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable. good first issue These are recommended starting points for newcomers looking to make their first contributions. CCI College Contributor Initiative and removed untriaged Require the attention of the repository maintainers and may need to be prioritized labels Oct 9, 2023
@d-wwwang
Copy link

I can take a jab at this!

@peternied
Copy link
Member Author

@d-wwwang Sure - I'll assign this to you, looking forward to seeing a PR for this, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CCI College Contributor Initiative good first issue These are recommended starting points for newcomers looking to make their first contributions. refactoring code/test refactoring triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable.
Projects
None yet
Development

No branches or pull requests

3 participants