-
Notifications
You must be signed in to change notification settings - Fork 3
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
Switch to JWT based auth #48
Conversation
I like this! Perhaps the fetch-wrapping methods could catch the timeouted token, and then transparently do a refresh and retry the fetch. I think this would be the best way. It would just cause some additional delay here and there, but that would be it. I think we use the isAuthed check to determine whether to show the users the account pages or whether to show a login form there. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all the work! I have some small nits.
@@ -62,11 +60,7 @@ const MenuBar: React.FC<MenuBarProps> = ({ | |||
</button> | |||
|
|||
<button className="btn outline" onClick={onLoginButtonClick}> | |||
{loginStatusKnown |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that we always synchronously know the login state now.
src/model/stations.ts
Outdated
const url = hasTokens() ? JWT_ALL_STATIONS_URL : APP_ALL_STATIONS_URL; | ||
return fetchJsonEnsureOk(url, undefined, 5, hasTokens()).then((stations) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain why we need to use both URLs here? I think these endpoints don't provide user-specific data and thus don't require auth. So I think we don't ever need to use the JWT routes here and we can remove this complexity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree this is complexity, but there might be additional information available in authed routes in the future, which would mean this change would be relevant.
Another point is, now Velocity knows which user has requested the data, with possible exemptions from rate limiting or other preferential request handling. If and when such measures might be implemented by Velocity is questionable, but this would future-proof the application for such changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For authed routes I would like to provide a second function, however, such that the guarantees are clear and you know exactly which data the function will be giving you. And, in their API they very clearly separated authed routes from unauthed routes, and make you do the matching itself (stations endpoint gives data you can use with the booking endpoint, for example).
And in regards to the rate-limiting: While theoretically yes, I doubt Velocity will take the user accounts into account for any rate-limiting they do. I doubt they even rate-limit at all from my past experiences. So I'd prefer to remove this complexity and change in behavior that's dependent on global state from the function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have changed the default behaviour back to using unauthenticated routes, and provided functions which call the authenticated counterparts for use in the future.
In preparation for supporting smart-locks, whose API is only available in JWT based routes.