Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

XWIKI-22121: Improve the registration experience #3155

Open
wants to merge 26 commits into
base: master
Choose a base branch
from

Conversation

Sereza7
Copy link
Contributor

@Sereza7 Sereza7 commented Jun 5, 2024

Jira URL

https://jira.xwiki.org/browse/XWIKI-22121

Changes

Description

  • Part A: Make the registration immediately visible: changed the position of the registration login/logout buttons in the UI.
  • Part B: Improve live validation (especially for the password)
  • Part C: Improved the default page displayed after a successful registration

Clarifications

  • Extra details in the jira ticket.
  • The most tricky part was the overhaul of livevalidation. Before, validation was handled by field. In order to display a status that's more detailed, I needed to increase granularity and keep a state based on each validation rule for each field. Trying to keep backwards compatibility was a bit challenging and would explain the high amount of safeguards I used in the code.

Screenshots & Video

2024-06-04.13-58-09.mp4

(note that there was a small bug with the display name after the successful registration on the video, it's been fixed since this video has been recorded)

On the video, we can see, in this order:

  • The page location element (based on livevalidation) is still completely functional -- testing backwards compatibility
  • The logged in UI is the same as before.
  • The guest UI does not have anything in the drawer anymore. The login and register buttons are in the top right navbar instead.
  • The form for registration now has a better support. Every validation has its own message.
  • When the account is created, the new UI with a welcome picture is displayed.

Executed Tests

Successfully built xwiki-platform-administration with the quality, integration-tests and docker profiles. Built xwiki-platform-flamingo-skin too. The only fails left are already here on the CI (without the changes from this branch).
Manual tests, see video. For backward compatibility of the livevalidation feature, I tested on the page location form (no changes to this form, but everything worked properly still). Note that it would be good to test more backward compatibility with a couple other forms, but AFAIK this is the only other use of livevalidation_prototype.js in XWiki standard.

Expected merging strategy

  • Prefers squash: Yes
  • Backport on branches:
    • None. The changes proposed here are pretty massive and can wait on the master branch until the next stable release.

Sereza7 and others added 20 commits April 29, 2024 10:18
* Added the `back to home` button.
* Fixed the style of the

TODO: * replace the inline styling with some CSS
* Moved the login and register buttons from the drawer to the navbar.
* Updated style
* Part B: moved around the first and last name fields.

TODO: * C. replace the registration confirmation inline styling with some CSS
* Split the form into two sections
* Added an `About you` title for the second section.

TODO: * C. replace the registration confirmation inline styling with some CSS
* Split the validation messages in multiple parts - WIP

TODO: * C. replace the registration confirmation inline styling with some CSS
* Split the validation messages in multiple parts - Done.

TODO: * C. replace the registration confirmation inline styling with some CSS
* B. Check that other uses of Livevalidation are not broken by the changes.
* Replaced the registration confirmation inline styling with some CSS
* Escaped the content added in RegistrationConfig

TODO:
* B. Check that other uses of Livevalidation are not broken by the changes.
* Took care of a fallback for backwards compatibility
* Tried to fix the register form not submitting, WIP
* Fixed a wrong ID for the UIExtensionClass
* Fixed the bug for unexpected lack of form submission
* Added comments
* Removed special rule for regex type validation in the JS initialization
* Updated the template for creating a validationContainer to fit loading the form with values already given
* Improved backwards compatibility
(WIP)
* Updated the style of `mandatory` and `must-match` fields when valid to never show the `Ok` text. (wasn't an issue before, because the text was left empty as a mistake)
* fixed booleans for validation
* fixed boolean for the welcome message formatting.
* Fixed the base page object register and login function to fit the new position of those links.
@Sereza7 Sereza7 changed the title Xwiki 22121 XWIKI-22121: Improve the registration experience Jun 5, 2024
@manuelleduc
Copy link
Contributor

@Sereza7 since you are working on live validation, I just saw https://jira.xwiki.org/browse/XWIKI-9797, could it be fixed by your change?

@Sereza7
Copy link
Contributor Author

Sereza7 commented Jun 6, 2024

I wasn't aware of this. Since I started working on the project, I wasn't aware that there were livevalidation for usernames or page names repeating. From what I could see when checking those livevalidations, there is nothing about this.

My guess is that livevalidation is client-side, and we don't want to have the whole list of usernames or pages on the client. This could be a performance and security issue at least.

Validation is done server side but I don't think livevalidation has anything to do with it.

This issue should probably be closed as Won't Fix because I don't see it being fixed soon. Technically I don't understand how we'd make it work without some massive drawbacks.
AFAICS, for duplicate usernames, there isn't any message anymore. This is a regression introduced by this PR. I'll provide a fix soon. No issue about the page duplicates.
When submitting the form, the same page is returned, but the username field is emptied.

@Sereza7 Sereza7 marked this pull request as draft June 6, 2024 10:06
@michitux
Copy link
Contributor

michitux commented Jun 6, 2024

There is no reason why live validation couldn't check if the user/page already exists with a call to the server. We don't hide if a username exists or not, from an attacker's point there is no difference if this check is done after clicking submit or before as the attacker could automate both calls.

@tkrieck
Copy link
Contributor

tkrieck commented Jun 6, 2024

From the video, it was not clear to me if all the password requirements are visible from the start. It's an important point for someone thinking of a password in the beginning.

Also, we can reduce the image size, on my mockups I used 200px. If that's still too big, meaning the user has to scroll to see the buttons, then maybe we will change the order of elements, first the message (Welcome), then the image.

* Fixed the username duplicate validation message
* Fixed the size of the hero image on successful registration
@Sereza7
Copy link
Contributor Author

Sereza7 commented Jun 7, 2024

@michitux

There is no reason why live validation couldn't check if the user/page already exists with a call to the server. We don't hide if a username exists or not, from an attacker's point there is no difference if this check is done after clicking submit or before as the attacker could automate both calls.

I'll consider this issue for a BFD then 👍
Don't we have something to prevent spam on the subscription form submission? I'm pretty sure our login system can support blocking a user if there's too many mistakes in a row but I'm not sure about the registration one.


@tkrieck

From the video, it was not clear to me if all the password requirements are visible from the start. It's an important point for someone thinking of a password in the beginning.

The password requirements are visible from the start. On this video I used the default policy, where we ask for at least 6 characters for a password. What's not visible at the start though are error message for Required fields (since there's already text next to the field name to display that :) )
Here is another video with more focus on the password validation:

2024-06-07.15-31-18.mp4

Also, we can reduce the image size, on my mockups I used 200px. If that's still too big, meaning the user has to scroll to see the buttons, then maybe we will change the order of elements, first the message (Welcome), then the image.

Note that my screen is halfway covered by the inspector, so the display is unusually horizontal. I just checked and there's no need to scroll down with my screen. I quickly checked with standard phone/tablet resolutions and it's okay too. The only place I saw this scrolling issue is for the CI tests, they use a square window with relatively low resolution and the buttons are just a bit too low.

I improved it in 683ccd5 :
afterChanges
I decided to go with a height of half the viewport height which has the advantage of being a bit more resilient to viewport size than the default auto sizing we had or 200px.


AFAICS, for duplicate usernames, there isn't any message anymore. This is a regression introduced by this PR. I'll provide a fix soon. No issue about the page duplicates.
When submitting the form, the same page is returned, but the username field is emptied.

Done in 683ccd5 👍
PR ready for reviews.

@Sereza7 Sereza7 marked this pull request as ready for review June 7, 2024 13:37
* Updated the version for deprecation
@Sereza7 Sereza7 marked this pull request as draft August 26, 2024 16:37
@Sereza7 Sereza7 marked this pull request as ready for review September 11, 2024 15:19
@@ -295,7 +296,7 @@
},
'noReturn' : true
})
#set($discard = $fields.add($field))
#set($discard = $aboutYouFields.add($field))
Copy link
Member

Choose a reason for hiding this comment

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

It doesn't feel accurate to put the captcha in the about you fields, also you didn't provided any screenshot for registration with captcha?

@@ -487,6 +505,16 @@
##
#end## createUser Macro
{{/velocity}}</content>
<attachment>
<filename>undraw_done_re_oak4.svg</filename>
Copy link
Member

Choose a reason for hiding this comment

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

Small detail but would be better to have a name more understandable.

#if ($xcontext.user == 'XWiki.XWikiGuest' &amp;&amp; !$xcontext.inactiveUserReference &amp;&amp; $xwiki.hasAccessLevel('register', 'XWiki.XWikiPreferences'))
&lt;li&gt;
&lt;a href="$xwiki.getURL('XWiki.XWikiRegister', 'register', "xredirect=$escapetool.url($xwiki.relativeRequestURL)")" id="tmRegister" rel="nofollow"&gt;$escapetool.xml($services.localization.render('register'))&lt;/a&gt;
&lt;/li&gt;
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering the impact of this for private wikis, you could have a wiki entirely private in which registration is still possible no? In which case here we're loosing seeing the register link at all, isn't it?

#set($userLink = $xwiki.getUserName("$userSpace$userName"))
{{info}}$message.replace('USERLINK', "{{html clean=false}}$userLink{{/html}}"){{/info}}</registrationSuccessMessage>
#set($successAndLogin = $services.localization.render('core.register.successful.successandlogin'))
[[image:undraw_done_re_oak4.svg||data-xwiki-image-style-alignment="center" height="50vh"]]
Copy link
Member

Choose a reason for hiding this comment

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

Any reason for not using directly some html to put in the html macro for that one?

#set($successAndLogin = $services.localization.render('core.register.successful.successandlogin'))
[[image:undraw_done_re_oak4.svg||data-xwiki-image-style-alignment="center" height="50vh"]]

{{html clean="true"}}
Copy link
Member

Choose a reason for hiding this comment

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

clean="true" is the default AFAIK


{{html clean="true"}}
&lt;div class="registration-success-headline"&gt;
&lt;h2&gt;$headline &lt;/h2&gt;
Copy link
Member

Choose a reason for hiding this comment

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

Probably need some escaping

@@ -19,7 +19,7 @@
## ---------------------------------------------------------------------------
## Defines what server generated error messages should look like
## The error message when a field is entered incorrectly
#set ($failureMessageParams = {'class': 'LV_validation_message LV_invalid'})
#set ($failureMessageParams = {'class': 'LV_invalid'})
Copy link
Member

Choose a reason for hiding this comment

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

Why making that change? And you're sure it doesn't impact any test?

#foreach ($regex in $field.validate.regexes)
#set($extraValidationClass = 'regex-'+ $foreach.count)
#set($validation = $regex)
$validationContainer
Copy link
Member

Choose a reason for hiding this comment

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

Honestly it's not easy to review all this, I really think at a point you should get rid of all that and introduce a proper java API for this.

this.options.insertAfterWhatNode.up().appendChild(validationMessageHolder);
}
/* We add one validation object to the list of validations for the current field. */
this.validations.push( { type: validationFunction, params: validationParamsObj || {}, messageHolder: validationMessageHolder } );
Copy link
Member

Choose a reason for hiding this comment

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

Hmmm we're suppose to try to migrate out of prototype... a bit a pity to add new code using it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants