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

OnNav runs before being able to access localstorage states in v10? #992

Open
frimod1 opened this issue Sep 20, 2024 · 14 comments
Open

OnNav runs before being able to access localstorage states in v10? #992

frimod1 opened this issue Sep 20, 2024 · 14 comments

Comments

@frimod1
Copy link

frimod1 commented Sep 20, 2024

Hello,

Im migrating a project from v9 to v10 and ran into some problems regarding states.

My app is built with a menu sidebar and a "content" component that updates depending on what the user clicks in the menu.
Im using OnNav to run an authentication check towards my backend upon every "content" component change but now in v10 I cant access my states at this time?

This same setup worked in v9. Did I miss some bigger architecture change?

@maxence-charriere
Copy link
Owner

Can i see your code?

@maxence-charriere
Copy link
Owner

What's up with this, local storage access is independent from component lifecycle. There should not be an issue with OnNav.

@frimod1
Copy link
Author

frimod1 commented Oct 29, 2024

Sorry for a long delay, been busy with other stuff

// Every time the user navigates to a new page an authcheck is sent to confirm
// that the user session is valid (cookie) and that the user has permission
// to view the requested page. If the check fails the user is logged out.
func (ui *userInfo) OnNav(ctx app.Context) {
        // test
	var backend_addr string
	ctx.GetState("backend_addr", &backend_addr)
	log.Println("OnNav addr:", backend_addr)
	//app.Window().Call("alert", xxx)

	authCheck(ctx.Page().URL().Path, ctx)
}

// Check if a user-session is valid by sending the session cookie to the server
// and checking if a user is allowed to visit the requested page.
func authCheck(authPage string, ctx app.Context) bool {
	fmt.Println("authCheck ", authPage)

	authPage = strings.Trim(authPage, "/") // trims leading slash
	data := &DataStruct{
		Requested_page: authPage,
	}

	var backend_addr string
	var client http.Client
	ctx.GetState("backend_addr", &backend_addr)
	ctx.GetState("cookiejar-client", &client)

	fmt.Println("authCheck backend_addr:", backend_addr)

        // send request...
	resp, err := httpRequestNew(http.MethodPost, backend_addr+"/v1/authcheck", &client, data)
	// handle response and logic...

image
it seems to work

until I reload the page:

2024/10/29 10:14:02 OnNav addr: 
authCheck  /overview
authCheck backend_addr: 
httpRequestNew: POST /v1/authcheck
Error occured. Error is: Post "/v1/authcheck": unsupported protocol scheme ""
panic: runtime error: index out of range [1] with length 1

goroutine 48 [running]:
main.authCheck.logRequest.func1()
	/home/vs/git/repo/src/login.go:300 +0x409
github.com/maxence-charriere/go-app/v10/pkg/app.(*engineX).async.func1()
	/home/vs/git/repo/vendor/github.com/maxence-charriere/go-app/v10/pkg/app/engine.go:359 +0x23
created by github.com/maxence-charriere/go-app/v10/pkg/app.(*engineX).async in goroutine 13
	/home/vs/git/repo/vendor/github.com/maxence-charriere/go-app/v10/pkg/app/engine.go:358 +0x7c

backend_addr is not loaded since its trying to send a request to just /v1/authcheck instead of https://localhost:8085/v1/authcheck

@maxence-charriere
Copy link
Owner

maxence-charriere commented Oct 29, 2024

It seems like the state is set elsewhere.
What the code where you set the state look like?
Chances are the state is set after the OnNav of this component.

@frimod1
Copy link
Author

frimod1 commented Oct 29, 2024

ctx.SetState("backend_addr", fmt.Sprintf("https://%s:%s", backend_addr, backend_port)).PersistWithEncryption()

Im running this in OnMount() from my loginpage. Successful login -> redirect to other pages that read the state. The state should persist when refreshing right? It worked like this before in v9 without problems.

@oderwat
Copy link
Contributor

oderwat commented Oct 29, 2024

Just some comments:

  • we just use err := c.ctx.LocalStorage().Set("some_key", "some value") instead of using a state for this kind of data. I wonder what is the benefit of using State() in this case?
  • I never realized there is encryption for the persisted data and think that this implementation is very insecure and just scrambles data as the key is easily obtained in the frontend.
  • This kind of checking auth may be insecure, depending on what data you send (signing with keys for example).
  • Even if there is a more secure check, there is still potential when the protected data or functionality is available in the frontend.
  • Having the application checking permissions on every page navigation may also be a bit much. We use JWT that are created on login and have an expiration time. The frontend can check the permission without going to the server but needs to revalidate this in sane intervals by getting a new JWT before expiration of the old one.
  • And my usual comment: PWA are actually made for offline usage and asking the server on every navigation makes this a bit pointless. @maxence-charriere I think I asked you somewhere else Go-App should not also support a non PWA setup (without the service worker) for all the people that just use it as a "Go in the frontend web server"?

@maxence-charriere
Copy link
Owner

Could you put a log before the setstate to see where it is called?

@frimod1
Copy link
Author

frimod1 commented Oct 29, 2024

@oderwat I will try that tomorrow. You have a ctx pointer in your struct that you assign in OnMount? Encryption works fine. My webapp works until I refresh it.

@maxence-charriere Can you explain this? Put a log in a function to see where it is called? Its called from my login-page's login function?

@oderwat
Copy link
Contributor

oderwat commented Oct 29, 2024

We often copy the ctx to the component in OnMount() and nil it OnDismount() because we need it sometimes in custom event handlers. This slipped in as I copied some code.

Using LocalStorage() instead of State() is because I actually think of "State()" as a mechanism for observables or at least for something that changes in its lifetime. I also find it less obscure for just storing some data. But this is probably just nitpicking and my view of things.

Encryption works fine

Sure, but it uses ctx.Encrypt() that uses ctx.DeviceID() for the key. Which is a UUID stored in local storage. So everybody can simply grab that key using devs tools and decrypt the data. This is why I wrote it is insecure. I would rather just use no encryption (base64) or some rot13() which in this case to me is "as secure" but does not need a full AES encoder. Each byte in the frontend counts when it comes to the WASM size. Also "Encryption" implies something that is not given when using a key like that :)

@oderwat
Copy link
Contributor

oderwat commented Oct 29, 2024

Could you put a log before the setstate to see where it is called?

I guess this should mean "when" it gets called. I would add logs before the set/get lines to see in what order they are used.

@frimod1
Copy link
Author

frimod1 commented Oct 30, 2024

  • Having the application checking permissions on every page navigation may also be a bit much. We use JWT that are created on login and have an expiration time. The frontend can check the permission without going to the server but needs to revalidate this in sane intervals by getting a new JWT before expiration of the old one.

Might be more optimal than our authcheck that runs all the time. Wanted to be 100% secure noone accesses something they should not be able to.

  • And my usual comment: PWA are actually made for offline usage and asking the server on every navigation makes this a bit pointless. @maxence-charriere I think I asked you somewhere else Go-App should not also support a non PWA setup (without the service worker) for all the people that just use it as a "Go in the frontend web server"?

Do it max 😆
The service worker bugs in v9 are part of the reason we have to go to v10

Could you put a log before the setstate to see where it is called?

On my login page.
It works like this:

  1. login page reads app.Getenv to get my server backend address since it can change depending on configuration. This address is put into a state and a client is created which saves a session cookie from my initial login to my server, this client is also put in a state. Redirect user to /overview page. From here on we should have every state setup from /login and we wont access that page until user logs out.
ctx.LocalStorage().Set("backend_addr", full_backend_addr) # trying this way, same problem
fmt.Println("setting local storage oderwat style >:)")

image
image

  1. While navigating around Im running my authcheck from OnNav() on my menu component so its always called when swapping pages. My authcheck calls my small httlib helper which prints my state:
ctx.LocalStorage().Get("backend_addr", &backend_addr)
ctx.GetState("cookiejar-client", &client)
fmt.Println("httpReq localstorage:", backend_addr)

Which works fine, I get the address printed.
image

  1. As soon as I refresh the page everything dies.
    go-app server output: (why are variables printed here upon crash and which ones are picked?)
2024/10/30 16:20:04 Log: Logged in as user: max
authCheck  /overview
httpRequest: POST /v1/authcheck
httpReq localstorage:                 <----------------- EMPTY???

I just dont understand how this happens since it worked in v9. Is the state management too slow?

Also a bonus question; are there no docs from changes other than the release notes?
I noticed that any script files im loading in can no longer contain capital letters and the favicon doesnt seem to work anymore?

Thanks for replies

@oderwat
Copy link
Contributor

oderwat commented Oct 30, 2024

Well, this leaves open what the full logging output for the initial and the reload case looks like. Also: Is the data still inside the local storage at that point using dev tools?

I wonder why you store the server location at all in local storage? If you hand it over from backend to frontend using app.Env it is available in every part of your frontend code anyway.

We make extensively use of local storage and index db without having problems on reloads. So I guess there is something just not working as you expect it.

More debug output may help. We actually have every single call with debug output and implemented a way to download pprof traces. Just because stuff is not always working as we "think" it does.

@frimod1
Copy link
Author

frimod1 commented Nov 7, 2024

Managed to solve it but I dont even understand HOW this solution makes it work.
Basically I added another authCheck method that runs in the frontend only, this one is called from OnNav from another component that is also in my side menu. After this authCheck is called, the original authCheck runs and now it works.

I tried adding sleeps since I thought my backend authcheck (which got empty states) loaded before the states somehow, didnt work.
I tried adding random state accesses before authchecking since i thought this might somehow update them, didnt work.

func (lm *leftMenu) OnNav(ctx app.Context) {
	endpoint := ctx.Page().URL().Path

	// Dont remove this or the real authcheck towards the backend doesnt work
	// Noone knows why
	if !lm.authCheck(endpoint, ctx) {
		app.Log("Autcheck denied:", endpoint)
		ctx.Navigate("/login")
	}
}

// return true if we are allowed to visit given page
func (lm *leftMenu) authCheck(endpoint string, ctx app.Context) bool {
	var allowedPages []string
	ctx.GetState("allowed-pages", &allowedPages)

	endpoint = strings.Trim(endpoint, "/")

	return Contains(allowedPages, endpoint)
}

Weird :D

@frimod1
Copy link
Author

frimod1 commented Nov 7, 2024

PS The favicon is broken without any docs about what changed with it. A bug or something Im missing?
The logo is spinning while loading the page so it should be loaded.

http.Handle("/", &app.Handler{
	Icon: app.Icon{Default: "/web/logo.png"},
        ...
}

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

No branches or pull requests

3 participants