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

Send Tab #676

Merged
merged 1 commit into from Apr 22, 2019

Conversation

Projects
None yet
7 participants
@eoger
Copy link
Contributor

commented Feb 14, 2019

TODO

  • Needs changelog entry (moved from @rfk's comment in #821)

@eoger eoger added the WIP label Feb 14, 2019

@eoger eoger force-pushed the send-tab-devices-API branch 2 times, most recently from a70fedf to 2375363 Feb 15, 2019

@eoger eoger force-pushed the send-tab-devices-API branch 4 times, most recently from 8f20af1 to 620a8ba Feb 19, 2019

@eoger eoger requested review from grigoryk, vladikoff and rfk Feb 20, 2019

@rfk
Copy link
Member

left a comment

Thanks @eoger, I left a couple of initial thoughts but will have to come back to this again another day

Show resolved Hide resolved components/fxa-client/Cargo.toml Outdated
Show resolved Hide resolved ...xa-client/android/src/main/java/org/mozilla/fxaclient/internal/Device.kt Outdated
Show resolved Hide resolved ...xa-client/android/src/main/java/org/mozilla/fxaclient/internal/Device.kt Outdated
Show resolved Hide resolved ...t/android/src/main/java/org/mozilla/fxaclient/internal/FirefoxAccount.kt Outdated
/**
* Ensure the current device "Send Tab" commands has been registered with the server.
* This method should be called once per "device lifetime" after the Sync Keys have been
* obtained and called again if they change.

This comment has been minimized.

Copy link
@rfk

rfk Feb 21, 2019

Member

It feels like we should be able to detect internally if the sync keys change (e.g. because our refreshToken gets revoked) so I wonder if there's anything we can do to make this easier on the calling code.

This comment has been minimized.

Copy link
@eoger

eoger Feb 21, 2019

Author Contributor

I guess we could attempt a registration every time we see a Sync Key pass in front of us since there is only one way to get them, however we'd need a way for apps to opt-out of Send Tab and register the display-uri command (maybe with Config?).

Show resolved Hide resolved ...t/android/src/main/java/org/mozilla/fxaclient/internal/FirefoxAccount.kt Outdated
rustCall { e ->
FxaClient.INSTANCE.fxa_ensure_send_tab_registered(validHandle(), e)
}
}

This comment has been minimized.

Copy link
@rfk

rfk Feb 21, 2019

Member

FWIW, I haven't given up hope that we'll one day figure out how to factor this out into a separate module rather than growing a method off the main FxAClient object, perhaps after we get more experience with shipping multi-component megazord setups on different platforms. But a big 👍 to getting it out the door this way to start with :-)

Show resolved Hide resolved components/fxa-client/src/commands/send_tab.rs Outdated
Show resolved Hide resolved components/fxa-client/src/lib.rs Outdated

@eoger eoger force-pushed the send-tab-devices-API branch 7 times, most recently from 8cb2077 to 95d3b03 Feb 25, 2019

@eoger

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2019

Note: we need to re-register our device record when we drop our refresh token to craft a new one

@mhammond mhammond added review and removed in progress labels Mar 4, 2019

@rfk
Copy link
Member

left a comment

I'm working my way through a review of this, still in progress but got to stop for some meetings. Initial comments below.

Show resolved Hide resolved ...ient/android/src/main/java/mozilla/appservices/fxaclient/AccountEvent.kt
Show resolved Hide resolved ...fxa-client/android/src/main/java/mozilla/appservices/fxaclient/Device.kt Outdated
Show resolved Hide resolved ...nt/android/src/main/java/mozilla/appservices/fxaclient/FirefoxAccount.kt Outdated
rustCall { e ->
LibFxAFFI.INSTANCE.fxa_set_push_subscription(this.handle.get(), endpoint, publicKey, authKey, e)
}
}

This comment has been minimized.

Copy link
@rfk

rfk Mar 11, 2019

Member

What should the caller do if the network request fails here? Do they need to remember that it failed and retry again at some later time? Let's discuss with @grigoryk at the next sync-up to ensure we have consistent expectations around how to handle failed device data updates.

*
* This performs network requests, and should not be used on the main thread.
*
* @return A collection of [AccountEvent] that should be handled by the caller.

This comment has been minimized.

Copy link
@rfk

rfk Mar 11, 2019

Member

Where should the caller look for documentation on the possible [AccountEvent] types and what actions it should take in response to them? Is this a doc that needs to exist somewhere in FxA?

Show resolved Hide resolved ...nt/android/src/main/java/mozilla/appservices/fxaclient/FirefoxAccount.kt
Show resolved Hide resolved ...nt/android/src/main/java/mozilla/appservices/fxaclient/FirefoxAccount.kt Outdated
/**
* Ensure the current device "Send Tab" commands has been registered with the server.
* This method should be called once per "device lifetime" after the Sync Keys have been
* obtained and called again if they change.

This comment has been minimized.

Copy link
@rfk

rfk Mar 11, 2019

Member

Is "device lifetime" a sufficiently-well-defined term that we can use it here, or should we try to be more specific? I wonder if we can explicitly enumerate the set of circumstances under which this should be called, e.g. "after successful signin", "after re-authenticating", maybe others as well?

Alternately, I believe that we can tell whether or not the send-tab command was registered from our internal state, would it be possible for us to take care of this re-registration automagically after device lifetime events?

Show resolved Hide resolved components/fxa-client/ffi/src/lib.rs Outdated
Show resolved Hide resolved components/fxa-client/src/browser_id.rs
@rfk
Copy link
Member

left a comment

This is a big PR @eoger but it's looking good! Sorry it's taken me so long to go through it properly. I've left a bunch of comments about ways I think it could be improved, but they're mostly code-quality questions/concerns rather than functionality changes.

From talking to @vladikoff, it sounds like https://devices2.dev.lcip.org/ should be ready for you to try out this code against the "for real" version of the device API changes, if you want to give it a spin. otherwise, I think our next step here is to get @grigoryk to take the API for a spin and see how we want to iterate it from there.

Show resolved Hide resolved components/fxa-client/src/commands/send_tab.rs
Show resolved Hide resolved components/fxa-client/src/commands/send_tab.rs Outdated
Show resolved Hide resolved components/fxa-client/src/commands/send_tab.rs Outdated
Show resolved Hide resolved components/fxa-client/src/commands/send_tab.rs Outdated
Show resolved Hide resolved components/fxa-client/src/commands/send_tab.rs Outdated
last_handled_command: Option<u64>,
// Remove serde(default) once we are V3.
#[serde(default)]
commands_data: HashMap<String, String>,

This comment has been minimized.

Copy link
@rfk

rfk Mar 12, 2019

Member

Given that you're storing the commands data in the local state here, what's the reason that we don't also want to store the other device-related info like name, type etc? ISTM that keeping a local copy would make it easier to do things like replace_device in a more robust manner.

This comment has been minimized.

Copy link
@eoger

eoger Mar 12, 2019

Author Contributor

I'd like to avoid having to track locally what's registered on the server, but if we need it I'm not opposed to do it.

&device_info.available_commands,
) {
log::warn!("Device information restoration failed: {:?}", err);
}

This comment has been minimized.

Copy link
@rfk

rfk Mar 12, 2019

Member

So if this does error out (or equivalently, if we crashed in between deleting the refresh token and re-creating the device record) is there some way for the app to detect that it no longer has a device record and needs to re-register push endpoints etc?

This comment has been minimized.

Copy link
@eoger

eoger Mar 12, 2019

Author Contributor

I don't think it does, but I don't really expect this to be a problem. We can do regular re-registrations if we feel it becomes a problem later.

Show resolved Hide resolved components/fxa-client/src/scoped_keys.rs Outdated
Show resolved Hide resolved components/fxa-client/src/send_tab.rs Outdated
};
let encrypted_payload: EncryptedSendTabPayload = serde_json::from_value(payload)?;
Ok((sender, encrypted_payload.decrypt(&send_tab_key)?))
}

This comment has been minimized.

Copy link
@rfk

rfk Mar 12, 2019

Member

One API method that might be missing here, is a method to list all available send-tab targets.

IIUC the only way for the calling code to do this right now is to call get_devices and then iterate over each device, looking for the ones that have registered the open-uri command, remembering to skip itself, and maybe even checking that their open-uri command data is encrypted with the correct kid.

Can we (and should we?) hide that complexity behind a get_send_tab_target_devices() method of some kind, rather than requiring the calling code to know about the internal implementation details of the open-uri command?

An advantage of doing it this way is that we may be able to split the available targets into several categories:

  • The device advertises send-tab and we're able to send to it.
  • The device advertises send-tab, but we can't send to it for some reason (maybe it's command data is encrypted with an old key or something).
    • The app could show these greyed-out or similar
  • The device does not support send-tab at all
    • Fennec would be in this list; maybe the app wants to show them greyed out or similar?

@grigoryk would love to get your thoughts on this one.

This comment has been minimized.

Copy link
@rfk

rfk Apr 17, 2019

Member

This got a thumbs-up but doesn't look like we ended up going in that direction; future work, or did it not seem worth it in practice in the end?

This comment has been minimized.

Copy link
@eoger

eoger Apr 18, 2019

Author Contributor

Sorry I should have commented on that. @grigoryk and I settled on populating a capabilities set on every device, so that Android can do the filtering itself (and avoid having to grow a new getDevice method for each new command in the future).

@rfk

This comment has been minimized.

Copy link
Member

commented Mar 12, 2019

One other general comment I have here is about lifecycle management. My impression of the current PR is that we're trying now to do much of that ourselves, and the application is going to be responsible for calling things like setPushEndpoint and ensureSendTabCommandRegistered. Does the application have appropriate visibility into the lifecycle events that would drive it to make such calls? It's not clear to me whether it does, and perhaps we'll find out during the initial attempt to integrate this with android-components.

@thomcc

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2019

It's worth noting that my clients collection is basically accurate (It's missing one device but I guess I haven't turned that one on in long enough that, well, sure, it's fair that it's missing).

(However, clients collection does more involved filtering than just last use time)

@vladikoff

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

I'm able to poll for commands, but fxaclient is always failing to decrypt while processing the open-uri command
fxa_client::device: Error while processing command: http-ece encryption error: OpenSSL error: OpenSSL error

@grigoryk @eoger There seems to be an issue with the generated keys and how they are persisted into state. Adding some logs here: 9d0bfce#diff-59b417af97d50deff45a82e0bbc98373R19

I noticed that every time I restarted the android app new keys are generated from_random(). (You can send a log there to see it)

@grigoryk

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2019

Following up on the keys and state, a-c's FxaAccountManager never registered a state persistence callback, and so we were dropping the SendTab state on the floor after an app restart. Introducing state persistence solves the problem.

@eoger

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2019

API changes requested by @grigoryk:

  • Create an enum Command (or Capability) and attach it to Device, so r-b can tell which whose devices are compatible with send tab
  • ensureDeviceRegistered with type: DeviceType/name: String params (also registers Send Tab)
@vladikoff

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

@thomcc left some comments on the code here in this review: #821 (review) (just want to make sure we don't lose it)

@eoger eoger force-pushed the send-tab-devices-API branch 2 times, most recently from 5eadc02 to 47cdfee Apr 4, 2019

@grigoryk

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2019

Another API change requests:

Instead of a single ensureDeviceRegistered(name: String, type: DeviceType) method, let's split it into two: setInitialNameAndType(name: String, type: DeviceType), and ensureDeviceRegistered().

This simplifies the consumer side: this separates out a very particular action (setting initial name), so we don't have to worry about renaming our device accidentally by restarting an app. We have a separation in the fxa manager in a-c before "account restore" and "account logged-in", and this split maps well to that. It also gives us a clear migration path in the future should we need to change something on an app upgrade - the ensure call can take care of it. My expectation is that the ensure call will be idempotent, and eventually be entirely a no-op if the device record already exists (e.g. no networking performed).

@grigoryk

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

IIUC, it's not possible to declare current device's capabilities via the current API. That is, it's not possible to register an FxA device, but also opt-out of being a Send Tab target.
I assume something like Lockbox would want something like that.

Because of this, in my integration PR I've elected to not try to register a device if the application didn't explicitly state it wants to process events targeted at the device. But that doesn't quite seem right either, since I assume we always want to see connected devices in the FxA device manager, regardless of their capabilities.

@rfk

This comment has been minimized.

Copy link
Member

commented Apr 9, 2019

since I assume we always want to see connected devices in the FxA device manager,
regardless of their capabilities.

That they don't currently show up until you explicitly register a device record, is a bug in FxA which will be fixed as part of mozilla/fxa#466.

I agree that you'll want to be able to register a device without necessarily registering for send-tab, but that shouldn't be a blocker for appearing in the FxA "devices and apps" list.

@eoger eoger force-pushed the send-tab-devices-API branch 2 times, most recently from 147a375 to 7f869c2 Apr 12, 2019

@eoger

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

@grigoryk just letting you know the APIs we discussed have been integrated to this PR.

@grigoryk

This comment has been minimized.

Copy link
Contributor

commented Apr 15, 2019

@eoger great, thanks! This looks good to me from the API perspective. I'll update my PRs and report back.

@vladikoff

This comment has been minimized.

Copy link
Member

commented Apr 15, 2019

deviceType is DESKTOP for OAuth clients. We need to have ability to set device type to mobile when we first create a device record

This should be fixed in the next FxA release, new clients will be mobile.

@grigoryk

This comment has been minimized.

Copy link
Contributor

commented Apr 15, 2019

deviceType is DESKTOP for OAuth clients. We need to have ability to set device type to mobile when we first create a device record

This should be fixed in the next FxA release, new clients will be mobile.

The API we have now requires specifying device type during initialization, which should be enough here as well.

/// Initalizes our own device, most of the time this will be called right after logging-in
/// for the first time.
#[no_mangle]
pub unsafe extern "C" fn fxa_initialize_device(

This comment has been minimized.

Copy link
@rfk

rfk Apr 17, 2019

Member

nit: document why this this needs to be unsafe?

get() = this

sealed class AccountEvent {
class TabReceived(val from: Device?, val entries: Array<TabData>) : AccountEvent()

This comment has been minimized.

Copy link
@rfk

rfk Apr 17, 2019

Member

+1 to TabsReceived FWIW

val lastAccessTime: Long?,
val capabilities: List<Capability>
) {
enum class Capability {

This comment has been minimized.

Copy link
@rfk

rfk Apr 17, 2019

Member

I had a bit of a chuckle at this, because we used to call these "capabilities" on the server side until we needed more data and turned them into "commands".

If we end up removing support for non-megazord builds, does that clear a future path to separating the send-tab stuff out into its own distinct component, at which point we might not need "capability" flags like this inside the core FxA component? (For the future obviously, this seems fine for now).

This comment has been minimized.

Copy link
@eoger

eoger Apr 18, 2019

Author Contributor

I had a bit of a chuckle at this, because we used to call these "capabilities" on the server side until we needed more data and turned them into "commands".

Yup my rationale is that the clients don't need the Commands "values" as the UI only care about "capabilities", but I might be wrong in the future :)

If we end up removing support for non-megazord builds, does that clear a future path to separating the send-tab stuff out into its own distinct component, at which point we might not need "capability" flags like this inside the core FxA component?

Yes it might make it easier, we should look into that later this year.

Show resolved Hide resolved ...nt/android/src/main/java/mozilla/appservices/fxaclient/FirefoxAccount.kt Outdated
Show resolved Hide resolved components/fxa-client/src/commands/send_tab.rs Outdated
};
let encrypted_payload: EncryptedSendTabPayload = serde_json::from_value(payload)?;
Ok((sender, encrypted_payload.decrypt(&send_tab_key)?))
}

This comment has been minimized.

Copy link
@rfk

rfk Apr 17, 2019

Member

This got a thumbs-up but doesn't look like we ended up going in that direction; future work, or did it not seem worth it in practice in the end?

@eoger eoger force-pushed the send-tab-devices-API branch from 7f869c2 to 38ed596 Apr 18, 2019

@eoger

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2019

Updated with latest @rfk review comments addressed, thanks!

@eoger eoger force-pushed the send-tab-devices-API branch from 38ed596 to 57823e5 Apr 22, 2019

@grigoryk grigoryk self-requested a review Apr 22, 2019

@grigoryk
Copy link
Contributor

left a comment

🚢 🇮🇹

@eoger eoger merged commit 1456555 into master Apr 22, 2019

10 checks passed

Taskcluster (pull_request) TaskGroup: success
Details
ci/circleci: Carthage build Your tests passed on CircleCI!
Details
ci/circleci: Check Rust dependencies Your tests passed on CircleCI!
Details
ci/circleci: Check Rust formatting Your tests passed on CircleCI!
Details
ci/circleci: Deploy website Your tests passed on CircleCI!
Details
ci/circleci: Lint Rust with clippy Your tests passed on CircleCI!
Details
ci/circleci: Rust benchmarks Your tests passed on CircleCI!
Details
ci/circleci: Rust tests - beta Your tests passed on CircleCI!
Details
ci/circleci: Rust tests - stable Your tests passed on CircleCI!
Details
ci/circleci: Sync integration tests Your tests passed on CircleCI!
Details

@eoger eoger deleted the send-tab-devices-API branch Apr 30, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.