search

canonical / identity-platform-admin-ui

Admin UI for the Canonical identity broker and identity provider solution
Other
6 stars 4 forks source link

Add UI serving #313

Closed nsklikas closed 3 months ago

nsklikas commented 3 months ago
nsklikas commented 3 months ago

The frontend is now served on /, all the API is served on /api (as it used to).

One interesting thing (which I think we should look into) is that the UI changes the path when you click on a component, e.g if you click on provider you will be redirected to /provider image

This means that if I try to refresh the page I will get a 404 because no such page exists.

edlerd commented 3 months ago

One interesting thing (which I think we should look into) is that the UI changes the path when you click on a component, e.g if you click on provider you will be redirected to /provider

This means that if I try to refresh the page I will get a 404 because no such page exists.

In other apps we serve under /ui and all paths below (like /ui/provider) get served as the same index file. That file loads some js that handles the current path correctly. Can we do a similar approach here as well?

nsklikas commented 3 months ago

In other apps we serve under /ui and all paths below (like /ui/provider) get served as the same index file. That file loads some js that handles the current path correctly. Can we do a similar approach here as well?

This feels a little wierd to implement in the backend. IMO the backend should not have to implement such logic, but I have no problem with adding it for now.

@BarcoMasile @shipperizer any problem with this?

Also @edlerd I am having trouble understanding how that works in the LXD code you sent before, are you sure that they implement this? (e.g. for /ui/providers serving /ui/)

BarcoMasile commented 3 months ago

In other apps we serve under /ui and all paths below (like /ui/provider) get served as the same index file. That file loads some js that handles the current path correctly. Can we do a similar approach here as well?

This feels a little wierd to implement in the backend. IMO the backend should not have to implement such logic, but I have no problem with adding it for now.

@BarcoMasile @shipperizer any problem with this?

I think @edlerd is referring to the react app handling URL change without actually invoking the backend. This should be ok doing it in the backend, we need a catch all route to make sure that all "non api" endpoint redirect to the same endpoint that returns the main index.html with the compiled react app

shipperizer commented 3 months ago

In other apps we serve under /ui and all paths below (like /ui/provider) get served as the same index file. That file loads some js that handles the current path correctly. Can we do a similar approach here as well?

This feels a little wierd to implement in the backend. IMO the backend should not have to implement such logic, but I have no problem with adding it for now.

@BarcoMasile @shipperizer any problem with this?

Also @edlerd I am having trouble understanding how that works in the LXD code you sent before, are you sure that they implement this? (e.g. for /ui/providers serving /ui/)

I'm limited in my knowledge here, but as far as my experience goes with frontends with single page paths were after a # in the URL

now I don't have any preference here but one thing I'm not really moving from is that backend logic stays in the backend, frontend logic stays in the frontend no meddling with boundaries please, unless we have a really good reason about it or this is the best practice and I'm completely off with my reasoning

nsklikas commented 3 months ago

now I don't have any preference here but one thing I'm not really moving from is that backend logic stays in the backend, frontend logic stays in the frontend

That's a good point and that's why I have another issue with this line: https://github.com/canonical/identity-platform-admin-ui/pull/313/files#diff-f792df7e2bcc8df566b7d3af852ed78b8fd98bef52587eb2ef643ab4429fb7d3R49

It defines a CSP about ubuntu.com and canonical.com, because (I suppose) the UI pulls assets from these servers. I think that usually in a deployment such headers would be added by the reverse proxy

Lukewh commented 3 months ago

My 2c — what @BarcoMasile said. Generally, there will be a catch-all route for all possible UI pages, so when a user visits /ui/*? you serve /ui/index.html and the SPA will pick up routing for subsequent routing. We could use the #/sub-path approach, but it's largely considered bad practice these days.

You all owe me 2c each.

shipperizer commented 3 months ago

now I don't have any preference here but one thing I'm not really moving from is that backend logic stays in the backend, frontend logic stays in the frontend

That's a good point and that's why I have another issue with this line: https://github.com/canonical/identity-platform-admin-ui/pull/313/files#diff-f792df7e2bcc8df566b7d3af852ed78b8fd98bef52587eb2ef643ab4429fb7d3R49

It defines a CSP about ubuntu.com and canonical.com, because (I suppose) the UI pulls assets from these servers. I think that usually in a deployment such headers would be added by the reverse proxy

the CORS should come via env variable https://github.com/canonical/identity-platform-admin-ui/pull/313/files#diff-e207df27591c5716f54912cf2c0c7e63ffc26755540755cf20ed34fb750f6e04R70 u can use those probably

shipperizer commented 3 months ago

why did we remove the api prefix?

right I see what happened, let me add some proper comments for a more minimal approach that might work the same

nsklikas commented 3 months ago

The Content-Security-Policy might need updating, did you test serving the ui and ensured all pages load as expected and entities can be created / edited / deleted through the forms? I didn't have an easy way to QA.

This is exactly why it's not a good idea to have the backend setting these headers, I tested all the pages and it seems to work fine.

edlerd commented 3 months ago

This is exactly why it's not a good idea to have the backend setting these headers, I tested all the pages and it seems to work fine.

Thanks for checking!

I don't have a better approach, as the UI code can't control or set the headers. I think the UI needs to depend on the backend for the security headers, we can't remove the dependency here.

nsklikas commented 3 months ago

Should be good to go

nsklikas commented 3 months ago

After testing it with traefik, I moved all the ui assets back to /ui, the reason is we cannot tell if the URL that the user used has a trailing slash or not for the root url (due to traefik re-writing the path).

To serve the UI under the root path, traefik would have to provide a middleware that adds a trailing slash to all URLs if it is missing (it is possible, but the charm does not implement it).

The index page now works fine, but the UI needs to fix its pathing so that it works with relative paths. This may cause new issue to come to light.