/synapse

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Add device_id support to /login #929

Merged
merged 2 commits into from Jul 19, 2016

Conversation

Reviewers
No reviews
Assignees

@erikjohnston erikjohnston

Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@richvdh @erikjohnston
@richvdh
Member

richvdh commented Jul 18, 2016 • edited

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).
@richvdh
Add device_id support to /login 
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.

Some checks were not successful

2 errored, 1 failing, and 7 successful checks
@matrixbot
Sytest Dendron (Commit) — Build #277 origin/rav/support_deviceid_in_login failed in 2 min 27 sec
Details
@matrixbot
Sytest Postgres (Commit) — Build #1114 origin/rav/support_deviceid_in_login failed in 6 min 18 sec
Details
@matrixbot
Sytest Postgres (Merged PR) — Build finished.
Details
@matrixbot
Flake8 + Packaging (Commit) — Build #1163 origin/rav/support_deviceid_in_login succeeded in 33 sec
Details
@matrixbot
Flake8 + Packaging (Merged PR) — Build finished.
Details
@matrixbot
Sytest Dendron (Merged PR) — Build finished.
Details
@matrixbot
Sytest SQLite (Commit) — Build #1136 origin/rav/support_deviceid_in_login succeeded in 5 min 59 sec
Details
@matrixbot
Sytest SQLite (Merged PR) — Build finished.
Details
@matrixbot
Unit Tests (Commit) — Build #1202 origin/rav/support_deviceid_in_login succeeded in 1 min 37 sec
Details
@matrixbot
Unit Tests (Merged PR) — Build finished.
Details
f863a52

@richvdh richvdh assigned erikjohnston Jul 18, 2016

@richvdh
Member

richvdh commented Jul 18, 2016

hrm, will investigate sytest fails
@richvdh
schema fix 
device_id should be text, not bigint.

All checks have passed

10 successful checks
@matrixbot
Flake8 + Packaging (Commit) — Build #1167 origin/rav/support_deviceid_in_login succeeded in 34 sec
Details
@matrixbot
Flake8 + Packaging (Merged PR) — Build finished.
Details
@matrixbot
Sytest Dendron (Commit) — Build #281 origin/rav/support_deviceid_in_login succeeded in 7 min 20 sec
Details
@matrixbot
Sytest Dendron (Merged PR) — Build finished.
Details
@matrixbot
Sytest Postgres (Commit) — Build #1118 origin/rav/support_deviceid_in_login succeeded in 6 min 36 sec
Details
@matrixbot
Sytest Postgres (Merged PR) — Build finished.
Details
@matrixbot
Sytest SQLite (Commit) — Build #1140 origin/rav/support_deviceid_in_login succeeded in 6 min 6 sec
Details
@matrixbot
Sytest SQLite (Merged PR) — Build finished.
Details
@matrixbot
Unit Tests (Commit) — Build #1206 origin/rav/support_deviceid_in_login succeeded in 1 min 33 sec
Details
@matrixbot
Unit Tests (Merged PR) — Build finished.
Details
022b917

@erikjohnston erikjohnston commented on the diff Jul 19, 2016

View full changes
synapse/handlers/auth.py
@@ -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:
@erikjohnston

erikjohnston Jul 19, 2016

Owner

What does a None device id mean?

@richvdh

richvdh Jul 19, 2016

Member

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

@erikjohnston erikjohnston commented on the diff Jul 19, 2016

View full changes
synapse/storage/devices.py
+ Raises:
+ StoreError: if ignore_if_known is False and the device was already
+ known
+ """
+ try:
+ yield self._simple_insert(
+ "devices",
+ values={
+ "user_id": user_id,
+ "device_id": device_id,
+ "display_name": initial_device_display_name
+ },
+ desc="store_device",
+ or_ignore=ignore_if_known,
+ )
+ except Exception as e:
@erikjohnston

erikjohnston Jul 19, 2016

Owner

Why are we catching the generic exception ooi? Rather than e.g. StoreError

@richvdh

richvdh Jul 19, 2016

Member

well, _simple_insert won't ever throw StoreError.

But generally, good question. I cargo-culted this from similar code elsewhere. I thought the idea was to turn the database-layer exceptions into synapse exceptions here, and presumably catching every conceivable database-layer exception is tricky.
@erikjohnston
Owner

erikjohnston 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
@richvdh
Member

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
Owner

erikjohnston commented Jul 19, 2016

LGTM

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

@richvdh richvdh merged commit 9a7a77a into develop Jul 19, 2016
10 checks passed

10 checks passed

Flake8 + Packaging (Commit) Build #1167 origin/rav/support_deviceid_in_login succeeded in 34 sec
Details
Flake8 + Packaging (Merged PR) Build finished.
Details
Sytest Dendron (Commit) Build #281 origin/rav/support_deviceid_in_login succeeded in 7 min 20 sec
Details
Sytest Dendron (Merged PR) Build finished.
Details
Sytest Postgres (Commit) Build #1118 origin/rav/support_deviceid_in_login succeeded in 6 min 36 sec
Details
Sytest Postgres (Merged PR) Build finished.
Details
Sytest SQLite (Commit) Build #1140 origin/rav/support_deviceid_in_login succeeded in 6 min 6 sec
Details
Sytest SQLite (Merged PR) Build finished.
Details
Unit Tests (Commit) Build #1206 origin/rav/support_deviceid_in_login succeeded in 1 min 33 sec
Details
Unit Tests (Merged PR) Build finished.
Details

@richvdh richvdh deleted the rav/support_deviceid_in_login branch Jul 19, 2016

@MrChoclate MrChoclate referenced this pull request in matrix-org/matrix-android-sdk Jun 20, 2017

Open

add device_id param to login api #180

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment