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

Optionally include password hash in createUser endpoint #905

Merged
merged 3 commits into from Jul 4, 2016

Conversation

Projects
None yet
3 participants
Contributor

KentShikama commented Jul 3, 2016

I'm working on a potential application service called Diaspora, a decentralized social networking site, which I would like to register Matrix users from. The idea is that Vector would be embedded in Diaspora as attached below. However I would also like any Diaspora user to login to their Matrix accounts through Vector or any other clients that are made in the future using their Diaspora credentials.

Matthew brought up the concern in matrix-dev that this would couple bcrypt to Synapse. My response is that if bcrypt became insecure for some reason, then it would mean that Diaspora would also have to change hashing algorithms too, and thus there wouldn't be much concern about making breaking changes. I would also not mind simply having this optional argument marked as unstable - as in Synapse will be free to make breaking changes whenever they want in terms of hashing algorithms - as it would be much easier than maintaining my own fork.

vector

Can one of the admins verify this patch?

Can one of the admins verify this patch?

Can one of the admins verify this patch?

Can one of the admins verify this patch?

Can one of the admins verify this patch?

Optionally include password hash in createUser endpoint
Signed-off-by: Kent Shikama <kent@kentshikama.com>

@erikjohnston erikjohnston commented on an outdated diff Jul 4, 2016

synapse/rest/client/v1/register.py
@@ -410,12 +410,14 @@ def _do_create(self, user_json):
raise SynapseError(400, "Failed to parse 'duration_seconds'")
if duration_seconds > self.direct_user_creation_max_duration:
duration_seconds = self.direct_user_creation_max_duration
+ password_hash = user_json["password_hash"].encode("utf-8") if user_json["password_hash"] else None
@erikjohnston

erikjohnston Jul 4, 2016

Owner

You probably want to do:

... if user_json.get("password_hash") else None

So that it doesn't explode if no password_hash key is present.

Owner

erikjohnston commented Jul 4, 2016

@matrixbot ok to test

(I really wish that jenkins wasn't quite so spammy)

Owner

erikjohnston commented Jul 4, 2016 edited

The two "correct" ways of doing this would be:

  1. add a custom login flow that would allow matrix clients to delegate login to Diaspora (clients that didn't support it would then use the fallback mechanism), or
  2. add a config option that allowed synapse to query a separate API to test password credentials

Both of those would be non-trivial things to add, though 2. could potentially be doable. (In the future the plan is to support a more pluggable/modular auth system.)

However, given this is a restricted AS API, I'm happy enough to accept this PR.

Owner

erikjohnston commented Jul 4, 2016

You have a couple of code style violations: http://matrix.org/jenkins/job/SynapseFlake8Packaging/621/violations/

Looks like they're just lines that are too long; we prefer lines < 80 and require lines to be < 90.

Fix style violations
Signed-off-by: Kent Shikama <kent@kentshikama.com>
Contributor

KentShikama commented Jul 4, 2016 edited

Yeah 2. would be ideal indeed. Diaspora already does something like that with a Prosody (XMPP) mod: a couple hundred lines of lua code I believe.

Thanks for allowing this!

Owner

erikjohnston commented Jul 4, 2016

np!

@erikjohnston erikjohnston merged commit 3de8168 into matrix-org:develop Jul 4, 2016

5 checks passed

Flake8 + Packaging (Merged PR) Build finished.
Details
Sytest Dendron (Merged PR) Build finished.
Details
Sytest Postgres (Merged PR) Build finished.
Details
Sytest SQLite (Merged PR) Build finished.
Details
Unit Tests (Merged PR) Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment