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

[iOS] Address Bad UX on Password Change #1431

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

vantimothy10
Copy link

@vantimothy10 vantimothy10 commented Feb 11, 2025

Summary

My contribution here, would appreciate any feedback.

Resolves: #1413

Previously a user would enter a 'retry' loop when attempting to log into an account that has had its password changed. This fix seeks to provide the user clearer verbiage and add an exit to the flow. This is done by catching APIError(401) inside the ServerCheckViewModel and using it to trigger a new view state in ServerCheckView

Outstanding Items

Would specific feedback on:

  • The specific verbiage on the new screen
  • How to localize the new verbiage
  • Potential edge cases I could be missing

Screenshots

New screen

Example flow gif

@JPKribs JPKribs linked an issue Feb 11, 2025 that may be closed by this pull request
@JPKribs JPKribs added the bug Something isn't working label Feb 11, 2025
@JPKribs
Copy link
Member

JPKribs commented Feb 11, 2025

I don't want to claim to be the authority in this area but I have a couple of thoughts:

  1. 401 doesn't always necessarily mean that the AccessToken is invalid. I would say it's the most likely cause, but there is a chance that is this error is being thrown from somewhere that isn't Jellyfin. An example of this would be a Reverse Proxy. For this reason, I think should have an option to refresh button that attempts to reload the view like the standard errorView and then add a .destructive button for Re-authenticate. That way, if the 401 is generated from another source, the user can choose whether to reset their account on Swiftfin or not.
  2. Since this is logging the user out, the button should have a .confirmationDialog prior to proceeding. This way, someone doesn't accidentally log themselves out. Adding an extra confirmation will prevent some heartache.
  3. Instead of logging the user out, we should add a function to the UserSignInViewModel for reAuthenticateUser. I made something for this on another branch I could share but the idea would be that we take the existing username and a new password. Then, use that with the SignIn function to generate a new UserState and use that to update the existing entry. This way, all customizations and settings set in Swiftfin are not lost when you change your password. Reauthenticating will allow you to replace the token while leaving the rest of the UserState intact.

Localization for any strings can be found in the Translations folder. If you make the item in English, it can be called from code using L10n.name. For example: L10n.accessSchedules returns Access Schedules:

Screenshot 2025-02-10 at 21 26 01

Let me know if you need any help on the items above! As I mentioned, I'm definitely not the authority so if @LePips has any other insight/direction, his advice is absolutely the correct direction to take this! But, I think the 3 items above would help flesh this out a bit more and make this more well rounded experience for these cases.

@vantimothy10
Copy link
Author

@JPKribs

After some analysis it turns out that setting a new accessToken for a UserState that was already in the dataStack persists the change to the keychain automatically and by extension to the dataStack. To get around this, ive made changes to make the getExistingUser() check BEFORE we set the access token. This changes a bunch of the logic of the sign in flow but I think its necessary facilitate future flows around duplicate users.

I am also not super familiar with the navigation pattern used here. I am sure there are a couple mistakes with my approach so would appreciate any guidance!

@LePips
Copy link
Member

LePips commented Feb 14, 2025

Thanks for the UX consideration!

An implementation to fix this flow would consist of 2 parts: duplicate user sign in and what to navigate to/properly do when presented with a 401.

In UserSignInViewModel and UserSignInView, we should already have the foundations in place to allow overriding local user access tokens when signed in again. Although, I really wish past me detailed what issues need fixing.

The main navigation ideas are to:
1 - navigate to UserSignInView
- this allows signing into any user that isn't the current user we are expecting to sign into. To make that flow better it would require more work that I don't think would be best.
2 - present a smaller modal view to just sign into the current user
- this could possibly re-use UserSignInViewModel where we just present 2 text fields: username that's locked to the current user and the password. This could also allow QuickConnect, but there would have to be additional work to verify the quick-connected user is the current user.
- makes more views and logic I don't want to maintain
3 - just "fix" the current duplicate user sign in issues and have the error message tell the user to sign in again through the normal flow.

I don't consider 401 that prevalent of an issue to justify 1 and 2, and think we can just go with 3 by seeing what isn't working.

@vantimothy10
Copy link
Author

Apologies @JPKribs and @LePips, I just realized I didnt push my changes from last night. I added some mechanisms to navigate back to the sign-in view from server check. I also re-worked the sign-in flow to better handle the case of a UserState still existing. It seems like under the hood, the duplicate user flow was updating the accessToken anyways. The confirmation dialog was the only thing preventing the user from progressing.

Please let me know if im on track with all of this. I would specifically appreciate guidance on how to leverage Stinsen. I was having some trouble searching for views in the navigation stack in order to dismiss modals. Thanks

@JPKribs
Copy link
Member

JPKribs commented Feb 14, 2025

Apologies @JPKribs and @LePips, I just realized I didnt push my changes from last night.

No worries at all! I am traveling this week so I apologies if I don't respond. I only have element on my computer so I'm slower on Matrix than I am on Github.

For UI, this is what I originally thinking. Please bear with me for the filler text. I don't have a great idea of the wording that makes the most sense. Personally, I like the idea that we just retain the API error but just make a note under. This way, we're not obscuring 401 from those who want that information while providing a set of directions for those who want that.

Retry + Re-authenticate
Simulator.Screen.Recording.-.iPhone.16.Pro.-.2025-02-13.at.20.46.42.mp4

This is what I did to make this work. Feel free to use any of it that you like:

//
// Swiftfin is subject to the terms of the Mozilla Public
// License, v2.0. If a copy of the MPL was not distributed with this
// file, you can obtain one at https://mozilla.org/MPL/2.0/.
//
// Copyright (c) 2025 Jellyfin & Jellyfin Contributors
//

import SwiftUI

struct ServerCheckView: View {

    @EnvironmentObject
    private var router: MainCoordinator.Router

    @StateObject
    private var viewModel = ServerCheckViewModel()

    @StateObject
    private var userViewModel = SettingsViewModel()

    @State
    private var isPresentingSignOut: Bool = false

    @ViewBuilder
    private func errorView<E: Error>(_ error: E) -> some View {
        VStack(spacing: 10) {
            Image(systemName: "xmark.circle.fill")
                .font(.system(size: 72))
                .foregroundColor(Color.red)

            Text(viewModel.userSession.server.name)
                .fontWeight(.semibold)
                .foregroundStyle(.secondary)

            Text(error.localizedDescription)
                .frame(minWidth: 50, maxWidth: 240)
                .multilineTextAlignment(.center)

            // TODO: More accurate way of getting 401.
            if error.localizedDescription.contains("401") {
                // TODO: Better description & localize
                Text("This error often means that your Jellyfin Access Token something something sign out and sign back in.")
                    .frame(minWidth: 50, maxWidth: 240)
                    .multilineTextAlignment(.center)
            }

            PrimaryButton(title: L10n.retry)
                .onSelect {
                    viewModel.send(.checkServer)
                }
                .frame(maxWidth: 300)
                .frame(height: 50)

            // TODO: More accurate way of getting 401.
            if error.localizedDescription.contains("401") {
                PrimaryButton(title: "Reauthenticate", role: .destructive)
                    .onSelect {
                        isPresentingSignOut = true
                    }
                    .frame(maxWidth: 300)
                    .frame(height: 50)
            }
        }
    }

    var body: some View {
        ZStack {
            switch viewModel.state {
            case .initial, .connecting, .connected:
                ZStack {
                    Color.clear

                    ProgressView()
                }
            case let .error(error):
                errorView(error)
            }
        }
        .animation(.linear(duration: 0.1), value: viewModel.state)
        .onFirstAppear {
            viewModel.send(.checkServer)
        }
        // TODO: Localize & better message
        .confirmationDialog(
            "Sign Out",
            isPresented: $isPresentingSignOut,
            titleVisibility: .visible
        ) {
            Button("Sign Out", role: .destructive) {
                userViewModel.signOut()
            }
            Button(L10n.cancel, role: .cancel) {
                isPresentingSignOut = false
            }
        } message: {
            Text("Are you sure?")
        }
        .onReceive(viewModel.$state) { newState in
            if newState == .connected {
                withAnimation(.linear(duration: 0.1)) {
                    let _ = router.root(\.mainTab)
                }
            }
        }
        .topBarTrailing {

            SettingsBarButton(
                server: viewModel.userSession.server,
                user: viewModel.userSession.user
            ) {
                router.route(to: \.settings)
            }
        }
    }
}

For that formatting, I did update PrimaryButton to reflect the destructive from ListRowButton:

//
// Swiftfin is subject to the terms of the Mozilla Public
// License, v2.0. If a copy of the MPL was not distributed with this
// file, you can obtain one at https://mozilla.org/MPL/2.0/.
//
// Copyright (c) 2025 Jellyfin & Jellyfin Contributors
//

import Defaults
import SwiftUI

struct PrimaryButton: View {

    @Default(.accentColor)
    private var accentColor

    private let title: String
    private let role: ButtonRole?
    private var onSelect: () -> Void

    var body: some View {
        Button {
            onSelect()
        } label: {
            ZStack {
                Rectangle()
                    .foregroundColor(role == .destructive ? Color.red.opacity(0.2) : accentColor)
                    .frame(maxWidth: 400)
                    .frame(height: 50)
                    .cornerRadius(10)

                Text(title)
                    .fontWeight(.bold)
                    .foregroundColor(role == .destructive ? Color.red : accentColor.overlayColor)
            }
        }
    }
}

extension PrimaryButton {

    init(title: String, role: ButtonRole? = nil) {
        self.init(
            title: title,
            role: role,
            onSelect: {}
        )
    }

    func onSelect(_ action: @escaping () -> Void) -> Self {
        copy(modifying: \.onSelect, with: action)
    }
}

This should go to SWITCH users instead of fulling signing them out. From there, adding a context menu above Delete might be a good route:

Screenshot 2025-02-13 at 20 44 21

This would just route them back to the sign in page but pre-set the username to their previous user. Then, just regular signup but replace the AccessToken.

Again, anything @LePips says trumps what I am saying here but this is how I would approach it since I think we can just hook into all the existing login logic and the ServerCheckView changes above are pretty minimally intrusive. IMO, that .confirmationDialog can actually probably be skipped since we're not logging them out just doing the Switch Users logic.

@vantimothy10
Copy link
Author

vantimothy10 commented Feb 14, 2025

Video demo
demo.mp4

Just pushed up a change to make the flow a bit simpler navigation-wise. I could add some more enhancements like the update button on long press and a flow to pre-populate the username but I did want to get your guys opinion on the current mechanics, first. Thanks!

@JPKribs
Copy link
Member

JPKribs commented Feb 14, 2025

That seems good for flow! My only big notes are UI nitpicks which I know you're still working on.

Only functional change, I might just have the replace token still pop up a confirmation dialog first. Just say "This will update your password for user. Are you sure you want to proceed?" That should both work for duplicate user and updating a password. Other than that, flow looks great IMO!


For UI nitpicks, feel free to take/ignore any of the below!

  • I thinking having "Back" as destructive in style would help it stand out. Chatting with my wife, she said she said as a non-tech person, she likely wouldn't even read the message on first go so having the separation makes the "Back" stand out as different than "Retry" (You can pull this from the previous PrimaryButton change I provided.)
  • I might replace the "Username :..." message with "401:...". That way, those who know/care can still see the number it originated from.
  • The text should imply the invalid token but we can't know that for sure just from 401. So like "401: Your connection may be expired. You may need to update your password to proceed" or something like that. Just since 401 could mean something else as well so updating the password isn't a guaranteed fix. IMO, providing the "May be expired" helps for troubleshooting since if they try resetting and it doesn't work that indicates maybe it's something else. Just how my head works so maybe that doesn't make the same sense to everyone else.

Nothing that big, and I know you had some work in mind there so feel free to ignore any of those you don't need!

Great work so far!

@vantimothy10
Copy link
Author

Just pushed up the UI updates. Let me know what you think! Here is the updated flow:

demo
demo-update-style.mp4

@JPKribs
Copy link
Member

JPKribs commented Feb 14, 2025

Just pushed up the UI updates. Let me know what you think! Here is the updated flow:

IMO, this looks great! Only change I would change "Back" to "Switch User" using L10n.switchUser. Just since we have the label for it and it more accurately mirrors the action being performed. "Back" works if we come from the login screen each time but if you are already logged into the account when you enter the app, "Back" doesn't mean as much. It's a really tiny item so I'm not that worried about it but just while we're looking at it I thought I would say.

Other than that, I think this looks solid! .confirmationDialog on the login screen when duplicate users would be my only functional change. Something like:

Screenshot 2025-02-14 at 14 44 53

This is just using this on tvOS

        .confirmationDialog(
            Text(L10n.updatePassword),
            isPresented: $isPresentingDuplicateUser,
            presenting: duplicateUser
        ) { _ in

            Button(L10n.update, role: .destructive) {
                viewModel.send(.signIn(username: username, password: password, policy: .none))
            }
        } message: { duplicateUser in
            Text("This user is already logged in. Would you like to update the password for \(duplicateUser.username)")
        }

So, I think the route we would want is to have it go through the regular signIn process. If that produces a duplicateUser event, then catch that event with a .confirmationDialog and re-send the signIn with the override. That way, we're at least informing the user what is going on. I don't see that as being too dire but the one specific scenario I can think of is (1364)[https://github.com//issues/1364]. I have had this issue myself. If you duplicate your Jellyfin Server, both servers will treat themselves as the same Server since their ServerId will be the same. So, if I log into Swiftfin with both servers on the same user, the AccessToken will be different between the two servers but the users will be "duplicates" since they are saved with the same ServerId & username. Without specifying that we're updating an existing user, people in this scenario will have that confusion that they're just replacing the same user's AccessToken instead of creating 2 users, 1 from each server. IMO that's a niche scenario, but it looks like the login flow @LePips originally created there used this mechanism so it'd just be reactivating what is there and making sure the "Replace" or "UpdatePassword" function runs through your updated logic to replace the Token.

Other than those 2 items, this looks great!

Let me know if you have any questions! We're getting to some weird edge cases so overall what you have is solid but I just want to make sure that the steps have feedback so users who run into these weird cases have some visibility into what is going on.

Copy link
Member

@LePips LePips left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've only briefly played around with this, but there seems to be a bug in the simulator where:

1 - 401 user server check
2 - on select user, select 401 user

the app hangs. I haven't been able to test on device but this still shouldn't be happening and seems new to this PR.

@@ -215,7 +181,7 @@ final class UserSignInViewModel: ViewModel, Eventful, Stateful {
}
}

private func signIn(username: String, password: String, policy: UserAccessPolicy) async throws -> UserState {
private func makeSignInRequest(username: String, password: String) async throws -> AuthenticationResult {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't need a separate function for this, just inline in the one place it's used. Also, this would be returning the result, not the actual request.

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

Successfully merging this pull request may close these issues.

401 error after password change No login promt after manually removing device from web admin ui
3 participants