From 46c77d13712eb7c117b1f5290c02047e3589c0ee Mon Sep 17 00:00:00 2001 From: MishNajam <61416092+MishNajam@users.noreply.github.com> Date: Fri, 3 Nov 2023 12:27:19 +0000 Subject: [PATCH] UML-3121 make redirect uri dynamic (#2390) * UML-3121 make redirect uri dynamic * Clean up authenticateOneLoginHandler * Fix redirect fail on welsh bug * revert url helper logic * Refactor work, rename redirect url and store ui_locale in authsession * Add behat test * Revert workflow changes * Refactor behat tests --- .github/workflows/_build-and-push.yml | 25 ++++++----------- docker-compose.dependencies.yml | 2 +- .../OneLoginAuthenticationRequestHandler.php | 11 ++++++-- .../OneLoginAuthenticationRequestService.php | 4 +-- ...eLoginAuthenticationRequestServiceTest.php | 8 ++---- service-front/app/config/routes.php | 1 + .../features/context/UI/AccountContext.php | 28 ++++++++++++++----- service-front/app/features/one-login.feature | 9 +++++- .../Handler/AuthenticateOneLoginHandler.php | 16 +++++++++-- .../src/Service/OneLogin/OneLoginService.php | 5 ++-- .../Service/OneLogin/OneLoginServiceTest.php | 8 ++++-- 11 files changed, 75 insertions(+), 42 deletions(-) diff --git a/.github/workflows/_build-and-push.yml b/.github/workflows/_build-and-push.yml index 464cc91f08..e06a8343e6 100644 --- a/.github/workflows/_build-and-push.yml +++ b/.github/workflows/_build-and-push.yml @@ -146,19 +146,9 @@ jobs: matrix.run_unit_tests == 'true' && (inputs.specific_path == 'all' || inputs.specific_path == matrix.svc_prefix) - - name: ecr login - id: login_ecr - uses: aws-actions/amazon-ecr-login@f8672cc4c5410eabbb9df3d9bb4c7ad01fc4cc3e # pin@v1.5.1 - with: - registries: 311462405659 - if: | - inputs.push_to_ecr == 'true' && - (inputs.specific_path == 'all' || inputs.specific_path == matrix.svc_prefix) - - name: docker integration and acceptance tests env: SVC_PREFIX: ${{ matrix.svc_prefix }} - ECR_REGISTRY: ${{ steps.login_ecr.outputs.registry }} run: | if [[ "${SVC_PREFIX}" = "api" ]]; then docker run -d --rm --name lpa-codes-pact-mock pactfoundation/pact-cli:latest mock-service -p "80" --host "0.0.0.0" \ @@ -167,15 +157,10 @@ jobs: --pact-dir /tmp/pacts --consumer use_a_lasting_power_of_attorney --provider api-gateway docker run -d --rm --name iap-images-mock pactfoundation/pact-cli:latest mock-service -p "80" --host "0.0.0.0" \ --pact-dir /tmp/pacts --consumer use_a_lasting_power_of_attorney --provider iap-api-gateway - docker run -d --rm --name mock-one-login $ECR_REGISTRY/use_an_lpa/mock_onelogin_app export DOCKER_REMOTE_LPA_PACT_IP="$(docker inspect --format='{{.NetworkSettings.IPAddress}}' lpa-codes-pact-mock)" export DOCKER_REMOTE_API_PACT_IP="$(docker inspect --format='{{.NetworkSettings.IPAddress}}' api-gateway-pact-mock)" export DOCKER_REMOTE_IAP_PACT_IP="$(docker inspect --format='{{.NetworkSettings.IPAddress}}' iap-images-mock)" - export DOCKER_REMOTE_MOCK_ONELOGIN_IP="$(docker inspect --format='{{.NetworkSettings.IPAddress}}' mock-one-login)" - - curl -XGET $DOCKER_REMOTE_MOCK_ONELOGIN_IP:8080/token - docker logs mock-one-login docker run -d --name behattests \ --add-host lpa-codes-pact-mock:$DOCKER_REMOTE_LPA_PACT_IP \ @@ -189,7 +174,6 @@ jobs: docker stop lpa-codes-pact-mock docker stop api-gateway-pact-mock docker stop iap-images-mock - docker stop mock-one-login elif [[ "${SVC_PREFIX}" = "front" ]]; then docker run -d --name behattests front-app:latest @@ -272,6 +256,15 @@ jobs: with: sarif_file: 'trivy-results.sarif' + - name: ecr login + id: login_ecr + uses: aws-actions/amazon-ecr-login@f8672cc4c5410eabbb9df3d9bb4c7ad01fc4cc3e # pin@v1.5.1 + with: + registries: 311462405659 + if: | + inputs.push_to_ecr == 'true' && + (inputs.specific_path == 'all' || inputs.specific_path == matrix.svc_prefix) + - name: tag and push container env: ECR_REGISTRY: ${{ steps.login_ecr.outputs.registry }} diff --git a/docker-compose.dependencies.yml b/docker-compose.dependencies.yml index f6513d7a4a..08537569e2 100644 --- a/docker-compose.dependencies.yml +++ b/docker-compose.dependencies.yml @@ -109,4 +109,4 @@ services: environment: PUBLIC_URL: http://localhost:4013 INTERNAL_URL: http://mock-one-login:8080 - REDIRECT_URL: http://localhost:9002/auth/redirect + REDIRECT_URL: http://localhost:9002/home/login diff --git a/service-api/app/src/App/src/Handler/OneLoginAuthenticationRequestHandler.php b/service-api/app/src/App/src/Handler/OneLoginAuthenticationRequestHandler.php index ca43f38e67..dfd1e5deff 100644 --- a/service-api/app/src/App/src/Handler/OneLoginAuthenticationRequestHandler.php +++ b/service-api/app/src/App/src/Handler/OneLoginAuthenticationRequestHandler.php @@ -31,15 +31,22 @@ public function handle(ServerRequestInterface $request): ResponseInterface { $requestData = $request->getQueryParams(); + if (empty($requestData['redirect_url'])) { + throw new BadRequestException('Redirect URL must be provided'); + } + if (empty($requestData['ui_locale'])) { throw new BadRequestException('Ui locale must be provided'); } - $ui_locale = strtolower($requestData['ui_locale']); + $redirect_url = $requestData['redirect_url']; + $ui_locale = strtolower($requestData['ui_locale']); if ($ui_locale !== 'en' and $ui_locale !== 'cy') { throw new BadRequestException('ui_locale is not set to en or cy'); } - return new JsonResponse($this->authenticationRequestService->createAuthenticationRequest($ui_locale)); + $authRequest = $this->authenticationRequestService->createAuthenticationRequest($ui_locale, $redirect_url); + + return new JsonResponse($authRequest); } } diff --git a/service-api/app/src/App/src/Service/Authentication/OneLoginAuthenticationRequestService.php b/service-api/app/src/App/src/Service/Authentication/OneLoginAuthenticationRequestService.php index 9ee79f2a8c..e11c3a3c58 100644 --- a/service-api/app/src/App/src/Service/Authentication/OneLoginAuthenticationRequestService.php +++ b/service-api/app/src/App/src/Service/Authentication/OneLoginAuthenticationRequestService.php @@ -22,7 +22,7 @@ public function __construct( ) { } - public function createAuthenticationRequest(string $uiLocale): array + public function createAuthenticationRequest(string $uiLocale, string $redirectURL): array { $cachedBuilder = new MetadataProviderBuilder(); @@ -59,7 +59,7 @@ public function createAuthenticationRequest(string $uiLocale): array [ 'scope' => 'openid email', 'state' => $state, - 'redirect_uri' => 'http://localhost:9002/auth/redirect', //TODO: use dynamic domain UML-3121 + 'redirect_uri' => $redirectURL, 'nonce' => $nonce, 'vtr' => '["Cl.Cm.P2"]', 'ui_locales' => $uiLocale, diff --git a/service-api/app/test/AppTest/Service/Authentication/OneLoginAuthenticationRequestServiceTest.php b/service-api/app/test/AppTest/Service/Authentication/OneLoginAuthenticationRequestServiceTest.php index dfffef47d0..3966ce07ae 100644 --- a/service-api/app/test/AppTest/Service/Authentication/OneLoginAuthenticationRequestServiceTest.php +++ b/service-api/app/test/AppTest/Service/Authentication/OneLoginAuthenticationRequestServiceTest.php @@ -53,15 +53,13 @@ public function create_authentication_request(): void $this->issuerBuilder->reveal(), $this->cacheFactory->reveal(), ); - $authorisationRequest = $authorisationRequestService->createAuthenticationRequest('en'); + $fakeRedirect = 'http://fakehost/auth/redirect'; + $authorisationRequest = $authorisationRequestService->createAuthenticationRequest('en', $fakeRedirect); $authorisationRequestUrl = $authorisationRequest['url']; $this->assertStringContainsString('client_id=client-id', $authorisationRequestUrl); $this->assertStringContainsString('scope=openid+email', $authorisationRequestUrl); $this->assertStringContainsString('vtr=["Cl.Cm.P2"]', urldecode($authorisationRequestUrl)); $this->assertStringContainsString('ui_locales=en', $authorisationRequestUrl); - $this->assertStringContainsString( - 'redirect_uri=http://localhost:9002/auth/redirect', - urldecode($authorisationRequestUrl) - ); + $this->assertStringContainsString('redirect_uri=' . $fakeRedirect, urldecode($authorisationRequestUrl)); } } diff --git a/service-front/app/config/routes.php b/service-front/app/config/routes.php index 0b6ba0892b..1abe8b947e 100644 --- a/service-front/app/config/routes.php +++ b/service-front/app/config/routes.php @@ -100,6 +100,7 @@ $app->get('/session-expired', Actor\Handler\ActorSessionExpiredHandler::class, 'session-expired'); $app->get('/session-check', Actor\Handler\ActorSessionCheckHandler::class, 'session-check'); $app->get('/session-refresh', Common\Handler\SessionRefreshHandler::class, 'session-refresh'); + $app->get('/home/login', Actor\Handler\LoginPageHandler::class, 'auth-redirect'); $app->get( '/logout', diff --git a/service-front/app/features/context/UI/AccountContext.php b/service-front/app/features/context/UI/AccountContext.php index 22250b5d85..abe9f2f5e4 100644 --- a/service-front/app/features/context/UI/AccountContext.php +++ b/service-front/app/features/context/UI/AccountContext.php @@ -26,6 +26,7 @@ * @property string userEmailResetToken * @property string email * @property string password + * @property string language */ class AccountContext implements Context { @@ -44,6 +45,7 @@ class AccountContext implements Context private const USER_SERVICE_CAN_PASSWORD_RESET = 'UserService::canPasswordReset'; private const USER_SERVICE_COMPLETE_PASSWORD_RESET = 'UserService::completePasswordReset'; private const USER_SERVICE_DELETE_ACCOUNT = 'UserService::deleteAccount'; + private const ONE_LOGIN_SERVICE_AUTHENTICATE = 'OneLoginService::authenticate'; /** * @Then /^An account is created using (.*) (.*) (.*)$/ @@ -2037,6 +2039,7 @@ public function iReceiveAnEmailAndShownUniqueInstructionsOnHowToResetMyPassword( */ public function iAmOnTheTemporaryOneLoginPage(): void { + $this->language = 'en'; $this->ui->visit('/home'); $this->ui->assertPageAddress('/home'); $this->ui->assertElementContainsText('button[name=sign-in-one-login]', 'Sign in via One Login'); @@ -2057,26 +2060,37 @@ public function iClickTheOneLoginButton(): void 'url' => 'http://fake.url/authorize', ] ), - self::USER_SERVICE_REQUEST_PASSWORD_RESET + self::ONE_LOGIN_SERVICE_AUTHENTICATE ) ); $this->iDoNotFollowRedirects(); $this->ui->pressButton('Sign in via One Login'); $this->iDoFollowRedirects(); - - $request = $this->apiFixtures->getLastRequest(); - $params = $request->getUri()->getQuery(); - Assert::assertStringContainsString('ui_locale=en', $params); } /** - * @Then /^I am redirected to the redirect page$/ + * @Then /^I am redirected to the redirect page in (English|Welsh)$/ */ - public function iAmRedirectedToTheRedirectPage(): void + public function iAmRedirectedToTheRedirectPage($language): void { $locationHeader = $this->ui->getSession()->getResponseHeader('Location'); + $request = $this->apiFixtures->getLastRequest(); + $params = $request->getUri()->getQuery(); + $language = $language === 'English' ? 'en' : 'cy'; + assert::assertTrue(isset($locationHeader)); assert::assertEquals($locationHeader, 'http://fake.url/authorize'); + assert::assertEquals($language, $this->language); + assert::assertStringContainsString('ui_locale=' . $this->language, $params); + } + + /** + * @When /^I select the Welsh language$/ + */ + public function iSelectTheWelshLanguage(): void + { + $this->language = 'cy'; + $this->ui->clickLink('Cymraeg'); } } diff --git a/service-front/app/features/one-login.feature b/service-front/app/features/one-login.feature index 0b18d102a0..382ce36c4a 100644 --- a/service-front/app/features/one-login.feature +++ b/service-front/app/features/one-login.feature @@ -5,4 +5,11 @@ Scenario: I initiate authentication via one login Given I am on the temporary one login page When I click the one login button - Then I am redirected to the redirect page + Then I am redirected to the redirect page in English + + @ui @actor @ff:allow_gov_one_login:true @welsh + Scenario: I initiate authentication via one login in Welsh + Given I am on the temporary one login page + And I select the Welsh language + When I click the one login button + Then I am redirected to the redirect page in Welsh \ No newline at end of file diff --git a/service-front/app/src/Actor/src/Handler/AuthenticateOneLoginHandler.php b/service-front/app/src/Actor/src/Handler/AuthenticateOneLoginHandler.php index e2c81e26fb..f4ae73cd5e 100644 --- a/service-front/app/src/Actor/src/Handler/AuthenticateOneLoginHandler.php +++ b/service-front/app/src/Actor/src/Handler/AuthenticateOneLoginHandler.php @@ -16,6 +16,7 @@ use Facile\OpenIDClient\Session\AuthSession; use Laminas\Diactoros\Response\HtmlResponse; use Laminas\Diactoros\Response\RedirectResponse; +use Mezzio\Helper\ServerUrlHelper; use Mezzio\Helper\UrlHelper; use Mezzio\Session\SessionMiddleware; use Mezzio\Template\TemplateRendererInterface; @@ -39,6 +40,7 @@ public function __construct( UrlHelper $urlHelper, LoggerInterface $logger, private OneLoginService $authenticateOneLoginService, + private ServerUrlHelper $serverUrlHelper, ) { parent::__construct($renderer, $urlHelper, $logger); } @@ -48,12 +50,20 @@ public function handle(ServerRequestInterface $request): ResponseInterface $form = new OneLoginForm($this->getCsrfGuard($request)); if ($request->getMethod() === 'POST') { - $url = $this->urlHelper->generate(); - $uiLocale = (str_contains($url, '/cy/') ? 'cy' : 'en'); - $result = $this->authenticateOneLoginService->authenticate($uiLocale); + $signInLink = $this->serverUrlHelper->generate($this->urlHelper->generate('auth-redirect')); + $uiLocale = \Locale::getPrimaryLanguage($request->getAttribute('locale')); + + if ($uiLocale === 'cy') { + $signInLink = str_replace('/cy', '', $signInLink); + } + + $result = $this->authenticateOneLoginService->authenticate($uiLocale, $signInLink); + $result['customs'] = ['ui_locale' => $uiLocale]; + $this ->getSession($request, SessionMiddleware::SESSION_ATTRIBUTE) ?->set(self::OIDC_AUTH_INTERFACE, AuthSession::fromArray($result)); + return new RedirectResponse($result['url']); } diff --git a/service-front/app/src/Common/src/Service/OneLogin/OneLoginService.php b/service-front/app/src/Common/src/Service/OneLogin/OneLoginService.php index f9b643a40c..9af86873f6 100644 --- a/service-front/app/src/Common/src/Service/OneLogin/OneLoginService.php +++ b/service-front/app/src/Common/src/Service/OneLogin/OneLoginService.php @@ -12,10 +12,11 @@ public function __construct(private ApiClient $apiClient) { } - public function authenticate(string $uiLocale): ?array + public function authenticate(string $uiLocale, string $redirectUrl): ?array { return $this->apiClient->httpGet('/v1/auth-one-login', [ - 'ui_locale' => $uiLocale, + 'ui_locale' => $uiLocale, + 'redirect_url' => $redirectUrl, ]); } } diff --git a/service-front/app/test/CommonTest/Service/OneLogin/OneLoginServiceTest.php b/service-front/app/test/CommonTest/Service/OneLogin/OneLoginServiceTest.php index d7a8ab5625..61b86343f9 100644 --- a/service-front/app/test/CommonTest/Service/OneLogin/OneLoginServiceTest.php +++ b/service-front/app/test/CommonTest/Service/OneLogin/OneLoginServiceTest.php @@ -18,12 +18,13 @@ public function can_get_authentication_request_uri(): void { $state = 'STATE'; $nonce = 'aEwkamaos5B'; + $redirect = 'FAKE_REDIRECT'; $uri = '/authorize?response_type=code &scope=YOUR_SCOPES &client_id=YOUR_CLIENT_ID &state=' . $state . - '&redirect_uri=YOUR_REDIRECT_URI - &nonce=' . $nonce . + '&redirect_uri=' . $redirect . + '&nonce=' . $nonce . '&vtr=["Cl.Cm"] &ui_locales=en'; @@ -34,11 +35,12 @@ public function can_get_authentication_request_uri(): void '/v1/auth-one-login', [ 'ui_locale' => 'en', + 'redirect_url' => $redirect, ] )->willReturn(['state' => $state, 'nonce' => $nonce, 'url' => $uri]); $oneLoginService = new OneLoginService($apiClientProphecy->reveal()); - $response = $oneLoginService->authenticate('en'); + $response = $oneLoginService->authenticate('en', $redirect); $this->assertEquals(['state' => $state, 'nonce' => $nonce, 'url' => $uri], $response); } }