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

Username changes are not reflected in UI #375

Closed
jimmycuadra opened this issue Jan 16, 2022 · 6 comments · Fixed by #437
Closed

Username changes are not reflected in UI #375

jimmycuadra opened this issue Jan 16, 2022 · 6 comments · Fixed by #437
Labels
P-high High priority item T-bug Something isn't working the way it is supposed to

Comments

@jimmycuadra
Copy link
Contributor

Describe the bug
When you change your username, subsequent requests fail and the old username is displayed.

To Reproduce

  1. Start brand new install of Dim with a fresh database
  2. Register an admin account
  3. Go to "Preferences > Profile > Username" and change username.
  4. A PATCH request is made to /api/v1/auth/username and it returns status code 200.
  5. Running SELECT * FROM users; in sqlite shows the new username.
  6. A GET request is made to /api/v1/auth/whoami and it returns status code 200. The response body contains the old username, e.g.:
    {"picture":null,"roles":["owner"],"spentWatching":0,"username":"oldusername"}
  7. Refreshing the page and clicking "Profile" shows the old username in the UI.
  8. Two GET requests are made to /api/v1/user/settings and both return status code 500 with this body:
    {"error":"DatabaseError","messsage":"A database error occured: DatabaseError(RowNotFound)"}
  9. Sticking some dbg! statements in dim::routes::settings::get_user_settings and hitting /api/v1/user/settings via curl reveals this error in the server logs:
    [dim/src/routes/settings.rs] &User::get(&mut tx, &user.0.claims.get_user()).await = Err(
        DatabaseError(
            RowNotFound,
        ),
    )
    
    Perhaps the authentication token used by the client is not being updated after changing usernames?

Expected behavior
The new username should be reflect in the UI after reloading the page, and API requests should not return status code 500.

Device and browser including versions:
Firefox 95.0.2.

@jimmycuadra jimmycuadra added S-needs-triage Status: Needs triage T-bug Something isn't working the way it is supposed to labels Jan 16, 2022
@vgarleanu
Copy link
Member

Yeah I think the issue is that the Auth token contains the username and we use it for subsequent database fetches. And we can't really update the username in the token, we'd have to somehow tell the clients that the token needs to be updated.

I think the best way to solve this is to instead add a new user ID row to the users table and use that as the primary key. The user id will never change thus it is safe to store in the token.

@jimmycuadra
Copy link
Contributor Author

jimmycuadra commented Jan 23, 2022

On the subject, what is the reason Dim uses JWT instead of a simple session ID stored in a cookie? Everything I've read on the subject suggests that the use cases for JWT are rare and they are not appropriate for managing session data. But I haven't looked very closely at how Dim's authentication system works, so maybe it needs something standard sessions don't?

@vgarleanu
Copy link
Member

On the subject, what is the reason Dim uses JWT instead of a simple session ID stored in a cookie? Everything I've read on the subject suggests that the use cases for JWT are rare and they are not appropriate for managing session data. But I haven't looked very closely at how Dim's authentication system works, so maybe it needs something standard sessions don't?

The main reasoning is that its a lot simpler to implement, most of the ground work has already been done for us in various JWT libs. We also initially didnt plan on adding very granular user management to dim as we didnt see it as necessary and that went hand in hand with us not wanting to touch the database too much. Looking back this isnt ideal and can cause bugs like these.

I havent looked into session-ids too much but Ill give it another look. Another option is to use oauth here but that might be overkill.

One way to solve this bug would be to re-issue a token when user information changes but this will require clients to be careful about how they manage their tokens and when they should update them.

@vgarleanu vgarleanu added P-high High priority item and removed S-needs-triage Status: Needs triage labels Jan 24, 2022
@jimmycuadra
Copy link
Contributor Author

jimmycuadra commented Jan 27, 2022

Thanks for the context! It sounds like replacing JWT with an encrypted user ID (stored on the client in a cookie) would be a good move. The server should be the source of truth for all data—the only thing it needs from the client is proof of who the client is. By adding additional information to the client, you end up having expiry issues like the one this issue is about. This is why JWT are not recommended for sessions. I believe their main use case is for very short-lived, one-use operations. There are lots of resources on the web that talk about this. One that captures what I'm saying and says it more eloquently is this: http://cryto.net/~joepie91/blog/2016/06/13/stop-using-jwt-for-sessions/

@cobyge
Copy link
Contributor

cobyge commented Apr 30, 2022

@vgarleanu mind if I try to handle this?
Currently I'm thinking of replacing JWT with an encrypted cookie using the same strategy as the private feature of cookie (Specifically this part) where the value of the cookie will be the encrypted User ID, which will then be used to get information about the user whenever needed.

@vgarleanu
Copy link
Member

@cobyge PRs are always welcome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-high High priority item T-bug Something isn't working the way it is supposed to
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants