/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

Make registration idempotent #649

Merged
merged 9 commits into from Mar 16, 2016

Conversation

Reviewers
No reviews
Assignees

@erikjohnston erikjohnston

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

dbkr commented Mar 16, 2016

If you specify the same session, make it give you an access token for the user that was registered on previous uses of that session. Tweak the UI auth layer to not delete sessions when their auth has completed and hence expire them so they don't hang around until server restart. Allow server-side data to be associated with UI auth sessions.

sytest: matrix-org/sytest#213
Fixes vector-im/vector-web#957
@dbkr
Make registration idempotent: if you specify the same session, make i… 
…t give you an access token for the user that was registered on previous uses of that session. Tweak the UI auth layer to not delete sessions when their auth has completed and hence expire themn so they don't hang around until server restart. Allow server-side data to be associated with UI auth sessions.

All checks have failed

4 errored and 4 failing checks
@matrixbot
Flake8 + Packaging (Commit) — Build #141 origin/dbkr/idempotent_registration failed in 28 sec
Details
@matrixbot
Sytest Postgres (Commit) — Build #144 origin/dbkr/idempotent_registration failed in 4 min 29 sec
Details
@matrixbot
Sytest SQLite (Commit) — Build #145 origin/dbkr/idempotent_registration failed in 3 min 18 sec
Details
@matrixbot
Unit Tests (Commit) — Build #186 origin/dbkr/idempotent_registration failed in 1 min 9 sec
Details
@matrixbot
Flake8 + Packaging (Merged PR) — Build finished.
Details
@matrixbot
Sytest Postgres (Merged PR) — Build finished.
Details
@matrixbot
Sytest SQLite (Merged PR) — Build finished.
Details
@matrixbot
Unit Tests (Merged PR) — Build finished.
Details
c12b9d7

@dbkr dbkr assigned erikjohnston Mar 16, 2016

dbkr added a commit to matrix-org/sytest that referenced this pull request Mar 16, 2016

@dbkr
Test registration idempotency (Tests matrix-org/synapse#649)

All checks have failed

2 errored checks
@matrixbot
Sytest-Branches — Build #1181 origin/dbkr/idempotent_registration failed in 3 min 53 sec
Details
@matrixbot
Sytest-Postgres-Branches — Build #1326 origin/dbkr/idempotent_registration failed in 4 min 52 sec
Details
2d709b1

@dbkr dbkr referenced this pull request in matrix-org/sytest Mar 16, 2016

Merged

Test registration idempotency #213

dbkr added some commits Mar 16, 2016

@dbkr
pep8 & remove debug logging

Some checks were not successful

3 errored, 3 failing, and 2 successful checks
@matrixbot
Sytest Postgres (Commit) — Build #145 origin/dbkr/idempotent_registration failed in 5 min 24 sec
Details
@matrixbot
Sytest SQLite (Commit) — Build #146 origin/dbkr/idempotent_registration failed in 4 min 11 sec
Details
@matrixbot
Unit Tests (Commit) — Build #187 origin/dbkr/idempotent_registration failed in 1 min 7 sec
Details
@matrixbot
Sytest Postgres (Merged PR) — Build finished.
Details
@matrixbot
Sytest SQLite (Merged PR) — Build finished.
Details
@matrixbot
Unit Tests (Merged PR) — Build finished.
Details
@matrixbot
Flake8 + Packaging (Commit) — Build #142 origin/dbkr/idempotent_registration succeeded in 29 sec
Details
@matrixbot
Flake8 + Packaging (Merged PR) — Build finished.
Details
9979794
@dbkr
Fix tests
ff7d3dc
@dbkr
take extra return val from check_auth in account too

All checks have passed

8 successful checks
@matrixbot
Flake8 + Packaging (Commit) — Build #143 origin/dbkr/idempotent_registration succeeded in 29 sec
Details
@matrixbot
Flake8 + Packaging (Merged PR) — Build finished.
Details
@matrixbot
Sytest Postgres (Commit) — Build #146 origin/dbkr/idempotent_registration succeeded in 4 min 24 sec
Details
@matrixbot
Sytest Postgres (Merged PR) — Build finished.
Details
@matrixbot
Sytest SQLite (Commit) — Build #147 origin/dbkr/idempotent_registration succeeded in 3 min 18 sec
Details
@matrixbot
Sytest SQLite (Merged PR) — Build finished.
Details
@matrixbot
Unit Tests (Commit) — Build #188 origin/dbkr/idempotent_registration succeeded in 1 min 1 sec
Details
@matrixbot
Unit Tests (Merged PR) — Build finished.
Details
f5e9042

@erikjohnston erikjohnston commented on an outdated diff Mar 16, 2016

View full outdated diff
synapse/handlers/auth.py
@@ -455,11 +485,17 @@ def add_threepid(self, user_id, medium, address, validated_at):
def _save_session(self, session):
# TODO: Persistent storage
logger.debug("Saving session %s", session)
+ session["last_used"] = time.time()
@erikjohnston

erikjohnston Mar 16, 2016

Owner

Why not use hs.get_clock()?

We generally try and consistently use milliseconds internally, rather than seconds.

@erikjohnston erikjohnston commented on an outdated diff Mar 16, 2016

View full outdated diff
synapse/handlers/auth.py
@@ -35,6 +36,7 @@
class AuthHandler(BaseHandler):
+ SESSION_EXPIRE_SECS = 48 * 60 * 60
@erikjohnston

erikjohnston Mar 16, 2016

Owner

We generally try and consistently always use milliseconds.

(You get brownie points for including "secs" in the name though)
@erikjohnston
Owner

erikjohnston commented Mar 16, 2016

To what extend do we care if session information gets dropped on the floor over restarts?
@dbkr
Member

dbkr commented Mar 16, 2016

We probably do care that session information gets dropped over restarts, but no more so now than before, so I think that would be a separate fix.

@erikjohnston erikjohnston and 1 other commented on an outdated diff Mar 16, 2016

View full outdated diff
synapse/handlers/auth.py
@@ -263,7 +293,7 @@ def _get_session_info(self, session_id):
if not session_id:
# create a new session
while session_id is None or session_id in self.sessions:
- session_id = stringutils.random_string(24)
+ session_id = stringutils.random_string_with_symbols(24)
@erikjohnston

erikjohnston Mar 16, 2016

Owner

Do the session ids get sent to the client? Or they purely internal?

@dbkr

dbkr Mar 16, 2016

Member

They get sent to the client

@erikjohnston

erikjohnston Mar 16, 2016

Owner

Its probably fine, but random_string_with_symbols will return a lot of silly symbols, so I've tended to avoid using them in public APIs (especially for anything that is used as query string params)

@dbkr

dbkr Mar 16, 2016

Member

Yeah, maybe - this one only goes into json so it should be fine, but possibly the extra token space isn't worth it.
@dbkr
Use hs get_clock instead of time.time()

All checks have passed

8 successful checks
@matrixbot
Flake8 + Packaging (Commit) — Build #144 origin/dbkr/idempotent_registration succeeded in 28 sec
Details
@matrixbot
Flake8 + Packaging (Merged PR) — Build finished.
Details
@matrixbot
Sytest Postgres (Commit) — Build #147 origin/dbkr/idempotent_registration succeeded in 5 min 32 sec
Details
@matrixbot
Sytest Postgres (Merged PR) — Build finished.
Details
@matrixbot
Sytest SQLite (Commit) — Build #148 origin/dbkr/idempotent_registration succeeded in 4 min 26 sec
Details
@matrixbot
Sytest SQLite (Merged PR) — Build finished.
Details
@matrixbot
Unit Tests (Commit) — Build #189 origin/dbkr/idempotent_registration succeeded in 1 min 11 sec
Details
@matrixbot
Unit Tests (Merged PR) — Build finished.
Details
742b6c6
@erikjohnston
Owner

erikjohnston commented Mar 16, 2016

We probably do care that session information gets dropped over restarts, but no more so now than before, so I think that would be a separate fix.

Fair enough

dbkr added some commits Mar 16, 2016

@dbkr
Replace other time.time().

All checks have passed

8 successful checks
@matrixbot
Flake8 + Packaging (Commit) — Build #145 origin/dbkr/idempotent_registration succeeded in 26 sec
Details
@matrixbot
Flake8 + Packaging (Merged PR) — Build finished.
Details
@matrixbot
Sytest Postgres (Commit) — Build #148 origin/dbkr/idempotent_registration succeeded in 5 min 20 sec
Details
@matrixbot
Sytest Postgres (Merged PR) — Build finished.
Details
@matrixbot
Sytest SQLite (Commit) — Build #149 origin/dbkr/idempotent_registration succeeded in 4 min 14 sec
Details
@matrixbot
Sytest SQLite (Merged PR) — Build finished.
Details
@matrixbot
Unit Tests (Commit) — Build #190 origin/dbkr/idempotent_registration succeeded in 1 min 10 sec
Details
@matrixbot
Unit Tests (Merged PR) — Build finished.
Details
9671e67
@dbkr
string with symbols is a bit too symboly.

All checks have passed

8 successful checks
@matrixbot
Flake8 + Packaging (Commit) — Build #146 origin/dbkr/idempotent_registration succeeded in 30 sec
Details
@matrixbot
Flake8 + Packaging (Merged PR) — Build finished.
Details
@matrixbot
Sytest Postgres (Commit) — Build #149 origin/dbkr/idempotent_registration succeeded in 5 min 5 sec
Details
@matrixbot
Sytest Postgres (Merged PR) — Build finished.
Details
@matrixbot
Sytest SQLite (Commit) — Build #150 origin/dbkr/idempotent_registration succeeded in 3 min 56 sec
Details
@matrixbot
Sytest SQLite (Merged PR) — Build finished.
Details
@matrixbot
Unit Tests (Commit) — Build #191 origin/dbkr/idempotent_registration succeeded in 1 min 6 sec
Details
@matrixbot
Unit Tests (Merged PR) — Build finished.
Details
3176aeb

@erikjohnston erikjohnston commented on an outdated diff Mar 16, 2016

View full outdated diff
synapse/handlers/auth.py
self.sessions[session["id"]] = session
-
- def _remove_session(self, session):
- logger.debug("Removing session %s", session)
- del self.sessions[session["id"]]
+ self._prune_sessions()
+
+ def _prune_sessions(self):
+ for sid, sess in self.sessions.items():
+ last_used = 0
+ if 'last_used' in sess:
+ last_used = sess['last_used']
+ if last_used < self.hs.get_clock().time() - AuthHandler.SESSION_EXPIRE_MS:
@erikjohnston

erikjohnston Mar 16, 2016

Owner

time_msec() :)

dbkr added some commits Mar 16, 2016

@dbkr
time_msec()

Some checks were not successful

1 errored, 1 failing, and 6 successful checks
@matrixbot
Flake8 + Packaging (Commit) — Build #147 origin/dbkr/idempotent_registration failed in 27 sec
Details
@matrixbot
Flake8 + Packaging (Merged PR) — Build finished.
Details
@matrixbot
Sytest Postgres (Commit) — Build #150 origin/dbkr/idempotent_registration succeeded in 4 min 50 sec
Details
@matrixbot
Sytest Postgres (Merged PR) — Build finished.
Details
@matrixbot
Sytest SQLite (Commit) — Build #151 origin/dbkr/idempotent_registration succeeded in 3 min 41 sec
Details
@matrixbot
Sytest SQLite (Merged PR) — Build finished.
Details
@matrixbot
Unit Tests (Commit) — Build #192 origin/dbkr/idempotent_registration succeeded in 1 min 5 sec
Details
@matrixbot
Unit Tests (Merged PR) — Build finished.
Details
3ee7d7d
@dbkr
pep8

All checks have passed

8 successful checks
@matrixbot
Flake8 + Packaging (Commit) — Build #148 origin/dbkr/idempotent_registration succeeded in 27 sec
Details
@matrixbot
Flake8 + Packaging (Merged PR) — Build finished.
Details
@matrixbot
Sytest Postgres (Commit) — Build #151 origin/dbkr/idempotent_registration succeeded in 5 min 17 sec
Details
@matrixbot
Sytest Postgres (Merged PR) — Build finished.
Details
@matrixbot
Sytest SQLite (Commit) — Build #152 origin/dbkr/idempotent_registration succeeded in 4 min 39 sec
Details
@matrixbot
Sytest SQLite (Merged PR) — Build finished.
Details
@matrixbot
Unit Tests (Commit) — Build #193 origin/dbkr/idempotent_registration succeeded in 1 min 6 sec
Details
@matrixbot
Unit Tests (Merged PR) — Build finished.
Details
b58d10a
@erikjohnston
Owner

erikjohnston commented Mar 16, 2016

LGTM

dbkr added a commit that referenced this pull request Mar 16, 2016

@dbkr
Merge pull request #649 from matrix-org/dbkr/idempotent_registration 
Make registration idempotent

All checks have passed

4 successful checks
@matrixbot
Flake8 + Packaging (Commit) — Build #149 origin/develop succeeded in 27 sec
Details
@matrixbot
Sytest Postgres (Commit) — Build #152 origin/develop succeeded in 5 min 11 sec
Details
@matrixbot
Sytest SQLite (Commit) — Build #153 origin/develop succeeded in 4 min 14 sec
Details
@matrixbot
Unit Tests (Commit) — Build #194 origin/develop succeeded in 1 min 12 sec
Details
48b2e85

@dbkr dbkr merged commit 48b2e85 into develop Mar 16, 2016
8 checks passed

8 checks passed

Flake8 + Packaging (Commit) Build #148 origin/dbkr/idempotent_registration succeeded in 27 sec
Details
Flake8 + Packaging (Merged PR) Build finished.
Details
Sytest Postgres (Commit) Build #151 origin/dbkr/idempotent_registration succeeded in 5 min 17 sec
Details
Sytest Postgres (Merged PR) Build finished.
Details
Sytest SQLite (Commit) Build #152 origin/dbkr/idempotent_registration succeeded in 4 min 39 sec
Details
Sytest SQLite (Merged PR) Build finished.
Details
Unit Tests (Commit) Build #193 origin/dbkr/idempotent_registration succeeded in 1 min 6 sec
Details
Unit Tests (Merged PR) Build finished.
Details

dbkr added a commit to matrix-org/matrix-react-sdk that referenced this pull request Mar 16, 2016

@dbkr
Poll for email validation once the validation email has been sent, an… 
…d continue with the registration process if/when it succeeds.


Fixes vector-im/riot-web#1027
Requires matrix-org/synapse#650 and matrix-org/synapse#649
c9ad370

@dbkr dbkr referenced this pull request in matrix-org/matrix-react-sdk Mar 16, 2016

Merged

Poll for email validation once the validation email has been sent #223

@richvdh richvdh deleted the dbkr/idempotent_registration branch Dec 1, 2016

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