handle unauthorized calls' return

[TheGostKasper]

Closes

What changed?
Provide a redirect to signin form when unauthorized call is emitted

[ahussein3]

There is already an Unauthorized interceptor that you can make use of here you can do that change here at the pipeline provider. As that change is duplicate of logic.

[TheGostKasper]

Using UnAuthorizedInterceptor which invoke a catch to every endpoint and check 401 to redirect to sign_in is valid but instead of wrapping services and change every provider then why not apply the same logic to one place which each endpoint use and save the wrapping as in our case will be :

• Progressive delivery
• pipelines
• terraform

also in the future when we add more providers, we won't worry about this issue again and taking in consideration the alert and global error handler in one place if we want to do so !?
@ahussein3

[ahussein3]

I Believe in having one interceptor all the components using it (Core, EE) is much better than having it two times.

[TheGostKasper]

let's stick to EE then

[foot]

Yeah we want to avoid multiple ways of doing this if possible. Can we keep using the UnAuthorizedInterceptor everywhere or remove it everywhere (in core too)? If we go down this road then probably some util fn in core we can use in EE ?

import { maybeLogout } from "@weaveworks/weave-gitops";

...[snip]...
      onError: error => { maybeLogout(error); }

Agree having the logic in a single spot is nice. However should the handlers be on the QueryCache itself too? https://react-query-v3.tanstack.com/reference/QueryCache#global-callbacks

[TheGostKasper]

In prev commit i used queryCache and noticed that for a sec Error displayed in UI Auth message before redirect to signin , I'm Ok to revert the last commit and keep it to queryCache .

Right, so we're keeping the error handler to useQuery and removing UnAuthorizedInterceptor everywhere else in EE (for this PR) ? then in a followup PR, export onError and remove UnAuthorizedInterceptor as well ?

[foot]

Right, so we're keeping the error handler to useQuery and removing UnAuthorizedInterceptor everywhere else in EE

Yes, does this approach work in core too? E.g. when working with the login handlers themselves or do they need to escape this somehow? Maybe they already use the UnAuthorizedInterceptor when calling signin so there is no real behaviour change.

I'm Ok to revert the last commit and keep it to queryCache .

My concern is that the queryCache handlers are always there, while doing it on the query-client only set defaults which we would be overriding in some places right now (e.g.

weave-gitops-enterprise/ui-cra/src/hooks/templates.tsx

Lines 16 to 24 in 00e3681

	const { isLoading, data } = useQuery<ListTemplatesResponse, Error>(
	'templates',
	() => api.ListTemplates({}),
	{
	keepPreviousData: true,
	onError,
	},
	);

), and in this case the auth-checking would be removed.

It would be nice to avoid a "flash of error message" before redirecting but maybe its unavoidable using this approach..

[TheGostKasper]

Yes, does this approach work in core too? E.g. when working with the login handlers themselves or do they need to escape this somehow?

Yes, core uses react-client as well

It would be nice to avoid a "flash of error message" before redirecting but maybe its unavoidable using this approach..

it's unavoidable yes

const { isLoading, data } = useQuery<ListTemplatesResponse, Error>( ...

In this case we might move the the notification to the shared onError or we keep the "flash of message" , any other Options !?

@foot

[TheGostKasper]

@foot, what do you think 👆 ?

[enekofb]

tested that works by

1. logged in, i could see resource

2) deleted cookie token

3. sent to login

4. after login, I am back to the last screen
   btw, is not pipelines but flux controllers cause the screenshot was taken in that flow but all good

[enekofb]

do we need any unit tests for this behaviour?

[enekofb]

Tested that works and the change make logical sense to me so approving based on that.

@TheGostKasper other concerns like interceptors or oss compatibility has not been considered ... in case you think they are a required before merge, other folks might need to check

my last suggestion would be to add some level of automated testing before merging