Refactor OAuth2Controller to actions. - #5605
Conversation
| * | ||
| * @return ResponseInterface | ||
| */ | ||
| public function action( |
There was a problem hiding this comment.
It would be nice to move more of the logic in this method to OAuth2ServerService, but it's not easy because it's a multi-step operation with different outcomes depending e.g. on the user login status.
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
I ran tests on this locally and they all passed, but I've only just started looking at the code. See below for one small thing I noticed while waiting to get set up for the Summit. I'll provide a more thorough follow-up after the Summit is over -- just submitting this item now so it doesn't get lost/forgotten.
Moves a significant part of the logic to a new OAuth2ServerService.
f19d0f9 to
d650826
Compare
demiankatz
added
the
architecture
|
Note: the functionality in the actions and OAuth2ServerService is almost completely covered by OAuth2Test Mink tests. |
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
Thanks, @EreMaijala. All tests are passing for me, but see below for some minor questions, suggestions and observations.
| if ($request->getMethod() == 'OPTIONS') { | ||
| // Disable session writes | ||
| $this->disableSessionWrites(); | ||
| $response = $this->getResponse(); | ||
| $response->setStatusCode(204); | ||
| $this->addCorsHeaders($response); | ||
| return $response; | ||
| } |
There was a problem hiding this comment.
It's not immediately obvious to me where this logic ended up in the new version. Is this important, or am I just overlooking something?
There was a problem hiding this comment.
Oops! Thanks for catching that. I completely forgot to bring it over. Now done. In future we may want to consider handling OPTIONS requests in middleware, but that's a future thing.
There was a problem hiding this comment.
An OPTIONS request also added to the Mink test.
| protected function handleOAuth2Exception( | ||
| ResponseInterface $response, | ||
| string $function, | ||
| \Exception $e |
There was a problem hiding this comment.
convertOAuthServerExceptionToResponse assumes that $e will have a generateHttpResponse method, which I assume is not part of the base exception class. Do we need a more specific type here, or some kind of type checking later?
There was a problem hiding this comment.
The exception handling methods were kind of confusing already in the controller. I renamed the methods and fixed the parameter typing.
| $response, | ||
| $this->oauth2Service->getUserInfo($request) | ||
| ); | ||
| return $responseHelper->addCorsHeaders($response); |
There was a problem hiding this comment.
As far as I can tell, the original version did not add CORS headers. Was that an oversight that is corrected here, or am I just missing something?
There was a problem hiding this comment.
I believe it was an oversight. It'd only affect browser-based authorization (public client), so probably that's why nobody has noticed.
|
|
||
| $responseHelper = $this->getHelper(ResponseHelper::class); | ||
| $response = $responseHelper->getJsonResponse($response, $this->oauth2Service->getJwks()); | ||
| return $responseHelper->addCorsHeaders($response); |
There was a problem hiding this comment.
Another where the original code does not seem to include CORS. Probably not a problem, just an observation.
|
|
||
| $responseHelper = $this->getHelper(ResponseHelper::class); | ||
| $response = $responseHelper->getJsonResponse($response, $this->oauth2Service->getWellKnownConfiguration()); | ||
| return $responseHelper->addCorsHeaders($response); |
| { | ||
| $baseUrl = rtrim($this->baseUrl, '/'); | ||
| $configuration = [ | ||
| 'issuer' => 'https://' . $_SERVER['HTTP_HOST'], // Same as OpenIDConnectServer\IdTokenResponse |
There was a problem hiding this comment.
Is there a way to do this without using the $_SERVER superglobal? (I realize that the existing code already did this, but it seems like using an abstraction of the server variables would be more elegant).
There was a problem hiding this comment.
Yes, I changed it now. I believe the original one was done using $_SERVER because that's what OpenIDConnectServer\IdTokenResponse uses internally as well.
Co-authored-by: Demian Katz <demian.katz@villanova.edu>
Co-authored-by: Demian Katz <demian.katz@villanova.edu>
2 participants
Moves a significant part of the logic to a new OAuth2ServerService.