Checks for auth token on app refresh

[vindex10]

Hi, All!

I noticed that, at least, in case of basic auth, the token stored in the storage is not checked for. Therefore, upon refresh user needs to login again.

I suggest some potential change that fixes this. I'm a newcomer to dispatch, so I expect I will learn something new soon :)

Look forward to your feedback!

Cheers,
Victor

[mvilanova]

Thanks for the PR, @vindex10 . I'll take a look and circle back soon.

[mvilanova]

Changes look good to me. I'll let @kevgliss take a look at them before merging them.

[kevgliss]

@vindex10 Could you explain a little more about the behavior you are seeing? Locally, I can't replicate what you describe (a page refresh does not force login if the user has a valid token).

If the app state gets wiped (e.g. by a refresh), we explicitly test for an existing token here and use if it's available:

dispatch/src/dispatch/static/dispatch/src/auth/basicAuthProvider.js

Line 4 in 138c983

let token = localStorage.getItem("token")

Local storage should not be wiped out by a refresh (a user needs to do a hard refresh or otherwise delete this data).

[vindex10]

OK, thanks! indeed it works. For dev, I had to:

export VITE_DISPATCH_AUTHENTICATION_PROVIDER_SLUG=dispatch-auth-provider-basic

For prod I had to change docker-compose.yaml:

core:
    build:
        context: ../dispatch
        args:
            VITE_DISPATCH_AUTHENTICATION_PROVIDER_SLUG: "dispatch-auth-provider-basic"

And basic auth is enabled by default in the backend? So should this become a part of?
https://github.com/Netflix/dispatch-docker

[vindex10]

upd: disregard this. it was a wrong guess, and the message above works now for both dev and compose run.

Hmm. It actually didn't work with docker compose. And I have a suspect.

In the setup.py, it looks like the env is not propagated to the npm command:

dispatch/setup.py

Line 147 in e52af85

self._run_npm_command(["run", "build", "--quiet"])

and build_static that used to build the frontend before, doesn't do it anymore:

dispatch/setup.py

Line 306 in e52af85

def _build_static(self):

but it actually handled env vars before.

[vindex10]

sorry for the noise :) in summary:

1. I was wrong. env in setup py is fine.
2. I had to change build-args for core not for web
3. I corrected my messages above so that they look less confusing

I'm trying to understand now, which PR to open :) I see the options:

1. Set default arg in the Netflix/dispatch dockerfile (moderate intrusive option)
2. Set build arg in the Netflix/dispatch-docker, in the docker-compose.yaml (the least intrusive option)
3. Set default in router (we are consistent with backend, and api.js)
4. just mention in the docs that empty auth slug is a feature and how to use it + comment that for the basic auth you need to specify the variable VITE_DISPATCH_AUTHENTICATION_PROVIDER_SLUG explicitly during build
5. do nothing :)

What confuses me is the slight inconsistency between:

api.js:

dispatch/src/dispatch/static/dispatch/src/api.js

Line 12 in 138c983

import.meta.env.VITE_DISPATCH_AUTHENTICATION_PROVIDER_SLUG || "dispatch-auth-provider-basic"

where default auth is assumed to be "basic auth"

router/index.js:

dispatch/src/dispatch/static/dispatch/src/router/index.js

Line 39 in 138c983

let authProviderSlug = import.meta.env.VITE_DISPATCH_AUTHENTICATION_PROVIDER_SLUG

here empty authentication slug falls back to custom auth (which seems to be a feature, not a bug)

backend config.py:

dispatch/src/dispatch/config.py

Line 100 in 17e1a64

"DISPATCH_AUTHENTICATION_PROVIDER_SLUG", default="dispatch-auth-provider-basic"

@kevgliss look forward to your feedback )

[kevgliss]

I think your right when you point out the inconsistency here:

dispatch/src/dispatch/static/dispatch/src/router/index.js

Line 39 in 138c983

let authProviderSlug = import.meta.env.VITE_DISPATCH_AUTHENTICATION_PROVIDER_SLUG

I think the change should align the default to basic-auth similar to what we do here:

dispatch/src/dispatch/static/dispatch/src/api.js

Line 12 in 138c983

import.meta.env.VITE_DISPATCH_AUTHENTICATION_PROVIDER_SLUG || "dispatch-auth-provider-basic"

So, in summary I think we should define:

const authProviderSlug =
  import.meta.env.VITE_DISPATCH_AUTHENTICATION_PROVIDER_SLUG || "dispatch-auth-provider-basic"

here:

dispatch/src/dispatch/static/dispatch/src/router/index.js

Line 20 in 138c983

I think this is option # 3 that you mentioned.

[vindex10]

@kevgliss done. I checked, at least it worked when I ran locally :)

[kevgliss]

LGTM, thanks for the contribution!