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

Infinite loop in geAuth #1252

Open
1 task done
koush opened this issue Jul 4, 2023 · 5 comments
Open
1 task done

Infinite loop in geAuth #1252

koush opened this issue Jul 4, 2023 · 5 comments
Labels
bug Something isn't working

Comments

@koush
Copy link
Contributor

koush commented Jul 4, 2023

Is there an existing issue for this?

  • I have searched the existing issues

Describe The Bug

https://github.com/dgreif/ring/blob/main/packages/ring-client-api/rest-client.ts#L268

Doesn't this line cause a recursive request loop on a service or connection failure?

To Reproduce

No response

Expected behavior

N/A

Relevant log output

No response

Screenshots

No response

Additional context

No response

OS

All

Node.js Version

All

NPM Version

N/A

ring-client-api

11

Operating System

All

@koush koush added the bug Something isn't working label Jul 4, 2023
@koush
Copy link
Contributor Author

koush commented Jul 4, 2023

Nevermind, I was having a hard time following the code. authConfig gets cleared. It does seem like there's an issue here where any service/connection failure clears the auth.

@tsightler
Copy link
Collaborator

I think this is a duplicate of my issue here: #1071

@koush
Copy link
Contributor Author

koush commented Jul 5, 2023

@tsightler yeah, I am not sure why the refresh token is ever cleared. If the client enters a auth failure state, the failures should continue with the last known refresh token, which may recover. But as of now it clears the refresh token and guarantees it will never recover.

@tsightler
Copy link
Collaborator

I think it's just because the same code path is used for the initial authentication so, for example, if there is a configured token, but that saved token isn't valid, it's cleared so the same code path can be used to re-authenticate via standard 2FA auth. That's actually fine for the case where the token truly is invalid (for example, user removes account from authorized devices), but it's not good for the case where the token is only temporarily not working, which is the far more common case, in my experience.

@koush
Copy link
Contributor Author

koush commented Jul 5, 2023

Yeah, gonna try this and see if it works better. I don't think the rest client should be responsible for refresh token clearing. That requires user interaction, and it happens silently now. koush@4e95093

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants