Add support for authentication flows

[cmelchior]

Closes #749

Opening this PR with a few design decisions we need to agree on:

• In Realm Java, we only report loggedIn/loggedOut, but there also exists a REMOVED state (which is loggedOut with extra semantics). Should we also expose removed in this flow?
• Currently, I expose an AuthenticationChange with a AuthenticationChange.Type change. Is that a good approach? There exist two alternatives: 1) Instead reuse the User.State which has the same semantics 2) Our object notifications use sealed classes, should we do this instead? Which would also allow us to remove the Type enum? I'm probably slightly leaning toward this alternative.
• #namingIsHard, authenticationChangeAsFlow mirrors, kinda, the SyncSession.connectionStateAsFlow() as API, not sure if a better name exists?

Variant1:
            public data class AuthenticationChange(
                public val type: Type,
                public val user: User
            ) {
                public enum class Type {
                    LOGGED_IN,
                    LOGGED_OUT
                    REMOVED
                }
                public fun didLogIn(): Boolean = (type == Type.LOGGED_IN)
                public fun didLogOut(): Boolean = (type == Type.LOGGED_OUT)
            }
            app.authenticationChangeAsFlow().collect { change: AuthenticationChange ->
                // Usage 1: Use the type
                when(change.type) {
                    is Type.LOGGED_IN -> handleLogin(change.user)
                    is Type.LOGGED_OUT -> handleLogout(change.user)
                    is Type.REMOVED -> handleLogout(change.user)
                }
                // Usage 2: Use the methods
                if (change.didLogIn()) {
                    handleLogin(change.user)
                } else {
                    handleLogOut(change.user)
                }
            }

Variant 2:
            public sealed interface AuthenticationChange {
                public val user: User
                public fun didLogIn(): Boolean
                public fun didLogOut(): Boolean
            }
            public interface LoggedIn: AuthenticationChange
            public interface LoggedOut: AuthenticationChange
            public interface Removed: AuthenticationChange
           
            app.authenticationChangeAsFlow().collect { change: AuthenticationChange ->
                // Usage 1, use the sealed hiearchy
                when(change) {
                    is LoggedIn -> handleLogin(change.user)
                    is LoggedOut -> handleLogout(change.user)
                    is Removed -> handleLogout(change.user)
                }

                // Usage 2: Use the methods
                if (change.didLogIn()) {
                    handleLogin(change.user)
                } else {
                    handleLogOut(change.user)
                }
            }

EDIT: After discussion we went with this API:

public sealed interface AuthenticationChange {
    public val user: User
}

public interface LoggedIn: AuthenticationChange
public interface LoggedOut: AuthenticationChange
public interface Removed: AuthenticationChange

app.authenticationChangeAsFlow().collect { change: AuthenticationChange ->
    when(change) {
        is LoggedIn -> handleLogin(change.user)
        is LoggedOut -> handleLogout(change.user)
        is Removed -> handleRemove(change.user)
    }
}

[clementetb]

I think that I would use a sealed interface, as we do with the other change notification interfaces. I would also remove the boolean as the status is already defined by the type of instance received. Naming is hard, I cannot think on something better, maybe we could shorten it to AuthChange?

public sealed interface AuthChange {
    public val user: User
}

public interface LoggedIn: AuthChange
public interface LoggedOut: AuthChange
public interface Removed: AuthChange

app.authChangeAsFlow().collect { change: AuthChange ->
    when(change) {
        is LoggedIn -> handleLogin(change.user)
        is LoggedOut -> handleLogout(change.user)
        is Removed -> handleRemove(change.user)
    }
}

I would leave it as simple as possible, we could add the booleans later on if required.

[cmelchior]

PR should now reflect our discussion and is ready for review.

[rorbech]

I think I would have put the dispatching logic in one method

fun reportAuthenticationChange(event: AuthenticationChange) { ... }

and just create the events on the fly

reportAuthenticationChange(LoggedInImpl(user))

Instead of having separate methods for each. This would also make it easier to have common logic if we fail to send the events to the flow ... which I also think we would need, right? 🤔 (We have a utility checkForBufferOverFlow to handle this uniformly for channels, but don't know how to fit that in here)

[cmelchior]

I have merged the different methods.

We are emitting events inside a suspend function and the flow is suspending, so I don't think we need to use checkForBufferOverFlow?

[rorbech]

Hmm. I guess you are right that we are just suspending if the recipient is not ready ... But is what we want? Would be quite tricky to see that the login/logout is blocked due to the observer of the authencationChangeAsFlow()-flow being locked. Don't know if there is an easy way to channel the flow to follow the approach from our other flows. This is also way less active, so maybe just fine by adding some buffer space ... or is it then even less likely to spot if it doesn't happen on every emit and we don't throw if the buffer is full 🤔

[rorbech]

This is not really safe. Two threads calling this simultaneous would issue two events but should only emit one. I guess we would either need a return value from core or synchronize the operations for a use. Don't know if it is a critical issue though. Same for the other events.

[cmelchior]

Good catch. I already created realm/realm-core#6516 which will also fix this. I'll think and see if there is something we can do to prevent this, but if it is super tricky I would probably just ignore this case. I doubt this is code that will see race conditions anyway.

[cmelchior]

I reworked the logic a little bit. The chance for a race condition should be less now, although still present. My stance would probably be that using locks for this would be overkill and we should wait for realm/realm-core#6516 for a proper fix, but not sure what you think?

Also, worst case you will see two logouts in a row (or something like that), but given that we don't promise anything about the state of the User object either, I don't think that is a big problem?

[rorbech]

Sounds ok to leave it as is for now. Maybe include a link to the core issue in a comment above reportLoggedOut.

[clementetb]

Looks great, added some minor comments on documentation. Happy on green on CI.

[rorbech]

Maybe a bit pedantic, but I guess all APIs hitting the new authentication change updating-operations (login, logout, delete and remove) that in rare circumstances could throw due to buffer overrun needs a @throws IllegalStateException-doc entry.

Also some unaddressed leftover comments from Clemente

Otherwise LGTM