From dadd720a6ca7b5cbfc4b312a0d0f4b485e5ae387 Mon Sep 17 00:00:00 2001 From: Peter Havekes Date: Wed, 3 Jun 2026 10:07:15 +0200 Subject: [PATCH 1/5] Add email to logging for registrations and authentications --- .../Controller/DefaultController.php | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/src/Surfnet/AzureMfa/Infrastructure/Controller/DefaultController.php b/src/Surfnet/AzureMfa/Infrastructure/Controller/DefaultController.php index 2efdcb7..08864b1 100644 --- a/src/Surfnet/AzureMfa/Infrastructure/Controller/DefaultController.php +++ b/src/Surfnet/AzureMfa/Infrastructure/Controller/DefaultController.php @@ -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)); @@ -137,12 +137,18 @@ public function acs(Request $request): Response // 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"', + $user->getEmailAddress()->getEmailAddress() + )); $userId = $this->azureMfaService->finishRegistration($user->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"', + $user->getEmailAddress()->getEmailAddress() + )); $this->azureMfaService->finishAuthentication($user->getUserId()); $this->authenticationService->authenticate(); } From 302299416cd0e175113424c8a287b3712c6063da Mon Sep 17 00:00:00 2001 From: Peter Havekes Date: Wed, 3 Jun 2026 10:09:00 +0200 Subject: [PATCH 2/5] BSR --- .../Controller/DefaultController.php | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/Surfnet/AzureMfa/Infrastructure/Controller/DefaultController.php b/src/Surfnet/AzureMfa/Infrastructure/Controller/DefaultController.php index 08864b1..b8b6a13 100644 --- a/src/Surfnet/AzureMfa/Infrastructure/Controller/DefaultController.php +++ b/src/Surfnet/AzureMfa/Infrastructure/Controller/DefaultController.php @@ -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 @@ -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 @@ -125,7 +125,11 @@ 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'); From bb92fec8d23cc1e79f4c5e43395c85d64e2c1cbb Mon Sep 17 00:00:00 2001 From: Peter Havekes Date: Thu, 4 Jun 2026 08:43:44 +0200 Subject: [PATCH 3/5] Use userId instead of email in logs --- .../Controller/DefaultController.php | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/Surfnet/AzureMfa/Infrastructure/Controller/DefaultController.php b/src/Surfnet/AzureMfa/Infrastructure/Controller/DefaultController.php index b8b6a13..f940591 100644 --- a/src/Surfnet/AzureMfa/Infrastructure/Controller/DefaultController.php +++ b/src/Surfnet/AzureMfa/Infrastructure/Controller/DefaultController.php @@ -134,39 +134,41 @@ public function acs(Request $request): Response { $this->logger->info('Receiving response from the Azure MFA remote IdP'); + $userId = 'unknown'; + 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(sprintf( 'Finishing the registration for user "%s"', - $user->getEmailAddress()->getEmailAddress() - )); + $userId )); $userId = $this->azureMfaService->finishRegistration($user->getUserId()); $this->registrationService->register($userId->getUserId()); } elseif ($user->getStatus()->isRegistered()) { // Handle authentication, this user is already registered $this->logger->info(sprintf( 'Process the authentication for user "%s"', - $user->getEmailAddress()->getEmailAddress() - )); + $userId )); $this->azureMfaService->finishAuthentication($user->getUserId()); $this->authenticationService->authenticate(); } } catch (Exception $e) { $this->logger->error( 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)); return $this->registrationService->replyToServiceProvider(); } } From a7b4ac6f0492175401189d05a44e1bb786fae696 Mon Sep 17 00:00:00 2001 From: Kay Joosten Date: Thu, 4 Jun 2026 14:04:08 +0200 Subject: [PATCH 4/5] fix: prevent fatal TypeError when logging userId after registration 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. --- .../Controller/DefaultController.php | 22 ++++++++++--------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/src/Surfnet/AzureMfa/Infrastructure/Controller/DefaultController.php b/src/Surfnet/AzureMfa/Infrastructure/Controller/DefaultController.php index f940591..180b06a 100644 --- a/src/Surfnet/AzureMfa/Infrastructure/Controller/DefaultController.php +++ b/src/Surfnet/AzureMfa/Infrastructure/Controller/DefaultController.php @@ -129,12 +129,13 @@ public function authentication(): RedirectResponse|Response * 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')] + */ + #[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'; + $userId = null; try { $this->logger->info('Load the associated Stepup user from this response'); @@ -143,32 +144,33 @@ public function acs(Request $request): Response // Check registration status if ($user->getStatus()->isPending()) { - // Handle registration, this user is already registered + // Handle registration — user has completed Azure MFA, finalise the registration $this->logger->info(sprintf( 'Finishing the registration for user "%s"', - $userId )); - $userId = $this->azureMfaService->finishRegistration($user->getUserId()); - $this->registrationService->register($userId->getUserId()); + $userId + )); + $registeredUserId = $this->azureMfaService->finishRegistration($user->getUserId()); + $this->registrationService->register($registeredUserId->getUserId()); } elseif ($user->getStatus()->isRegistered()) { // Handle authentication, this user is already registered $this->logger->info(sprintf( 'Process the authentication for user "%s"', - $userId )); + $userId + )); $this->azureMfaService->finishAuthentication($user->getUserId()); $this->authenticationService->authenticate(); } } catch (Exception $e) { $this->logger->error( sprintf( - 'The authentication or registration for user %s failed. Rejecting the Azure MFA response. Error message: "%s"', - $userId, + 'The authentication or registration failed. Rejecting the Azure MFA response. Error message: "%s"', $e->getMessage() ) ); $this->registrationService->reject($request->get('message', '')); } - $this->logger->info(sprintf('Sending a SAML response to the SP for userId "%s"', $userId)); + $this->logger->info(sprintf('Sending a SAML response to the SP for userId "%s"', $userId ?? 'unknown')); return $this->registrationService->replyToServiceProvider(); } } From 062ef4d942a4445ee7b772e13162c9c9251c2c7d Mon Sep 17 00:00:00 2001 From: Kay Joosten Date: Thu, 4 Jun 2026 14:09:16 +0200 Subject: [PATCH 5/5] =?UTF-8?q?fix:=20drop=20userId=20from=20final=20ACS?= =?UTF-8?q?=20log=20=E2=80=94=20only=20log=20it=20where=20it=20is=20known?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../Infrastructure/Controller/DefaultController.php | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/Surfnet/AzureMfa/Infrastructure/Controller/DefaultController.php b/src/Surfnet/AzureMfa/Infrastructure/Controller/DefaultController.php index 180b06a..6257377 100644 --- a/src/Surfnet/AzureMfa/Infrastructure/Controller/DefaultController.php +++ b/src/Surfnet/AzureMfa/Infrastructure/Controller/DefaultController.php @@ -135,16 +135,12 @@ public function acs(Request $request): Response { $this->logger->info('Receiving response from the Azure MFA remote IdP'); - $userId = null; - 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 — user has completed Azure MFA, finalise the registration $this->logger->info(sprintf( 'Finishing the registration for user "%s"', $userId @@ -152,7 +148,6 @@ public function acs(Request $request): Response $registeredUserId = $this->azureMfaService->finishRegistration($user->getUserId()); $this->registrationService->register($registeredUserId->getUserId()); } elseif ($user->getStatus()->isRegistered()) { - // Handle authentication, this user is already registered $this->logger->info(sprintf( 'Process the authentication for user "%s"', $userId @@ -170,7 +165,7 @@ public function acs(Request $request): Response $this->registrationService->reject($request->get('message', '')); } - $this->logger->info(sprintf('Sending a SAML response to the SP for userId "%s"', $userId ?? 'unknown')); + $this->logger->info('Sending a SAML response to the SP'); return $this->registrationService->replyToServiceProvider(); } }