Commit f1869931 authored by Grisha Kruglov's avatar Grisha Kruglov
Browse files

Post: address review feedback and test/lint failures

parent 29b118c6
......@@ -23,6 +23,7 @@ class AuthException(type: AuthExceptionType, cause: Exception? = null) : Throwab
/**
* An object that represents a login flow initiated by [OAuthAccount].
* @property state OAuth state parameter, identifying a specific authentication flow.
* This string is randomly generated during [OAuthAccount.beginOAuthFlowAsync] and [OAuthAccount.beginPairingFlowAsync].
* @property url Url which needs to be loaded to go through the authentication flow identified by [state].
*/
data class AuthFlowUrl(val state: String, val url: String)
......
......@@ -16,6 +16,8 @@ port.onMessage.addListener((event) => {
}));
});
// We should use pagehide/pageshow events instead, to better handle page navigation events.
// See https://github.com/mozilla-mobile/android-components/issues/2984.
window.addEventListener("unload", (event) => { port.disconnect() }, false);
/*
......
......@@ -20,7 +20,6 @@ import mozilla.components.service.fxa.FxaAuthData
import mozilla.components.service.fxa.SyncEngine
import mozilla.components.service.fxa.manager.FxaAccountManager
import mozilla.components.service.fxa.toAuthType
import mozilla.components.service.fxa.toNativeString
import mozilla.components.support.base.feature.LifecycleAwareFeature
import mozilla.components.support.base.log.logger.Logger
import org.json.JSONArray
......@@ -34,6 +33,8 @@ import java.util.WeakHashMap
* For more information https://github.com/mozilla/fxa/blob/master/packages/fxa-content-server/docs/relier-communication-protocols/fx-webchannel.md
* This feature uses a web extension to communicate with FxA Web Content.
*
* Boilerplate around installing and communicating with the extension will be cleaned up as part https://github.com/mozilla-mobile/android-components/issues/4297.
*
* @property context a reference to the context.
* @property customTabSessionId optional custom tab session ID, if feature is being used with a custom tab.
* @property engine a reference to application's browser engine.
......@@ -41,7 +42,7 @@ import java.util.WeakHashMap
* @property accountManager a reference to application's [FxaAccountManager].
*/
@Suppress("TooManyFunctions")
class WebChannelFeature(
class FxaWebChannelFeature(
private val context: Context,
private val customTabSessionId: String?,
private val engine: Engine,
......@@ -74,12 +75,12 @@ class WebChannelFeature(
/* ktlint-disable no-multi-spaces */
/**
* Communication channel is established from fxa-web-content to this class via webextension, as follows:
* [fxa-web-content] <--js events--> [fxawebchannel.js webextension] <--port messages--> [WebChannelFeature]
* [fxa-web-content] <--js events--> [fxawebchannel.js webextension] <--port messages--> [FxaWebChannelFeature]
*
* Overall message flow, as implemented by this class, is documented below. For detailed message descriptions, see:
* https://github.com/mozilla/fxa/blob/master/packages/fxa-content-server/docs/relier-communication-protocols/fx-webchannel.md
*
* [fxa-web-channel] [WebChannelFeature] Notes:
* [fxa-web-channel] [FxaWebChannelFeature] Notes:
* loaded ------> | fxa web content loaded
* fxa-status ------> | web content requests account status & device capabilities
* | <------ fxa-status-response this class responds, based on state of [accountManager]
......@@ -269,7 +270,7 @@ class WebChannelFeature(
data.put("capabilities", JSONObject().also { capabilities ->
capabilities.put("engines", JSONArray().also { engines ->
accountManager.supportedSyncEngines()?.forEach { engine ->
engines.put(engine.toNativeString())
engines.put(engine.nativeName)
} ?: emptyArray<SyncEngine>()
})
})
......
......@@ -6,6 +6,7 @@
package mozilla.components.feature.sendtab
import mozilla.components.concept.sync.AuthType
import mozilla.components.feature.push.AutoPushFeature
import mozilla.components.feature.push.PushType
import mozilla.components.support.test.mock
......@@ -19,7 +20,7 @@ class AccountObserverTest {
val feature: AutoPushFeature = mock()
val observer = AccountObserver(feature)
observer.onAuthenticated(mock(), true)
observer.onAuthenticated(mock(), AuthType.Signin)
verify(feature).subscribeForType(PushType.Services)
......@@ -31,7 +32,7 @@ class AccountObserverTest {
val feature: AutoPushFeature = mock()
val observer = AccountObserver(null)
observer.onAuthenticated(mock(), true)
observer.onAuthenticated(mock(), AuthType.Signup)
observer.onLoggedOut()
verifyNoMoreInteractions(feature)
......@@ -42,7 +43,7 @@ class AccountObserverTest {
val feature: AutoPushFeature = mock()
val observer = AccountObserver(feature)
observer.onAuthenticated(mock(), false)
observer.onAuthenticated(mock(), AuthType.Existing)
verifyNoMoreInteractions(feature)
}
......
......@@ -44,9 +44,14 @@ data class SyncConfig(
/**
* Describes possible sync engines that device can support.
*/
enum class SyncEngine {
History,
Bookmarks,
Passwords,
* @property nativeName Internally, Rust SyncManager represents engines as strings. Forward-compatibility
* with new engines is one of the reasons for this. E.g. during any sync, an engine may appear that we
* do not know about. At the public API level, we expose a concrete [SyncEngine] type to allow for more
* robust integrations. We do not expose "unknown" engines via our public API, but do handle them
* internally (by persisting their enabled/disabled status).
*/
enum class SyncEngine(val nativeName: String) {
HISTORY("history"),
BOOKMARKS("bookmarks"),
PASSWORDS("passwords"),
}
......@@ -65,20 +65,3 @@ fun handleFxaExceptions(logger: Logger, operation: String, block: () -> Unit): B
true
})
}
/**
* Internally, Rust SyncManager represents engines as strings. Forward-compatibility with new engines
* is one of the reasons for this. E.g. during any sync, an engine may appear that we do not know about.
* At the public API level, we expose a concrete [SyncEngine] type to allow for more robust integrations.
* We do not expose "unknown" engines via our public API, but do handle them internally (by persisting
* their enabled/disabled status). Since unknown engines are not exposed to the application, this
* conversion only needs to go one-way.
*/
fun SyncEngine.toNativeString(): String {
return when (this) {
SyncEngine.History -> "history"
SyncEngine.Bookmarks -> "bookmarks"
SyncEngine.Passwords -> "passwords"
}
}
......@@ -45,11 +45,11 @@ interface SyncStatusObserver {
object GlobalSyncableStoreProvider {
private val stores: MutableMap<String, SyncableStore> = mutableMapOf()
fun configureStore(storePair: Pair<String, SyncableStore>) {
stores[storePair.first] = storePair.second
fun configureStore(storePair: Pair<SyncEngine, SyncableStore>) {
stores[storePair.first.nativeName] = storePair.second
}
fun getStore(name: String): SyncableStore? {
internal fun getStore(name: String): SyncableStore? {
return stores[name]
}
}
......
......@@ -25,7 +25,6 @@ import mozilla.components.service.fxa.SyncAuthInfoCache
import mozilla.components.service.fxa.SyncConfig
import mozilla.components.service.fxa.SyncEngine
import mozilla.components.service.fxa.manager.authErrorRegistry
import mozilla.components.service.fxa.toNativeString
import mozilla.components.support.base.log.logger.Logger
import mozilla.components.support.base.observer.Observable
import mozilla.components.support.base.observer.ObserverRegistry
......@@ -218,7 +217,7 @@ class WorkManagerSyncDispatcher(
private fun getWorkerData(): Data {
val dataBuilder = Data.Builder().putStringArray(
KEY_DATA_STORES, supportedEngines.map { it.toNativeString() }.toTypedArray()
KEY_DATA_STORES, supportedEngines.map { it.nativeName }.toTypedArray()
)
return dataBuilder.build()
......
......@@ -307,9 +307,9 @@ class FxaAccountManagerTest {
assertEquals(0, syncStatusObserver.onErrorCount)
// No periodic sync.
manager.setSyncConfigAsync(SyncConfig(setOf(SyncEngine.History))).await()
manager.setSyncConfigAsync(SyncConfig(setOf(SyncEngine.HISTORY))).await()
assertEquals(setOf(SyncEngine.History), manager.supportedSyncEngines())
assertEquals(setOf(SyncEngine.HISTORY), manager.supportedSyncEngines())
assertNotNull(latestSyncManager)
assertNotNull(latestSyncManager?.dispatcher)
assertNotNull(latestSyncManager?.dispatcher?.inner)
......@@ -318,9 +318,9 @@ class FxaAccountManagerTest {
verify(latestSyncManager!!.dispatcher.inner, times(1)).syncNow(anyBoolean(), anyBoolean())
// With periodic sync.
manager.setSyncConfigAsync(SyncConfig(setOf(SyncEngine.History, SyncEngine.Passwords), 60 * 1000L)).await()
manager.setSyncConfigAsync(SyncConfig(setOf(SyncEngine.HISTORY, SyncEngine.PASSWORDS), 60 * 1000L)).await()
assertEquals(setOf(SyncEngine.History, SyncEngine.Passwords), manager.supportedSyncEngines())
assertEquals(setOf(SyncEngine.HISTORY, SyncEngine.PASSWORDS), manager.supportedSyncEngines())
verify(latestSyncManager!!.dispatcher.inner, times(1)).startPeriodicSync(any(), anyLong())
verify(latestSyncManager!!.dispatcher.inner, never()).stopPeriodicSync()
verify(latestSyncManager!!.dispatcher.inner, times(1)).syncNow(anyBoolean(), anyBoolean())
......@@ -364,7 +364,7 @@ class FxaAccountManagerTest {
// With a sync config this time.
var latestSyncManager: TestSyncManager? = null
val syncConfig = SyncConfig(setOf(SyncEngine.History), syncPeriodInMinutes = 120L)
val syncConfig = SyncConfig(setOf(SyncEngine.HISTORY), syncPeriodInMinutes = 120L)
val manager = object : TestableFxaAccountManager(
context = testContext,
config = ServerConfig.release("dummyId", "http://auth-url/redirect"),
......@@ -433,7 +433,7 @@ class FxaAccountManagerTest {
// assertEquals(1, syncStatusObserver.onErrorCount)
// Turn off periodic syncing.
manager.setSyncConfigAsync(SyncConfig(setOf(SyncEngine.History))).await()
manager.setSyncConfigAsync(SyncConfig(setOf(SyncEngine.HISTORY))).await()
verify(latestSyncManager!!.dispatcher.inner, never()).startPeriodicSync(any(), anyLong())
verify(latestSyncManager!!.dispatcher.inner, never()).stopPeriodicSync()
......
......@@ -188,11 +188,4 @@ class UtilsKtTest {
throw FxaPanicException("dunno")
}
}
@Test
fun `sync engine native name conversions`() {
assertEquals("passwords", SyncEngine.Passwords.toNativeString())
assertEquals("history", SyncEngine.History.toNativeString())
assertEquals("bookmarks", SyncEngine.Bookmarks.toNativeString())
}
}
\ No newline at end of file
......@@ -49,6 +49,8 @@ permalink: /changelog/
* `SyncConfig`'s `syncableStores` has been renamed to `supportedEngines`, expressed via new enum type `SyncEngine`.
* `begin*` OAuthAccount methods now return an `AuthFlowUrl`, which encapsulates an OAuth state identifier.
* `AccountObserver:onAuthenticated` method now has `authType` parameter (instead of `newAccount`), which describes in detail what caused an authentication.
* `GlobalSyncableStoreProvider.configureStore` now takes a pair of `Pair<SyncEngine, SyncableStore>`, instead of allowing arbitrary string names for engines.
* `GlobalSyncableStoreProvider.getStore` is no longer part of the public API.
# 11.0.0
......
......@@ -80,7 +80,7 @@ open class MainActivity : AppCompatActivity(), LoginFragment.OnLoginCompleteList
)
return@launch
}
openWebView(url)
openWebView(url.url)
}
}
)
......@@ -90,7 +90,7 @@ open class MainActivity : AppCompatActivity(), LoginFragment.OnLoginCompleteList
findViewById<View>(R.id.buttonCustomTabs).setOnClickListener {
launch {
account.beginOAuthFlowAsync(scopes).await()?.let {
openTab(it)
openTab(it.url)
}
}
}
......@@ -98,7 +98,7 @@ open class MainActivity : AppCompatActivity(), LoginFragment.OnLoginCompleteList
findViewById<View>(R.id.buttonWebView).setOnClickListener {
launch {
account.beginOAuthFlowAsync(scopes).await()?.let {
openWebView(it)
openWebView(it.url)
}
}
}
......
......@@ -47,8 +47,9 @@ class LoginFragment : Fragment() {
val uri = Uri.parse(url)
val code = uri.getQueryParameter("code")
val state = uri.getQueryParameter("state")
if (code != null && state != null) {
listener?.onLoginComplete(code, state, this@LoginFragment)
val action = uri.getQueryParameter("action")
if (code != null && state != null && action != null) {
listener?.onLoginComplete(code, state, action, this@LoginFragment)
}
}
......@@ -89,7 +90,7 @@ class LoginFragment : Fragment() {
}
interface OnLoginCompleteListener {
fun onLoginComplete(code: String, state: String, fragment: LoginFragment)
fun onLoginComplete(code: String, state: String, action: String, fragment: LoginFragment)
}
companion object {
......
......@@ -17,6 +17,7 @@ import kotlinx.coroutines.Job
import kotlinx.coroutines.async
import kotlinx.coroutines.launch
import mozilla.components.concept.sync.AccountObserver
import mozilla.components.concept.sync.AuthType
import mozilla.components.concept.sync.DeviceType
import mozilla.components.concept.sync.OAuthAccount
import mozilla.components.concept.sync.Profile
......@@ -24,11 +25,13 @@ import mozilla.components.service.fxa.FirefoxAccount
import mozilla.components.lib.fetch.httpurlconnection.HttpURLConnectionClient
import mozilla.components.service.fxa.manager.FxaAccountManager
import mozilla.components.service.fxa.DeviceConfig
import mozilla.components.service.fxa.FxaAuthData
import mozilla.components.service.fxa.ServerConfig
import mozilla.components.service.fxa.SyncConfig
import mozilla.components.service.fxa.SyncEngine
import mozilla.components.service.fxa.sync.GlobalSyncableStoreProvider
import mozilla.components.service.fxa.sync.SyncStatusObserver
import mozilla.components.service.fxa.toAuthType
import mozilla.components.service.sync.logins.AsyncLoginsStorageAdapter
import mozilla.components.service.sync.logins.SyncableLoginsStore
import mozilla.components.support.rusthttp.RustHttpConfig
......@@ -42,18 +45,7 @@ const val CLIENT_ID = "3c49430b43dfba77"
const val REDIRECT_URL = "https://accounts.firefox.com/oauth/success/$CLIENT_ID"
class MainActivity : AppCompatActivity(), LoginFragment.OnLoginCompleteListener, CoroutineScope, SyncStatusObserver {
private val loginsStorage by lazy {
SyncableLoginsStore(
AsyncLoginsStorageAdapter.forDatabase(File(this.filesDir, "logins.sqlite").canonicalPath)
) {
CompletableDeferred("my-not-so-secret-password")
}
}
init {
GlobalSyncableStoreProvider.configureStore("logins" to loginsStorage)
}
private lateinit var loginsStorage: SyncableLoginsStore
private lateinit var listView: ListView
private lateinit var adapter: ArrayAdapter<String>
private lateinit var activityContext: MainActivity
......@@ -63,7 +55,7 @@ class MainActivity : AppCompatActivity(), LoginFragment.OnLoginCompleteListener,
applicationContext,
ServerConfig.release(CLIENT_ID, REDIRECT_URL),
DeviceConfig("A-C Logins Sync Sample", DeviceType.MOBILE, setOf()),
SyncConfig(setOf(SyncEngine.Passwords))
SyncConfig(setOf(SyncEngine.PASSWORDS))
)
}
......@@ -73,6 +65,7 @@ class MainActivity : AppCompatActivity(), LoginFragment.OnLoginCompleteListener,
override fun onCreate(savedInstanceState: Bundle?) {
super.onCreate(savedInstanceState)
RustLog.enable()
RustHttpConfig.setClient(lazy { HttpURLConnectionClient() })
......@@ -89,6 +82,13 @@ class MainActivity : AppCompatActivity(), LoginFragment.OnLoginCompleteListener,
listView.adapter = adapter
activityContext = this
loginsStorage = SyncableLoginsStore(
AsyncLoginsStorageAdapter.forDatabase(File(activityContext.filesDir, "logins.sqlite").canonicalPath)
) {
CompletableDeferred("my-not-so-secret-password")
}
GlobalSyncableStoreProvider.configureStore(SyncEngine.PASSWORDS to loginsStorage)
accountManager.register(accountObserver, owner = this, autoPause = true)
launch { accountManager.initAsync().await() }
......@@ -110,7 +110,7 @@ class MainActivity : AppCompatActivity(), LoginFragment.OnLoginCompleteListener,
@Suppress("EmptyFunctionBlock")
override fun onLoggedOut() {}
override fun onAuthenticated(account: OAuthAccount, newAccount: Boolean) {
override fun onAuthenticated(account: OAuthAccount, authType: AuthType) {
accountManager.syncNowAsync()
}
......@@ -138,9 +138,11 @@ class MainActivity : AppCompatActivity(), LoginFragment.OnLoginCompleteListener,
}
}
override fun onLoginComplete(code: String, state: String, fragment: LoginFragment) {
override fun onLoginComplete(code: String, state: String, action: String, fragment: LoginFragment) {
launch {
accountManager.finishAuthenticationAsync(code, state).await()
accountManager.finishAuthenticationAsync(
FxaAuthData(action.toAuthType(), code = code, state = state)
).await()
supportFragmentManager?.popBackStack()
}
}
......
......@@ -43,5 +43,6 @@ dependencies {
implementation project(':support-rusthttp')
implementation project(':lib-fetch-httpurlconnection')
implementation Dependencies.kotlin_reflect
implementation Dependencies.androidx_recyclerview
}
......@@ -47,8 +47,9 @@ class LoginFragment : Fragment() {
val uri = Uri.parse(url)
val code = uri.getQueryParameter("code")
val state = uri.getQueryParameter("state")
if (code != null && state != null) {
listener?.onLoginComplete(code, state, this@LoginFragment)
val action = uri.getQueryParameter("action")
if (code != null && state != null && action != null) {
listener?.onLoginComplete(code, state, action, this@LoginFragment)
}
}
......@@ -89,7 +90,7 @@ class LoginFragment : Fragment() {
}
interface OnLoginCompleteListener {
fun onLoginComplete(code: String, state: String, fragment: LoginFragment)
fun onLoginComplete(code: String, state: String, action: String, fragment: LoginFragment)
}
companion object {
......
......@@ -19,6 +19,7 @@ import mozilla.components.browser.storage.sync.PlacesBookmarksStorage
import mozilla.components.browser.storage.sync.PlacesHistoryStorage
import mozilla.components.concept.storage.BookmarkNode
import mozilla.components.concept.sync.AccountObserver
import mozilla.components.concept.sync.AuthType
import mozilla.components.concept.sync.ConstellationState
import mozilla.components.concept.sync.Device
import mozilla.components.concept.sync.DeviceCapability
......@@ -37,7 +38,9 @@ import mozilla.components.service.fxa.sync.GlobalSyncableStoreProvider
import mozilla.components.service.fxa.sync.SyncStatusObserver
import mozilla.components.support.base.log.Log
import mozilla.components.lib.fetch.httpurlconnection.HttpURLConnectionClient
import mozilla.components.service.fxa.FxaAuthData
import mozilla.components.service.fxa.SyncEngine
import mozilla.components.service.fxa.toAuthType
import mozilla.components.support.base.log.sink.AndroidLogSink
import mozilla.components.support.rusthttp.RustHttpConfig
import mozilla.components.support.rustlog.RustLog
......@@ -58,8 +61,8 @@ class MainActivity :
}
init {
GlobalSyncableStoreProvider.configureStore("history" to historyStorage)
GlobalSyncableStoreProvider.configureStore("bookmarks" to bookmarksStorage)
GlobalSyncableStoreProvider.configureStore(SyncEngine.HISTORY to historyStorage)
GlobalSyncableStoreProvider.configureStore(SyncEngine.BOOKMARKS to bookmarksStorage)
}
private val accountManager by lazy {
......@@ -71,7 +74,7 @@ class MainActivity :
type = DeviceType.MOBILE,
capabilities = setOf(DeviceCapability.SEND_TAB)
),
SyncConfig(setOf(SyncEngine.History, SyncEngine.Bookmarks), syncPeriodInMinutes = 15L)
SyncConfig(setOf(SyncEngine.HISTORY, SyncEngine.BOOKMARKS), syncPeriodInMinutes = 15L)
)
}
......@@ -161,10 +164,12 @@ class MainActivity :
job.cancel()
}
override fun onLoginComplete(code: String, state: String, fragment: LoginFragment) {
override fun onLoginComplete(code: String, state: String, action: String, fragment: LoginFragment) {
launch {
supportFragmentManager?.popBackStack()
accountManager.finishAuthenticationAsync(code, state).await()
accountManager.finishAuthenticationAsync(
FxaAuthData(action.toAuthType(), code = code, state = state)
).await()
}
}
......@@ -227,6 +232,8 @@ class MainActivity :
}
private val accountObserver = object : AccountObserver {
lateinit var lastAuthType: AuthType
override fun onLoggedOut() {
launch {
val txtView: TextView = findViewById(R.id.fxaStatusView)
......@@ -261,10 +268,12 @@ class MainActivity :
}
}
override fun onAuthenticated(account: OAuthAccount, newAccount: Boolean) {
override fun onAuthenticated(account: OAuthAccount, authType: AuthType) {
launch {
lastAuthType = authType
val txtView: TextView = findViewById(R.id.fxaStatusView)
txtView.text = getString(R.string.signed_in_waiting_for_profile)
txtView.text = getString(R.string.signed_in_waiting_for_profile, authType::class.simpleName)
findViewById<View>(R.id.buttonLogout).visibility = View.VISIBLE
findViewById<View>(R.id.buttonSignIn).visibility = View.INVISIBLE
......@@ -285,6 +294,7 @@ class MainActivity :
val txtView: TextView = findViewById(R.id.fxaStatusView)
txtView.text = getString(
R.string.signed_in_with_profile,
lastAuthType::class.simpleName,
"${profile.displayName ?: ""} ${profile.email}"
)
}
......@@ -315,7 +325,7 @@ class MainActivity :
if (bookmarksRoot == null) {
bookmarksResultTextView.text = getString(R.string.no_bookmarks_root)
} else {
var bookmarksRootAndChildren = "Bookmarks\n"
var bookmarksRootAndChildren = "BOOKMARKS\n"
fun addTreeNode(node: BookmarkNode, depth: Int) {
val desc = " ".repeat(depth * 2) + "${node.title} - ${node.url} (${node.guid})\n"
bookmarksRootAndChildren += desc
......
......@@ -5,8 +5,8 @@
<resources>
<string name="app_name">Firefox Sync Demo</string>
<string name="sign_in">FxA sign in</string>
<string name="signed_in_waiting_for_profile">Signed in; waiting for profile</string>
<string name="signed_in_with_profile">Signed in: %1$s</string>
<string name="signed_in_waiting_for_profile">Signed in (type=%1$s); waiting for profile</string>
<string name="signed_in_with_profile">Authenticated (type=%1$s) as %2$s</string>
<string name="account_error">FxA error: %1$s</string>
<string name="sync_idle">Sync is idle</string>
<string name="syncing">Syncing&#8230;</string>
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment