Add email address to registration and authentication logs#245
Conversation
| { | ||
| $this->logger->info('Receiving response from the Azure MFA remote IdP'); | ||
|
|
||
| $userId = 'unknown'; |
There was a problem hiding this comment.
Remove this line entirely. Initializing $userId to a magic string 'unknown' is a code smell it hides whether we actually resolved a real user ID or not. If handleResponse() throws before line 143, we simply don't have a user ID, and the error log should reflect that honestly (see comment on the catch block below) rather than masking it with a sentinel value.
| $this->logger->info(sprintf( | ||
| 'Finishing the registration for user "%s"', | ||
| $userId )); | ||
| $userId = $this->azureMfaService->finishRegistration($user->getUserId()); |
There was a problem hiding this comment.
$userId is a string (assigned on line 143 via getUserId()->getUserId()), but here it is overwritten with the UserId object returned by finishRegistration(). UserId has no __toString(), so the sprintf calls at lines 164 and 172 will throw a fatal TypeError on every successful registration the SAML response is never sent to the SP.
Fix suggestion: use a separate variable for the return value:
$registeredUserId = $this->azureMfaService->finishRegistration($user->getUserId());
$this->registrationService->register($registeredUserId->getUserId());| $this->authenticationService->authenticate(); | ||
| } | ||
| } catch (Exception $e) { | ||
| $this->logger->error( |
There was a problem hiding this comment.
The catch block still references $userId (line 164), but after the fix on line 151 $userId
can still be undefined here if handleResponse() throws before line 143 ever runs.
Since we genuinely don't know the user at that point, drop it from the error message entirely:
'The authentication or registration failed. Rejecting the Azure MFA response. Error message: "%s"', $e->getMessage()| } | ||
|
|
||
| $this->logger->info('Sending a SAML response to the SP'); | ||
| $this->logger->info(sprintf('Sending a SAML response to the SP for userId "%s"', $userId)); |
There was a problem hiding this comment.
After the fix on line 151 (use $registeredUserId instead of reusing $userId), this log line will be correct $userId remains a string for the full method lifetime. No change needed here once the variable reuse on line 151 is fixed.
finishRegistration() returns a UserId object; reusing $userId caused a TypeError in sprintf on every successful registration. Use a separate $registeredUserId variable, init $userId as null with ?? fallback, and drop the unresolvable user from the catch-block error message.
2 participants
No description provided.