From c14abec16e035360aed536961e3cf3d1e4cd6dc0 Mon Sep 17 00:00:00 2001 From: Graham Dumpleton Date: Thu, 22 Aug 2024 17:02:58 +1000 Subject: [PATCH 1/2] Not merging notification param with redirection URL params correctly. --- .../apps/workshops/views/environment.py | 33 ++++++++-- .../project/apps/workshops/views/helpers.py | 46 ++++++++++++++ .../project/apps/workshops/views/session.py | 63 +++++++++++++------ 3 files changed, 118 insertions(+), 24 deletions(-) create mode 100644 training-portal/src/project/apps/workshops/views/helpers.py diff --git a/training-portal/src/project/apps/workshops/views/environment.py b/training-portal/src/project/apps/workshops/views/environment.py index 547c7187..ab2108a9 100644 --- a/training-portal/src/project/apps/workshops/views/environment.py +++ b/training-portal/src/project/apps/workshops/views/environment.py @@ -36,6 +36,7 @@ from ..manager.sessions import retrieve_session_for_user from ..manager.locking import resources_lock from ..models import TrainingPortal, Environment, EnvironmentState, SessionState +from .helpers import update_query_params @login_required @@ -56,12 +57,22 @@ def environment(request, name): instance = Environment.objects.get(name=name) except Environment.DoesNotExist: if index_url: - return redirect(index_url + "?notification=workshop-invalid") + return redirect( + update_query_params(index_url, {"notification": "workshop-invalid"}) + ) if not request.user.is_staff and settings.PORTAL_INDEX: - return redirect(settings.PORTAL_INDEX + "?notification=workshop-invalid") + return redirect( + update_query_params( + settings.PORTAL_INDEX, {"notification": "workshop-invalid"} + ) + ) - return redirect(reverse("workshops_catalog") + "?notification=workshop-invalid") + return redirect( + update_query_params( + reverse("workshops_catalog"), {"notification": "workshop-invalid"} + ) + ) # Retrieve a session for the user for this workshop environment. @@ -71,12 +82,22 @@ def environment(request, name): return redirect("workshops_session", name=session.name) if index_url: - return redirect(index_url + "?notification=session-unavailable") + return redirect( + update_query_params(index_url, {"notification": "session-unavailable"}) + ) if not request.user.is_staff and settings.PORTAL_INDEX: - return redirect(settings.PORTAL_INDEX + "?notification=session-unavailable") + return redirect( + update_query_params( + settings.PORTAL_INDEX, {"notification": "session-unavailable"} + ) + ) - return redirect(reverse("workshops_catalog") + "?notification=session-unavailable") + return redirect( + update_query_params( + reverse("workshops_catalog"), {"notification": "session-unavailable"} + ) + ) @require_http_methods(["GET"]) diff --git a/training-portal/src/project/apps/workshops/views/helpers.py b/training-portal/src/project/apps/workshops/views/helpers.py new file mode 100644 index 00000000..977e17ba --- /dev/null +++ b/training-portal/src/project/apps/workshops/views/helpers.py @@ -0,0 +1,46 @@ +"""Helper functions for dealing with URLs.""" + +from urllib.parse import urlparse, parse_qs, urlencode, urlunparse, urljoin + + +def update_query_params(url, params): + """Update the query parameters of a URL with the given parameters. If the URL + is malformed or cannot be parsed, the original URL is returned.""" + + try: + # Parse the URL. + + parsed_url = urlparse(url) + + # Handle URLs with no scheme or netloc (e.g., paths like '/page'). + + if not parsed_url.scheme and not parsed_url.netloc: + # Treat it as a relative path URL. + + base_url = "http://dummy" # Temporary base for relative path handling + parsed_url = urlparse(urljoin(base_url, url)) + + # Parse existing query parameters. + + query_params = parse_qs(parsed_url.query) + + # Update or add the new parameters. + + query_params.update({key: [value] for key, value in params.items()}) + + # Reconstruct the URL with the updated query string. + + updated_query = urlencode(query_params, doseq=True) + updated_url = urlunparse(parsed_url._replace(query=updated_query)) + + # If the URL was originally a path, strip out the dummy scheme and netloc. + + if parsed_url.scheme == "http" and parsed_url.netloc == "dummy": + return updated_url.replace("http://dummy", "") + + return updated_url + + except Exception: # pylint: disable=broad-except + # In case of any parsing errors, return the original URL. + + return url diff --git a/training-portal/src/project/apps/workshops/views/session.py b/training-portal/src/project/apps/workshops/views/session.py index aa615c4b..4ee93eaa 100644 --- a/training-portal/src/project/apps/workshops/views/session.py +++ b/training-portal/src/project/apps/workshops/views/session.py @@ -42,6 +42,7 @@ from ..manager.sessions import update_session_status, create_request_resources from ..manager.analytics import report_analytics_event from ..models import TrainingPortal, SessionState +from .helpers import update_query_params @login_required(login_url="/") @@ -67,25 +68,35 @@ def session(request, name): if not instance: if index_url: - return redirect(index_url + "?notification=session-invalid") + return redirect( + update_query_params(index_url, {"notification": "session-invalid"}) + ) if not request.user.is_staff and settings.PORTAL_INDEX: - return redirect(settings.PORTAL_INDEX + "?notification=session-invalid") - - return redirect(reverse("workshops_catalog") + "?notification=session-invalid") + return redirect( + update_query_params( + settings.PORTAL_INDEX, {"notification": "session-invalid"} + ) + ) + + return redirect( + update_query_params( + reverse("workshops_catalog"), {"notification": "session-invalid"} + ) + ) context["session"] = instance context["session_owner"] = instance.owner and instance.owner.get_username() or "" - context[ - "session_url" - ] = f"{settings.INGRESS_PROTOCOL}://{instance.name}.{settings.INGRESS_DOMAIN}" + context["session_url"] = ( + f"{settings.INGRESS_PROTOCOL}://{instance.name}.{settings.INGRESS_DOMAIN}" + ) portal_url = f"{settings.INGRESS_PROTOCOL}://{settings.PORTAL_HOSTNAME}" - context[ - "restart_url" - ] = f"{portal_url}/workshops/session/{instance.name}/delete/?notification=startup-timeout" + context["restart_url"] = ( + f"{portal_url}/workshops/session/{instance.name}/delete/?notification=startup-timeout" + ) context["startup_timeout"] = instance.environment.overdue.total_seconds() try: @@ -217,12 +228,22 @@ def session_delete(request, name): if not instance: if index_url: - return redirect(index_url + "?notification=session-invalid") + return redirect( + update_query_params(index_url, {"notification": "session-invalid"}) + ) if not request.user.is_staff and settings.PORTAL_INDEX: - return redirect(settings.PORTAL_INDEX + "?notification=session-invalid") - - return redirect(reverse("workshops_catalog") + "?notification=session-invalid") + return redirect( + update_query_params( + settings.PORTAL_INDEX, {"notification": "session-invalid"} + ) + ) + + return redirect( + update_query_params( + reverse("workshops_catalog"), {"notification": "session-invalid"} + ) + ) # Mark the instance as stopping now so that it will not be picked up # by the user again if they attempt to create a new session immediately. @@ -239,12 +260,18 @@ def session_delete(request, name): notification = "session-deleted" if index_url: - return redirect(index_url + f"?notification={notification}") + return redirect(update_query_params(index_url, {"notification": notification})) if not request.user.is_staff and settings.PORTAL_INDEX: - return redirect(settings.PORTAL_INDEX + f"?notification={notification}") - - return redirect(reverse("workshops_catalog") + f"?notification={notification}") + return redirect( + update_query_params(settings.PORTAL_INDEX, {"notification": notification}) + ) + + return redirect( + update_query_params( + reverse("workshops_catalog"), {"notification": notification} + ) + ) @protected_resource() From e86921c72ccaf845860daa6ce0e4460a09f55132 Mon Sep 17 00:00:00 2001 From: Graham Dumpleton Date: Thu, 22 Aug 2024 17:05:30 +1000 Subject: [PATCH 2/2] Document redirection query string param fix. --- project-docs/release-notes/version-3.0.0.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/project-docs/release-notes/version-3.0.0.md b/project-docs/release-notes/version-3.0.0.md index cee5043b..3bf90747 100644 --- a/project-docs/release-notes/version-3.0.0.md +++ b/project-docs/release-notes/version-3.0.0.md @@ -120,3 +120,10 @@ Bugs Fixed had not been created via the REST API but by the web interface would result in an internal error. Now properly disallow the request for this case and return an error saying session cannot be reacquired. + +* When an index URL was supplied to the training portal in the `TrainingPortal` + resource, or via the REST API, if the URL had query string parameters, the + query string param added by the training platform with the notification + message, was not being merged with the existing set of query string parameters + and was instead being added to the value of the last query string parameter in + the URL.