search

spring-projects / spring-security

Spring Security
http://spring.io/projects/spring-security
Apache License 2.0
8.45k stars 5.76k forks source link

Bugfix Null Safety and Code Consistency: Refactoring `getAuthorizedClients(WebSession session)` #14977

Open dukbong opened 2 weeks ago

dukbong commented 2 weeks ago

Original Code:

The getAuthorizedClients method retrieves a map of OAuth2 authorized clients from a WebSession. In the original code, if the session is null, the method assigns a default empty HashMap. This code does not explicitly assert that WebSession cannot be null, potentially leading to silent failures or unexpected behavior.

Closes #14975

Suggested Refactoring:

To ensure robustness and improve error handling, you can refactor the method by adding an assertion to prevent a null session. This approach uses Assert.notNull to raise an exception with a clear error message if the WebSession is null, helping developers quickly identify and fix the problem.

sjohnr commented 2 weeks ago

Hi @dukbong, thanks for the PR!

Just a heads up, I closed the original issue with this explanation. I appreciate what you're doing in this change by asserting the session cannot be null. I'm wondering though if this change is absolutely necessary?

This code does not explicitly assert that WebSession cannot be null, potentially leading to silent failures or unexpected behavior.

Can you explain the situation in which this would occur (other than the invalid configuration referred to in the original issue)?

dukbong commented 2 weeks ago

Hi @dukbong, thanks for the PR!

Just a heads up, I closed the original issue with this explanation. I appreciate what you're doing in this change by asserting the session cannot be null. I'm wondering though if this change is absolutely necessary?

This code does not explicitly assert that WebSession cannot be null, potentially leading to silent failures or unexpected behavior.

Can you explain the situation in which this would occur (other than the invalid configuration referred to in the original issue)?

Hi @sjohnr

Initially, I planned to submit a pull request to address the issue, but as I reviewed the code, I found the structure to be questionable, prompting a change in direction.

The getAuthorizedClient function is written with the assumption that the session might be null. However, if the session is null, this leads to an exception in the saveAuthorizedClient function. To prevent this, I believe it's better to ensure the session isn't null either in the getAuthorizedClient function or in the doOnSuccess block.

Based on these observations, I created a pull request to address the issue.

dukbong commented 2 weeks ago

Thanks for the PR @dukbong. In addition to the feedback inline below, can you please add a test that asserts an IllegalArgumentException is thrown when WebSession is null?

Thanks @sjohnr for agreeing with my feedback. I will make the changes and additions as per the comments below!

dukbong commented 1 week ago

Thanks @dukbong. I added one minor comment below. Can you also please rebase your changes on main and squash commits? I will schedule this to be merged in the next minor so you won't see it merged for a few weeks.

Hello. @sjohnr

As you suggested, I rebased onto the main branch and squashed the commits into one.

If there's anything I did wrong or need to correct, please let me know! I'll fix it right away!!