From 5d76e6ece41683aa157768056fe0316d674622fa Mon Sep 17 00:00:00 2001 From: Bence Kovacs Date: Tue, 5 Dec 2023 15:53:58 +0100 Subject: [PATCH] fix: fixes based on comments in pr #278 --- src/charm.py | 13 ++++------ tests/integration/oauth_tools/constants.py | 5 +++- tests/integration/oauth_tools/dex.py | 2 +- .../oauth_tools/oauth_test_helper.py | 4 ++-- tests/integration/test_grafana_oauth.py | 24 ++++++++++++++----- tests/unit/test_charm.py | 4 +++- 6 files changed, 33 insertions(+), 19 deletions(-) diff --git a/src/charm.py b/src/charm.py index 19780c76..e4534197 100755 --- a/src/charm.py +++ b/src/charm.py @@ -113,7 +113,7 @@ "/usr/local/share/ca-certificates/trusted-ca-cert-$rel_id-ca.crt" ) OAUTH = "oauth" -OAUTH_SCOPES = "openid email" +OAUTH_SCOPES = "openid email offline_access" OAUTH_GRANT_TYPES = ["authorization_code", "refresh_token"] @@ -312,7 +312,8 @@ def _on_config_changed(self, event: ConfigChangedEvent) -> None: def _on_ingress_ready(self, _) -> None: """Once Traefik tells us our external URL, make sure we reconfigure Grafana.""" - self.oauth.update_client_config(client_config=self._oauth_client_config) + if self.model.relations[OAUTH]: + self.oauth.update_client_config(client_config=self._oauth_client_config) self._configure() @@ -947,6 +948,8 @@ def _build_layer(self) -> Layer: "GF_AUTH_GENERIC_OAUTH_AUTH_URL": oauth_provider_info.authorization_endpoint, "GF_AUTH_GENERIC_OAUTH_TOKEN_URL": oauth_provider_info.token_endpoint, "GF_AUTH_GENERIC_OAUTH_API_URL": oauth_provider_info.userinfo_endpoint, + "GF_AUTH_GENERIC_OAUTH_USE_REFRESH_TOKEN": "True", + "GF_FEATURE_TOGGLES_ACCESS_TOKEN_EXPIRATION_CHECK": "True", } ) @@ -1427,14 +1430,8 @@ def _oauth_client_config(self) -> ClientConfig: def _on_oauth_info_changed(self, event: OAuthInfoChangedEvent) -> None: """Event handler for the oauth_info_changed event.""" - if not self.unit.is_leader(): - return - - self.oauth.update_client_config(client_config=self._oauth_client_config) logger.info(f"Received oauth provider info: {self.oauth.get_provider_info()}") - if not event.client_id or not event.client_secret_id: - return self.restart_grafana() def _on_oauth_info_removed(self, event: OAuthInfoRemovedEvent) -> None: diff --git a/tests/integration/oauth_tools/constants.py b/tests/integration/oauth_tools/constants.py index dbc115b1..9f2de9bf 100644 --- a/tests/integration/oauth_tools/constants.py +++ b/tests/integration/oauth_tools/constants.py @@ -23,6 +23,7 @@ "KRATOS", "KRATOS_EXTERNAL_IDP_INTEGRATOR", "IDENTITY_PLATFORM_LOGIN_UI_OPERATOR", + "SELF_SIGNED_CERTIFICATES", ], )( TRAEFIK_ADMIN="traefik-admin", @@ -31,14 +32,16 @@ KRATOS="kratos", KRATOS_EXTERNAL_IDP_INTEGRATOR="kratos-external-idp-integrator", IDENTITY_PLATFORM_LOGIN_UI_OPERATOR="identity-platform-login-ui-operator", + SELF_SIGNED_CERTIFICATES="self-signed-certificates", ) OAUTH_RELATION = collections.namedtuple( - "OAUTH_RELATION", ["OAUTH_APPLICATION", "OAUTH_INTERFACE", "OAUTH_PROXY"] + "OAUTH_RELATION", ["OAUTH_APPLICATION", "OAUTH_INTERFACE", "OAUTH_PROXY", "OAUTH_CERTIFICATES"] )( OAUTH_APPLICATION="hydra", OAUTH_INTERFACE="oauth", OAUTH_PROXY="traefik-public", + OAUTH_CERTIFICATES="self-signed-certificates", ) IDENTITY_BUNDLE = collections.namedtuple("IDENTITY_BUNDLE", ["NAME", "CHANNEL"])( diff --git a/tests/integration/oauth_tools/dex.py b/tests/integration/oauth_tools/dex.py index 60711580..d1650653 100644 --- a/tests/integration/oauth_tools/dex.py +++ b/tests/integration/oauth_tools/dex.py @@ -50,7 +50,7 @@ def update_redirect_uri(self, redirect_uri: str) -> None: self._redirect_uri = redirect_uri self._apply_dex_resources() - def close(self) -> None: + def remove_idp_service(self) -> None: # Removes the identity provider deployment if self._ops_test.keep_model: return diff --git a/tests/integration/oauth_tools/oauth_test_helper.py b/tests/integration/oauth_tools/oauth_test_helper.py index 0fc9dc05..8b049e73 100644 --- a/tests/integration/oauth_tools/oauth_test_helper.py +++ b/tests/integration/oauth_tools/oauth_test_helper.py @@ -72,8 +72,8 @@ async def deploy_identity_bundle(ops_test: OpsTest, external_idp_manager: Extern async def access_application_login_page(page: Page, url: str, redirect_login_url: str = ""): - """Convenience function for navigating the browser to the login page.""" - """ + """Convenience function for navigating the browser to the login page. + Usage: If the url of the application redirects to a login page, pass the application's url as url, and a pattern string for the login page as redirect_login_url. diff --git a/tests/integration/test_grafana_oauth.py b/tests/integration/test_grafana_oauth.py index 6b715501..975a9add 100644 --- a/tests/integration/test_grafana_oauth.py +++ b/tests/integration/test_grafana_oauth.py @@ -11,6 +11,7 @@ import logging from helpers import oci_image +import os import pytest import requests from playwright.async_api._generated import Page, BrowserContext @@ -26,7 +27,7 @@ verify_page_loads, get_cookie_from_browser_by_name, ) -from oauth_tools.constants import OAUTH_RELATION +from oauth_tools.constants import OAUTH_RELATION, EXTERNAL_USER_EMAIL logger = logging.getLogger(__name__) @@ -62,9 +63,17 @@ async def test_build_and_deploy(ops_test: OpsTest, grafana_charm): f"grafana:{OAUTH_RELATION.OAUTH_INTERFACE}", OAUTH_RELATION.OAUTH_APPLICATION ) await ops_test.model.integrate("grafana:ingress", f"{OAUTH_RELATION.OAUTH_PROXY}") + await ops_test.model.integrate( + "grafana:receive-ca-cert", f"{OAUTH_RELATION.OAUTH_CERTIFICATES}" + ) await ops_test.model.wait_for_idle( - apps=[OAUTH_RELATION.OAUTH_APPLICATION, "grafana", OAUTH_RELATION.OAUTH_PROXY], + apps=[ + OAUTH_RELATION.OAUTH_APPLICATION, + "grafana", + OAUTH_RELATION.OAUTH_PROXY, + OAUTH_RELATION.OAUTH_CERTIFICATES, + ], status="active", raise_on_blocked=False, raise_on_error=False, @@ -80,7 +89,7 @@ async def test_oauth_login_with_identity_bundle( grafana_proxy = await get_reverse_proxy_app_url( ops_test, OAUTH_RELATION.OAUTH_PROXY, "grafana" ) - redirect_login = f"{grafana_proxy}login" + redirect_login = os.path.join(grafana_proxy, "login") await access_application_login_page( page=page, url=grafana_proxy, redirect_login_url=redirect_login @@ -94,7 +103,7 @@ async def test_oauth_login_with_identity_bundle( page=page, ops_test=ops_test, external_idp_manager=external_idp_manager ) - redirect_url = grafana_proxy + "?*" + redirect_url = os.path.join(grafana_proxy, "?*") await verify_page_loads(page=page, url=redirect_url) # Verifying that the login flow was successful is application specific. @@ -103,10 +112,13 @@ async def test_oauth_login_with_identity_bundle( browser_context=context, name="grafana_session" ) request = requests.get( - f"{grafana_proxy}api/user", + os.path.join(grafana_proxy, "api/user"), headers={"Cookie": f"grafana_session={grafana_session_cookie}"}, verify=False, ) assert request.status_code == 200 + assert request.status_code == 200 + request.raise_for_status() + assert request.json()["email"] == EXTERNAL_USER_EMAIL - external_idp_manager.close() + external_idp_manager.remove_idp_service() diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index f15e871a..30c8345b 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -420,7 +420,9 @@ def test_config_is_updated_with_oauth_relation_data(self): services["environment"]["GF_AUTH_GENERIC_OAUTH_CLIENT_ID"], "grafana_client_id" ) self.assertEqual(services["environment"]["GF_AUTH_GENERIC_OAUTH_CLIENT_SECRET"], "s3cR#T") - self.assertEqual(services["environment"]["GF_AUTH_GENERIC_OAUTH_SCOPES"], "openid email") + self.assertEqual( + services["environment"]["GF_AUTH_GENERIC_OAUTH_SCOPES"], "openid email offline_access" + ) self.assertEqual( services["environment"]["GF_AUTH_GENERIC_OAUTH_AUTH_URL"], "https://example.oidc.com/oauth2/auth",