fix: authenticate per request #2258

dpschen · 2022-08-14T10:10:13Z

dpschen commented

2022-08-14 10:10:13 +00:00

It seems to me that the token should be checked on every request.

I'm not sure if this should also be used this way with the the AuthenticatedHTTPFactory.

It seems to me that the token should be checked on every request. I'm not sure if this should also be used this way with the the AuthenticatedHTTPFactory.

dpschen added the

kind/bug

label 2022-08-14 10:10:13 +00:00

konrad was assigned by dpschen

2022-08-14 10:10:13 +00:00

frederick commented

2022-08-14 10:14:41 +00:00

Hi dpschen!

Thank you for creating a PR!

I've deployed the changes of this PR on a preview environment under this URL: https://2258-feature-authenticate-per-request--vikunja-frontend-preview.netlify.app

You can use this url to view the changes live and test them out.
You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/.

Have a nice day!

Beep boop, I'm a bot.

Hi dpschen! Thank you for creating a PR! I've deployed the changes of this PR on a preview environment under this URL: https://2258-feature-authenticate-per-request--vikunja-frontend-preview.netlify.app You can use this url to view the changes live and test them out. You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/. Have a nice day! > Beep boop, I'm a bot.

dpschen changed title from ~~WIP: fix: authenticate per request~~ to fix: authenticate per request

2022-08-14 15:48:54 +00:00

dpschen requested review from konrad 2022-08-14 15:49:16 +00:00

konrad commented

2022-08-15 15:01:07 +00:00

There are cases where it would not be checked, like the login and register routes. IIRC these don't use a service, hence it made sense to put the check into AbstractService and not in the common http handler.

There are cases where it would not be checked, like the login and register routes. IIRC these don't use a service, hence it made sense to put the check into `AbstractService` and not in the common http handler.

👍 1

dpschen force-pushed feature/authenticate-per-request from 3098d9c70e to 24d67b512b

2022-08-18 09:41:21 +00:00

Compare

dpschen force-pushed feature/authenticate-per-request from 24d67b512b to 5b06302c23

2022-08-18 16:20:38 +00:00

Compare

dpschen force-pushed feature/authenticate-per-request from 5b06302c23 to 3b679bf273

2022-08-19 09:14:18 +00:00

Compare

konrad requested changes 2022-09-02 15:04:16 +00:00

src/services/abstractService.ts Outdated

						
				@ -66,2 +66,4 @@

						// Set the interceptors to process every request

						this.http.interceptors.request.use((config) => {

							// Set the default auth header if we have a token

							const token = getToken()

konrad commented

2022-09-02 15:03:36 +00:00

Isn't this now duplicated with the code in src/http-common/index.ts?

Isn't this now duplicated with the code in `src/http-common/index.ts`?

dpschen commented

2022-09-02 15:55:46 +00:00

I guess it could use the AuthenticatedHTTPFactory.
Here I was simply replacing all static token occurances with dynamic ones. It was static in line 91

I guess it could use the AuthenticatedHTTPFactory. Here I was simply replacing all static token occurances with dynamic ones. It was static in line 91

konrad commented

2022-09-03 10:34:25 +00:00

I'm not 100% sure if it would be easy or make sense but I think we shouf put this in a function used in both places.

dpschen commented

2022-09-03 11:59:02 +00:00

What's the difference to using AuthenticatedHTTPFactory directly?

konrad commented

2022-09-03 13:09:18 +00:00

I think that could work. Can you check?

dpschen commented

2022-09-04 10:50:40 +00:00

Done.

Now I also set the apiUrl fresh from window for every request.
By doing so we should never run in the case where an api instance exists but uses an outdated baseUrl (if the user changes it).

Done. Now I also set the `apiUrl` fresh from window for every request. By doing so we should never run in the case where an api instance exists but uses an outdated baseUrl (if the user changes it).