feathers icon indicating copy to clipboard operation
feathers copied to clipboard

Feathers auth client does not treat non-401 failures as failures

Open jnardone opened this issue 5 years ago • 3 comments

If you have an existing JWT, when an initial reAuthenticate call happens, if the referenced entity does not exist any more (e.g. deleted from users) then the feathers client does not handle this as a failure and no response is delivered to the caller.

We hit this in some tests, but it's easy enough if you either remove the test user from the users collection or supply a JWT with an invalid user ID/sub reference.

I can make this succeed if I change jwt.ts to:

-    const result = await entityService.get(id, omit(params, 'provider'));
+    try {
+      const result = await entityService.get(id, omit(params, 'provider'));
+    } catch(err) {
+      throw new NotAuthenticated(`Could not find entity`);
+    }

but I'm not advocating this as a specific approach. The client is clearly not looking for a 404 to come back, but that's what gets delivered back to the feathers authentication client.

Node 12.14 Feathers 4.4.3 (server and client)

jnardone avatar Jan 17 '20 18:01 jnardone

What i've found is the feathers auth client will not ever respond to an authentication request if the error that's returned is not a 401 (say, a 404). We had to put a server-side hook in to make sure that any non-401 errors from authentication are changed to 401 while this bug exists.

jnardone avatar Feb 04 '20 19:02 jnardone

Do you have an example to reproduce? I just tried throwing a different kind of error in the feathers-chat and the frontend catches it as expected.

daffl avatar Feb 05 '20 18:02 daffl

The way we see it happen in practice is when we try to authenticate a jwt with a sub/userId reference to a user that no longer exists in the users service. Generally we only hit this while testing.

I'll try to work up a smaller, self-contained example.

jnardone avatar Feb 05 '20 18:02 jnardone

I believe #2892 should fix this issue as well.

daffl avatar Nov 24 '22 23:11 daffl