matrix-react-sdk icon indicating copy to clipboard operation
matrix-react-sdk copied to clipboard

Support registration tokens

Open govynnus opened this issue 2 years ago • 6 comments

Would close https://github.com/vector-im/element-web/issues/18931 when complete.

Currently no registration token input is shown on the main registration form as I couldn't get the authentication request to actually send that way. Instead the token is requested after the main form has been submitted.

A screenshot of the registration token form is included below. 2022-05-16_13-05-1652705316

Related PRs for matrix-js-sdk:

  • https://github.com/matrix-org/matrix-js-sdk/pull/2020
  • https://github.com/matrix-org/matrix-js-sdk/pull/2048
  • https://github.com/matrix-org/matrix-js-sdk/pull/2181

Signed-off-by: Callum Brown [email protected]

Notes: Add support for token authenticated registration.


Here's what your changelog entry will look like:

✨ Features

  • Add support for [token authenticated registration](https (#7275). Fixes vector-im/element-web#18931. Contributed by @govynnus.

govynnus avatar Dec 03 '21 10:12 govynnus

If an invalid token is entered, the error message is only displayed briefly before disappearing. I think this is related to the fact that requests with empty auth dicts are constantly sent to the /register endpoint after submitting the main registration form (I assume to detect updates for other UIA types), but haven't figured out how that could be fixed.

govynnus avatar Feb 16 '22 17:02 govynnus

No worries for the wait. Updated to develop and added a screenshot to the description. Thanks.

govynnus avatar Apr 10 '22 12:04 govynnus

Going to send this off for Design review, who might have suggestions for how to improve the UI, as the text doesn't really explain where the token might have come from.

Designer: feel free to poke me for advice/help testing this flow.

turt2live avatar May 11 '22 21:05 turt2live

I've changed it to an AccessibleButton, though I'm not sure it's entirely correct, and I also updated the displayed message.

The screenshot in the description has been updated too.

govynnus avatar May 16 '22 12:05 govynnus

I also started having a look at tests, but got a bit stuck as I don't have anything to copy directly from :)

govynnus avatar Sep 10 '22 18:09 govynnus

Ok, I think that test is alright?

govynnus avatar Sep 10 '22 20:09 govynnus

Hi @govynnus thank you for the ping today and for working on this feature. We discussed this PR in our weekly meeting today. We're really sorry it's taken so long to respond fully.

Because this adds a new feature, and it potentially does it a different way from the direction we are taking with OIDC, we think this needs serious review from our Product team.

I'm adding a label to the linked issue so it should get the attention it deserves from the Product team. Bear in mind that this can take quite some time at the moment. Apologies again.

andybalaam avatar Jan 19 '23 16:01 andybalaam

Hi @govynnus thank you for the ping today and for working on this feature. We discussed this PR in our weekly meeting today. We're really sorry it's taken so long to respond fully.

Hi @andybalaam thanks for your response. I appreciate you all are busy, and I acknowledge that I could have got a response faster if I had made more a nuisance of myself :). I am not overly bothered about the delay, although it has led me to think that Element (as an organisation rather than any individuals) may not care much about community contributions.

Because this adds a new feature, and it potentially does it a different way from the direction we are taking with OIDC, we think this needs serious review from our Product team.

If OIDC can do the same thing registration tokens do, I see no reason (currently) why it can't replace them. However OIDC/OAuth etc. has been talked about for years, and had been talked about for years before I even started working on registration tokens. With https://areweoidcyet.com/ I see some progress is now being made and it looks like it is very likely to happen, however that is still going to take a good while. This is a feature that many people, including myself, have wanted natively in Element (as opposed to UIA fallback) since before this PR was created over a year ago. I wanted it enough to implement it in an unfamiliar project with a tech stack I do not personally enjoy using.

Of course Element is in no way obligated to support this functionality, but given that it is implemented and has been in a released version of the spec for nearly a year, I am slightly surprised that whether to support this or not is a 'product' question. I guess it's a good reminder of the power Element wields in the Matrix ecosystem.

I apologise for the ranty tone, it is not directed at any individual - everyone I have communicated with has been friendly and helpful - but this has quite honestly been my worst experience of contributing to open-source software so far, and I thought that might be worth pointing out.

govynnus avatar Jan 19 '23 19:01 govynnus

Hi @govynnus I agree this is not a good experience, and I can only apologise. I agree that we have not done well with encouraging contributions over recent time, and I hope that will improve this year.

I didn't actually realise this implemented something that is specced - I hope that will help a bit with the product conversation. I'll raise it with a member of the product team now.

andybalaam avatar Jan 20 '23 16:01 andybalaam

Thanks @andybalaam I understand the misunderstanding if that makes sense :)

It's good to know the team is aware of some of these issues and hopefully working on solutions.

govynnus avatar Jan 20 '23 18:01 govynnus

@govynnus our product person wants to try this out. Is there a server they can point it at to see how it looks? They don't need to log in, just get the token UI to appear.

andybalaam avatar Jan 23 '23 11:01 andybalaam

@andybalaam they can use calcuode.com

govynnus avatar Jan 23 '23 13:01 govynnus

Thanks so much for your contribution - apologies that it hasn't been as smooth or as fast as any of us would have liked! I appreciate your patience.

From the product side of things - this looks great. I've had a quick play around with it and I can see how users would benefit from this. Happy to give it the go ahead!

One quick question for you - in the video you can see that when I entered the incorrect token the input field showed an error message... this error message disappears quite quickly - is there anything that can be done to change that behaviour slightly, I'm worried users might miss it...

https://user-images.githubusercontent.com/89144281/214051051-844ed3ed-354b-4f13-8a89-bc8ffe1c3eb2.mov

daniellekirkwood avatar Jan 23 '23 13:01 daniellekirkwood

One quick question for you - in the video you can see that when I entered the incorrect token the input field showed an error message... this error message disappears quite quickly - is there anything that can be done to change that behaviour slightly, I'm worried users might miss it...

@daniellekirkwood I noticed this, but did not work out why, I can have another look.

govynnus avatar Jan 23 '23 14:01 govynnus

I remember now: the error message disappearing is because as soon as one clicks 'Register' on the main registration form element re-tries registration (i.e. the current UIA flow) every 2 seconds or so, and therefore the error message is cleared. I didn't have much luck figuring it out before but I could try again. The problem is external to this PR though.

govynnus avatar Jan 23 '23 18:01 govynnus

Yes, it's less than ideal as a user experience but not enough to outweigh the pro's of the solution included already.

daniellekirkwood avatar Jan 24 '23 09:01 daniellekirkwood

Thanks again @govynnus ! This should be in the release after next. (Probably v1.11.22)

andybalaam avatar Jan 24 '23 12:01 andybalaam

Thanks @andybalaam :-)

govynnus avatar Jan 24 '23 12:01 govynnus

💚

ShadowJonathan avatar Jan 24 '23 12:01 ShadowJonathan