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

Add initial authentication storage #644

Merged
merged 45 commits into from
Dec 30, 2020

Conversation

nielsvanvelzen
Copy link
Member

@nielsvanvelzen nielsvanvelzen commented Nov 29, 2020

Changes

  • Store credentials
    • Uses Android's AccountManager to store access tokens and the ability to remove accounts from within the system settings
  • Display all servers from: storage, server discovery, legacy credentials
  • Display all users from: public user endpoint, storage, legacy credentials
  • Display errors when adding a server/user (rough implementation)
  • Sign in
  • Backup Jellyfin app settings and credentials (without access tokens)

Future pull requests

  • Cache & show profile pictures
  • Remember last user

TODO

  • Remove accidently committed dev note files
  • Squash all commits
  • Rewrite legacy credential migration code
  • Clean up all code

Issues
Part of #612

Dev notes

The AlertFragment uses callbacks passed via the constructor. This is not a good idea for 2 reasons

  1. Fragment's should not have custom constructors per Android guidelines, this StackOverflow article explains it a bit.
  2. In this case the callbacks came from another fragment which was unloaded at the time the callback executed. This caused issues were the viewmodel was unavailable and crashed the app.

We should aim for fragments that are not dependent on other fragments/activies. All UI should only interact via viewmodels with repositories/services/etc. and data should be passed via Bundles.

Other

I decided to use UUID everywhere since this is what the new apiclient does too. In order to make things work I added a few helper methods to convert string -> uuid that support the "simple" format (which doesn't use hyphens). Because those UUID's differ from the strings in the old credentials I needed to convert strings -> uuid -> string a bunch of times. This will all be solved when we remove the legacy credential support and upgrade to the new apiclient.

I've also decided to use Kotlin's flow internally and convert it to Android's LiveData at the end of the pipeline because a flow is much easier to work with and provides a more rich API. I added a helper method for flows too to collect them to a LiveData<List<T>>.

@nielsvanvelzen
Copy link
Member Author

This PR became quite massive and I'm a bit lost in what still needs to happen etc. so I did some final cleaning and marking it ready or review.

I think there's still some small bugs which I'd say are easier to fix in future PR's instead of this one, but let me know what you think about that @thornbill.

@nielsvanvelzen nielsvanvelzen marked this pull request as ready for review December 12, 2020 12:55
@nielsvanvelzen nielsvanvelzen changed the title [WIP] Add credential storage Add initial credential storage Dec 12, 2020
@nielsvanvelzen nielsvanvelzen changed the title Add initial credential storage Add initial authentication storage Dec 12, 2020
@thornbill
Copy link
Member

This is crashing on launch for me on a Fire Stick:

java.lang.IllegalStateException: Flow invariant is violated:
		Flow was collected in [StandaloneCoroutine{Active}@c69d5fa, Main [immediate]],
		but emission happened in [DispatchedCoroutine{Active}@19e77ab, LimitingDispatcher@32313e08[dispatcher = DefaultDispatcher]].
		Please refer to 'flow' documentation or use 'flowOn' instead
	at kotlinx.coroutines.flow.internal.SafeCollector_commonKt.checkContext(SafeCollector.common.kt:84)
	at kotlinx.coroutines.flow.internal.SafeCollector.checkContext(SafeCollector.kt:82)
	at kotlinx.coroutines.flow.internal.SafeCollector.emit(SafeCollector.kt:68)
	at kotlinx.coroutines.flow.internal.SafeCollector.emit(SafeCollector.kt:54)
	at org.jellyfin.androidtv.auth.ServerRepositoryImpl$getDiscoveryServers$1$1$invokeSuspend$$inlined$map$1$2.emit(Collect.kt:139)
	at kotlinx.coroutines.flow.internal.SafeCollectorKt$emitFun$1.invoke(SafeCollector.kt:14)
	at kotlinx.coroutines.flow.internal.SafeCollectorKt$emitFun$1.invoke(SafeCollector.kt)
	at kotlinx.coroutines.flow.internal.SafeCollector.emit(SafeCollector.kt:71)
	at kotlinx.coroutines.flow.internal.SafeCollector.emit(SafeCollector.kt:54)
	at org.jellyfin.apiclient.discovery.ServerDiscovery$discover$1.invokeSuspend(ServerDiscovery.kt:111)
	at org.jellyfin.apiclient.discovery.ServerDiscovery$discover$1.invoke(ServerDiscovery.kt)
	at kotlinx.coroutines.flow.SafeFlow.collect(Builders.kt:56)
	at org.jellyfin.androidtv.auth.ServerRepositoryImpl$getDiscoveryServers$1$1$invokeSuspend$$inlined$map$1.collect(SafeCollector.common.kt:114)
	at org.jellyfin.androidtv.auth.ServerRepositoryImpl$getDiscoveryServers$1$1.invokeSuspend(ServerRepository.kt:104)
	at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
	at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:56)
	at kotlinx.coroutines.scheduling.CoroutineScheduler.runSafely(CoroutineScheduler.kt:571)
	at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.executeTask(CoroutineScheduler.kt:738)
	at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.runWorker(CoroutineScheduler.kt:678)
	at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.run(CoroutineScheduler.kt:665)

@nielsvanvelzen
Copy link
Member Author

Discovery crash should be fixed now.

Used this source (which explains flows and coroutines quite well). Basically I did an illegal operation by using withContext in the flow. I should've applied the scope to the flow by using flowOn instead.

@nielsvanvelzen
Copy link
Member Author

Bug found:

  • Login using auto discovery doesn't work - when authenticating the server does not exists yet and thus the address can not be found.
    • Fix for now is to manually add the server

@thornbill
Copy link
Member

thornbill commented Dec 17, 2020

I've found a couple things so far:

  1. Discovered server is displayed before manually added server
  2. Saved user listed twice if it's also a public user
  3. Passwordless login no longer bypasses the login form

@thornbill
Copy link
Member

The tests are failing due to a reference to some old code

e: /home/vsts/work/1/s/app/src/test/java/org/jellyfin/androidtv/util/apiclient/ModelExtensionsTest.kt: (45, 5): Unresolved reference: toUser

@nielsvanvelzen
Copy link
Member Author

nielsvanvelzen commented Dec 24, 2020

I realized that I accidently pushed some changes I was working on for the CardPresenter to optimize it. But since those changes were ready for review I'll leave them in.

edit: I did actually find an issue with it, the fallback tiles were missing their colors. So I removed my changes.

@thornbill
Copy link
Member

Just tested this again in an emulator and noticed the following issues:

  1. Passwordless login of public users no longer bypasses the login form
  2. Adding multiple servers seems to not work only my saved server is ever displayed.

@nielsvanvelzen
Copy link
Member Author

Passwordless login of public users no longer bypasses the login form

I just realized it only works for users not saved. I'll fix that.

Adding multiple servers seems to not work only my saved server is ever displayed.

Did you restart the app? There is a bug where new servers only show up after restart. The fix for that one will be in another PR because I expect it to be quite large.

We now remember if a user has a password (using a boolean) in the offline data set. If a user fails to authenticate because a password was added it will prompt for a password. If it's the other way around the user can just proceed in the password-prompt with an empty password and the boolean will be updated for future logins.
@nielsvanvelzen
Copy link
Member Author

copied from commit message

We now remember if a user has a password (using a boolean) in the offline data set. If a user fails to authenticate because a password was added it will prompt for a password. If it's the other way around the user can just proceed in the password-prompt with an empty password and the boolean will be updated for future logins.

@thornbill
Copy link
Member

I still seem to be randomly prompted for a password when logging in and out with a passwordless user. This can probably be addressed later though. Let's get this in once the conflicts are resolved. 👍

# Conflicts:
#	app/src/main/java/org/jellyfin/androidtv/JellyfinApplication.kt
#	app/src/main/java/org/jellyfin/androidtv/TvApp.java
#	app/src/main/java/org/jellyfin/androidtv/di/AppModule.kt
#	app/src/main/java/org/jellyfin/androidtv/ui/home/HomeFragment.java
#	app/src/main/java/org/jellyfin/androidtv/ui/preference/category/authentication.kt
@thornbill thornbill merged commit d3fc28d into jellyfin:master Dec 30, 2020
@nielsvanvelzen nielsvanvelzen deleted the accountmanager branch December 30, 2020 22:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants