Skip to content
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.

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

IAM-951 fix broken href for non-empty context-path #426

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

BarcoMasile
Copy link
Contributor

@BarcoMasile BarcoMasile commented Sep 25, 2024

Description

Switch to html/template for rendering context path dynamically for index.html
This way when serving Admin UI under a context path we don't break the UI.
UI still expects to be under /context-path(if present)/ui.

N.B.

We still need web team intervention to make the produced index.html to have the following prefix for its assets: {{ . }}ui

Example

<head>
    <meta charset="UTF-8" />
    <meta name="viewport" content="width=device-width, initial-scale=1" />

    <title>Identity platform</title>
    <link rel="shortcut icon" href="https://assets.ubuntu.com/v1/49a1a858-favicon-32x32.png" type="image/x-icon" />

    <script>const global = globalThis;</script>
    <script type="module" crossorigin src="{{ . }}ui/assets/index-D2_UOyoG.js"></script>
    <link rel="stylesheet" crossorigin href="{{ . }}ui/assets/index-tyeITY-6.css">
  </head>

This PR was manually tested simulating a fake context-path under which the UI could be served. See the example with with-context-path used as a fake path. The index html file GETs the right path for the assets.

pr-context-path

Fixes

Fixes #350

@BarcoMasile BarcoMasile requested a review from a team as a code owner September 25, 2024 13:45
Copy link
Contributor

@nsklikas nsklikas left a comment

Choose a reason for hiding this comment

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

I don't really love this, but if it works, it works.

I have one question though:
If we are going to start templating/injecting the returned html, why don't we set the html base tag instead of editing paths? This seems much safer and extendable. Something like this:

diff --git a/pkg/ui/handlers.go b/pkg/ui/handlers.go
index b9b0a3e..842c9d8 100644
--- a/pkg/ui/handlers.go
+++ b/pkg/ui/handlers.go
@@ -102,10 +102,7 @@ func (a *API) uiFiles(w http.ResponseWriter, r *http.Request) {
                w.Header().Set("Cache-Control", "no-cache, no-store, must-revalidate")
                w.WriteHeader(http.StatusOK)
 
-               normContextPath := a.contextPath
-               if !strings.HasSuffix(normContextPath, "/") {
-                       normContextPath += "/"
-               }
+               normContextPath := r.URL.JoinPath(a.contextPath, UIPrefix, "/")
 
                err = t.Execute(w, normContextPath)
                if err != nil {
diff --git a/ui/index.html b/ui/index.html
index 4dd324f..ea803e4 100644
--- a/ui/index.html
+++ b/ui/index.html
@@ -4,6 +4,7 @@
   <head>
     <meta charset="UTF-8" />
     <meta name="viewport" content="width=device-width, initial-scale=1" />
+    <base href="{{ . }}" target="_blank">
 
     <title>Identity platform</title>
     <link rel="shortcut icon" href="https://assets.ubuntu.com/v1/49a1a858-favicon-32x32.png" type="image/x-icon" />

@BarcoMasile
Copy link
Contributor Author

I don't really love this, but if it works, it works.

I have one question though: If we are going to start templating/injecting the returned html, why don't we set the html base tag instead of editing paths? This seems much safer and extendable.

I think it's a good idea honestly, but from my notes from the meeting with @huwshimi we explicitly chose not to use the base path (I may have misinterpreted in this case though).

If it works, I am open to it and actually think it's better (as you said).
I would leave the decision to @huwshimi since his team owns the frontend part and including also the /ui part in the context will have some changes over there too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

React App routes and hrefs are broken
2 participants