Skip to content

Add email address to registration and authentication logs #245

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.

Sign up for GitHub

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

Open
phavekes wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Add email address to registration and authentication logs #245

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
dadd720
Add email to logging for registrations and authentications
phavekes Jun 3, 2026
3022994
BSR
phavekes Jun 3, 2026
bb92fec
Use userId instead of email in logs
phavekes Jun 4, 2026
a7b4ac6
fix: prevent fatal TypeError when logging userId after registration
kayjoosten Jun 4, 2026
062ef4d
fix: drop userId from final ACS log — only log it where it is known
kayjoosten Jun 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 25 additions & 13 deletions src/Surfnet/AzureMfa/Infrastructure/Controller/DefaultController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,9 +54,9 @@ public function __construct(


/**
* Replace this example code with whatever you need.
* Handle Azure MFA registration by using available GSSP attributes or asking the user for an email address.
*
* See @see RegistrationService for a more clean example.
* Rejects failed registration callbacks and redirects valid registrations to the Azure MFA IdP.
*/
#[Route(path: '/registration', name: 'azure_mfa_registration')]
public function registration(Request $request): RedirectResponse|Response
Expand Down Expand Up @@ -88,10 +88,10 @@ public function registration(Request $request): RedirectResponse|Response
$form->handleRequest($request);

if ($form->isSubmitted() && $form->isValid()) {
$this->logger->info(
'Matched the user to an institution, continue registration by sending an ' .
'authentication request to the Azure MFA remote IdP'
);
$this->logger->info(sprintf(
'Matched the user "%s" to an institution, continue registration by sending an authentication request to the Azure MFA remote IdP',
$emailAddress->getEmailAddress()
));
$user = $this->azureMfaService->startRegistration(new EmailAddress($emailAddress->getEmailAddress()));

return new RedirectResponse($this->azureMfaService->createAuthnRequest($user));
Expand All @@ -105,9 +105,9 @@ public function registration(Request $request): RedirectResponse|Response
}

/**
* Replace this example code with whatever you need.
* Handle Azure MFA authentication by starting an authentication request for the current GSSP user.
*
* See @see AuthenticationService for a more clean example.
* Rejects requests when authentication is not required and redirects valid requests to the Azure MFA IdP.
*/
#[Route(path: '/authentication', name: 'azure_mfa_authentication')]
public function authentication(): RedirectResponse|Response
Expand All @@ -125,38 +125,50 @@ public function authentication(): RedirectResponse|Response
);
}

#[Route(path: '/saml/acs', name: 'azure_mfa_acs')]
/**
* Handle the Azure MFA SAML ACS response from the remote IdP.
*
* Finishes pending registrations or successful authentications and replies to the service provider.
*/#[Route(path: '/saml/acs', name: 'azure_mfa_acs')]
public function acs(Request $request): Response
{
$this->logger->info('Receiving response from the Azure MFA remote IdP');

$userId = 'unknown';

@kayjoosten kayjoosten Jun 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


try {
$this->logger->info('Load the associated Stepup user from this response');
$user = $this->azureMfaService->handleResponse($request);
$userId = $user->getUserId()->getUserId();

// Check registration status
if ($user->getStatus()->isPending()) {
// Handle registration, this user is already registered
$this->logger->info('Finishing the registration');
$this->logger->info(sprintf(
'Finishing the registration for user "%s"',
$userId ));
$userId = $this->azureMfaService->finishRegistration($user->getUserId());

@kayjoosten kayjoosten Jun 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$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->registrationService->register($userId->getUserId());
} elseif ($user->getStatus()->isRegistered()) {
// Handle authentication, this user is already registered
$this->logger->info('Process the authentication');
$this->logger->info(sprintf(
'Process the authentication for user "%s"',
$userId ));
$this->azureMfaService->finishAuthentication($user->getUserId());
$this->authenticationService->authenticate();
}
} catch (Exception $e) {
$this->logger->error(

@kayjoosten kayjoosten Jun 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()

sprintf(
'The authentication or registration failed. Rejecting the Azure MFA response. Error message: "%s"',
'The authentication or registration for user %s failed. Rejecting the Azure MFA response. Error message: "%s"',
$userId,
$e->getMessage()
)
);
$this->registrationService->reject($request->get('message', ''));
}

$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));

@kayjoosten kayjoosten Jun 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return $this->registrationService->replyToServiceProvider();
}
}
Loading