Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Add device_id support to /login #929

Merged
merged 2 commits into from
Jul 19, 2016
Merged

Conversation

richvdh
Copy link
Member

@richvdh richvdh commented Jul 18, 2016

Add a 'devices' table to the storage, as well as a 'device_id' column to refresh_tokens.

Allow the client to pass a device_id, and initial_device_display_name, to /login. If login is successful, then register the device in the devices table if it wasn't known already. If no device_id was supplied, make one up.

Associate the device_id with the access token and refresh token, so that we can get at it again later. Ensure that the device_id is copied from the refresh token to the access_token when the token is refreshed.

(Next step in this process will be to also support 'device_id' in the registration API).

Add a 'devices' table to the storage, as well as a 'device_id' column to
refresh_tokens.

Allow the client to pass a device_id, and initial_device_display_name, to
/login. If login is successful, then register the device in the devices table
if it wasn't known already. If no device_id was supplied, make one up.

Associate the device_id with the access token and refresh token, so that we can
get at it again later. Ensure that the device_id is copied from the refresh
token to the access_token when the token is refreshed.
@richvdh richvdh force-pushed the rav/support_deviceid_in_login branch from e260ff3 to f863a52 Compare July 18, 2016 15:39
@richvdh
Copy link
Member Author

richvdh commented Jul 18, 2016

hrm, will investigate sytest fails

device_id should be text, not bigint.
@@ -372,6 +372,7 @@ def get_login_tuple_for_user_id(self, user_id):

Args:
user_id (str): canonical User ID
device_id (str): the device ID to associate with the access token
Returns:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does a None device id mean?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It means "this access token is not associated with a device".

@erikjohnston
Copy link
Member

Do we actually use the fact that device_id is optional everywhere? I would have thought we'd want to enforce that device_id is specified everywhere

@richvdh
Copy link
Member Author

richvdh commented Jul 19, 2016

Do we actually use the fact that device_id is optional everywhere? I would have thought we'd want to enforce that device_id is specified everywhere

Well, certainly at this point, the registration flow still generates access_tokens without device_ids. In general, I'm not confident in my ability to comb exhaustively through the code (yay dynamic languages) and make sure that a device_id is generated in all the places we need one, and would rather be a bit defensive and try to make sure that things keep working rather than throwing runtime exceptions.

@erikjohnston
Copy link
Member

LGTM

(Would be good to see some sytests that hit the code paths)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants