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

Make registration idempotent #649

Merged
merged 9 commits into from
Mar 16, 2016
Merged

Make registration idempotent #649

merged 9 commits into from
Mar 16, 2016

Conversation

dbkr
Copy link
Member

@dbkr 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 element-hq/element-web#957

…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.
@@ -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()
Copy link
Member

Choose a reason for hiding this comment

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

Why not use hs.get_clock()?

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

@erikjohnston
Copy link
Member

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

@dbkr
Copy link
Member Author

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.

@@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

They get sent to the client

Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, maybe - this one only goes into json so it should be fine, but possibly the extra token space isn't worth it.

@erikjohnston
Copy link
Member

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

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:
Copy link
Member

Choose a reason for hiding this comment

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

time_msec() :)

@erikjohnston
Copy link
Member

LGTM

dbkr added a commit that referenced this pull request Mar 16, 2016
@dbkr dbkr merged commit 48b2e85 into develop Mar 16, 2016
dbkr added a commit to matrix-org/matrix-react-sdk that referenced this pull request Mar 16, 2016
@richvdh richvdh deleted the dbkr/idempotent_registration branch December 1, 2016 14:09
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