search

mozilla / fxa-auth-server

DEPRECATED - Migrated to https://github.com/mozilla/fxa
Mozilla Public License 2.0
399 stars 121 forks source link

Try to access devices w/ a refreshToken. #2896

Closed vladikoff closed 5 years ago

vladikoff commented 5 years ago

Connects to https://github.com/mozilla/fxa-auth-server/issues/2547 issue #2547

eoger commented 5 years ago

Could we also add a PATCH /account/device/commands endpoint using rfc7396 formatted payload to add/remove commands?

eoger commented 5 years ago

While I'm at it a GET /account/device might be nice to have.

rfk commented 5 years ago

Could we also add a PATCH /account/device/commands endpoint using rfc7396 formatted payload to add/remove commands?

Is this to avoid you having to track the set of registered commands client-side, or just to avoid overhead of uploading the entire set of commands every time?

eoger commented 5 years ago

Could we also add a PATCH /account/device/commands endpoint using rfc7396 formatted payload to add/remove commands?

Is this to avoid you having to track the set of registered commands client-side, or just to avoid overhead of uploading the entire set of commands every time?

The former

rfk commented 5 years ago

Some behaviours I've been trying to think through:

I think we have sensible answers to these questions, but we'll have to watch out to enforce them in implementation.

eoger commented 5 years ago

Would it be possible to arrange for a device (synthesized name + type inferred from client ID) to be created when getting a new refresh token?

One more thing to consider also: the FxAClient rust crate, in order to request new scopes and keep only 1 refresh token, will drop the refresh token and get a new one, which will make us lose our device info. What do we want to do about this? Should we prioritize incremental oauth?

rfk commented 5 years ago

Just to capture some outcomes from meetings this week:

I think it'd be worth attempting a DB migration to add a separate refreshTokenId column to the devices table, and seeing what flow-on effects this will have throughout the code.

We agreed to try this and I'm going to work on it imminently.

Could we also add a PATCH /account/device/commands endpoint using rfc7396 formatted payload to add/remove commands?

We're going to defer this for now; the client will need to re-upload the entire commands bundle whenever it wants to make a change.

Would it be possible to arrange for a device (synthesized name + type inferred from client ID) to be created when getting a new refresh token?

I believe @vladikoff has a work-in-progress PR that implements this, since we also need it for other reasons such as sending "now syncing with XYZ" notifications.

the FxAClient rust crate, in order to request new scopes and keep only 1 refresh token, will drop the refresh token and get a new one, which will make us lose our device info. What do we want to do about this? Should we prioritize incremental oauth?

I don't think we have the bandwidth for incremental oauth right now; is the client crate able to just re- register its device record as part of this flow?

While I'm at it a GET /account/device might be nice to have.

I don't think we made a decision about this one either way; @eoger what sort of priority would this be for you? It seems uncontroversial, but if we can avoid it for now, having less to do will probably let us land these changes faster.

eoger commented 5 years ago

I don't think we have the bandwidth for incremental oauth right now; is the client crate able to just re-register its device record as part of this flow?

Yes I could explicitly call /devices, grab our own device info, get the new token and replicate the device info.

I don't think we made a decision about this one either way; @eoger what sort of priority would this be for you? It seems uncontroversial, but if we can avoid it for now, having less to do will probably let us land these changes faster.

Let's drop it

rfk commented 5 years ago

From slack conversation with @eoger: running RUST_BACKTRACE=1 cargo run --example devices_api on his send-tab PR will panic due to some missing fields in device responses.

rfk commented 5 years ago

FYI I'm squashing and rebasing this on latest master.

rfk commented 5 years ago

running RUST_BACKTRACE=1 cargo run --example devices_api on his send-tab PR will panic due to some missing fields in device responses.

I'm able to reproduce this, which is good; I'll see if I can push a quick fix to unblock next steps.

@eoger the failing line in your rust code says:

    // Synthetize a default record. Should be done by the server.
    acct.set_display_name(DEFAULT_DEVICE_NAME).unwrap();

Can you help me undertand your expectations from the API here? My interpretation is that you don't want to have to distinguish between creating a new registration (and thus having to satisfy the requirement that If no device id is specified, both name and type must be provided) and updating a new registration (where you're supposed to send your deviceId). I think we can probably remove this pain-point for you by synthesizing a default name and type from your oauth client metadata, but want to make sure I understand your concern here.

rfk commented 5 years ago

OK, with the latest updates on this branch I am able to get cargo run --example devices_api working successfully. It does not show in the "Devices & apps" list on the web yet, but I think I understand why (that list actually uses /sessions rather than /devices).

IIUC the rust code is currently assuming that the response from /account/device will:

This is not currently promised anywhere in the API docs AFAICT, but seems reasonable to me.

It is currently possible for a device to set pushCallback without setting pushPublicKey and pushAuthKey, which was used by older versions of push. I don't know if any clients still exhibit this behaviour, and if possible, updating the rust code to accommodate this is probably easier than auditing our set of stored devices to find out.

eoger commented 5 years ago

I think we can probably remove this pain-point for you by synthesizing a default name and type from your oauth client metadata, but want to make sure I understand your concern here.

Yup that's exactly what I was thinking :)

eoger commented 5 years ago

From meeting: scope in the OAuth response is falsy.

eoger commented 5 years ago

It is currently possible for a device to set pushCallback without setting pushPublicKey and pushAuthKey, which was used by older versions of push. I don't know if any clients still exhibit this behaviour, and if possible, updating the rust code to accommodate this is probably easier than auditing our set of stored devices to find out.

Opened https://github.com/mozilla/fxa-auth-server/issues/2936.