Save original request on oauth2Client filter

[rozagerardo]

When we used the oauth2Client directive and requested an endpoint that
required client authorization on the authorization server, the
SPRING_SECURITY_SAVED_REQUEST was not persisted, and therefore after
creating the authorized client we were redirected to the root page ("/").

Now we are storing the session attribute and getting redirected back to
the original URI as expected.

Note that the attribute is stored only when a
ClientAuthorizationRequiredException is thrown in the chain, we don't
want to store it as a response to the
/oauth2/authorization/{registrationId} endpoint, since we would end
up in an infinite loop

Fixes related Issue: #6341

[rwinch]

Thanks for the PR! Can you please add a new test that verifies that the ServerRequestCache is used (i.e. that the exchange is saved) appropriately?

[rozagerardo]

Hey Rob thanks for taking the time to review the PR and for the comment
You mean a test not just mocking and verifying that the ServerRequestCache.saveRequest method is called, but to actually allow it save the request in the session, and validate that it was correctly stored?
I now added a new test to to that (filterWhenHtmlRequestWithExceptionThenRedirectedWithSavedRequestSessionAttribute), feel free to have a look, and let me know if it's ok :)
@rwinch

[rwinch]

Let's change this to a ServerRequestCache and mock out the methods. This ensures the test isn't relying on WebSessionServerRequestCache behavior

[rozagerardo]

Sorry for the comings and goings here @rwinch , but I'd like to make sure we're on the same page.
Wouldn't that be what I first did on the filterWhenExceptionThenRedirected test?
I first chose to modify that test instead of creating a new one, since that will be the actual scenario that we want to test, i.e. that when we receive a ClientAuthorizationRequiredException, we redirect and save the original request in the process.
I can create a new test, but it will be pretty much the same test that the one I mentioned above, wouldn't it? Please let me know if I'm missing something here :). I'll clean everything once I understand your perspective correctly, sounds good?

[rwinch]

Yes it is quite similar. However, please use a Mock instead of a Spy. With the Spy we are relying on behavior from WebSessionServerRequestCache to see if things are working. For example, the assertion is checking to see if a session attribute has been set or not. This means that this test requires WebSessionServerRequestCache sets that attribute. All we do care is that the save method is invoked with the correct arguments.

[rozagerardo]

Of course, I agree that we don't want an integration test here.
Let me try to explain better, I now removed the integration test and added a new test. I believe that's the test that you had in mind, correct?
My question is, does this test bring any value to the table? It's exactly the same scenario that for the filterWhenExceptionThenRedirected isn't it?
That's why I considered that we don't need a new test, but just adding the validation to the pre existing test would be enough, but please correct me if I'm wrong, I'm probably missing something here :)

[rwinch]

Thanks for clarifying. I see what you mean. However, I think the problem is that we have now done two things:

1. Modified existing tests (we don't want to do that) by setting the mock in the setup method. This means that we are not testing if things work without setting a RequestCache (i.e. using the defaults)
2. Made filterWhenExceptionThenRedirected more complicated that necessary. I'd prefer to keep it specific to, is a redirect performed (not also is the cache used)

Instead of modifying any of the existing tests we should add two tests that are using the mock ServerRequestCache. One that verifies the ServerRequestCache is used when a ClientAuthorizationRequiredException occurs and one that verifies the ServerRequestCache is not used if there isn't a ClientAuthorizationRequiredException.

[rozagerardo]

Cool, thanks for the explaination Rob :)
So we prefer to have more simple tests focused on the validation of just a single feature/piece of functionality. That sounds good to me (y)
I now cleaned the Test class and added the two tests. Please let me know if you want me to do anything else

[rwinch]

Thanks again for the PR @rozagerardo! This is now merged into master