Projects tigase _server tigase-http-api Issues #156
[Dashboard] Add form validation for fields (username) (#156)
Wojciech Kapcia (Tigase) opened 9 months ago

It may be tempting to put whole JID in username which is invalid and yields server error - it would be good idea to add constrains (regexp) to prevent it.

  • Wojciech Kapcia (Tigase) added "Related" tygrys/tygrys#245 9 months ago
  • Andrzej Wójcik (Tigase) commented 3 weeks ago

    @wojtek What validation you have in mind? It is possible to add regex in the field in HTML. Regex for parameter validation is also there when parameters are decoded. In this case, we accept string, so there is no validation - it is later done in code. If validation exception would be thrown upon catching TigaseStringPrepException it would display error page (after changes from #155) with correct error details (message in ValidationException would need to indicate cause of the error and field that caused it).

    Alternatively, we could add some custom type (ie. Localpart) that would provide correct validation. On the other hand, I wonder if we shouldn't do escaping in the form (replacing @ with a correct %xx value).

  • Andrzej Wójcik (Tigase) changed state to 'In Progress' 3 weeks ago
    Previous Value Current Value
    Open
    In Progress
  • Wojciech Kapcia (Tigase) commented 3 weeks ago

    What validation you have in mind? It is possible to add regex in the field in HTML.

    Yes, this is what I had in mind. (i.e. https://developer.mozilla.org/en-US/docs/Learn_web_development/Extensions/Forms/Form_validation#validating_against_a_regular_expression) This way user would have immediate feedback if there is an error in the field without submitting invalid input.

    Alternatively, we could add some custom type (ie. Localpart) that would provide correct validation. On the other hand, I wonder if we shouldn't do escaping in the form (replacing @ with a correct %xx value).

    Not sure if that would be the best option as it would most likely lead to confussion.

  • Andrzej Wójcik (Tigase) commented 3 weeks ago

    I've worked on a few improvements to resolve this issue and be able to make it easier in the future as well.

    I've added validation mechanism and improved parsing/conversation mechanism, so that we could report all issues related to conversion and validation of parameters in the method on a single error page, ie. you can annotate a String parameter with @JidLocalpart and method validator will report an error if provided parameter cannot be used as JID localpart presenting message that you can optionally specify in message parameter of @JidLocalpart annotation.

    The same will now be done for @NotNull, @NotEmpty, @NotBlank, @Pattern. I've decided to create @JidLocalpart annotation to not have to write @Pattern(message = "is not a valid username", regexp = "^[^ @&()\\[\\]\\t\\n\\r\\f\\a\\e]*$") on every field that should be verified if its value is a valid localpart of a JID.

    If many field will fail parsing or validation, all errors would be returned at once to make debugging (resolving issues) easier as it would indicate all issues at once. For that I've modified conversion mechanism that was throwing an error after the first encountered conversion issue to now record them and report them all at once.

    That connected with improved error reporting from #155 should improve user experience.

    I've also added regex in HTML (pattern attribute) to validate username in creation form early (in the browser just after user presses Create button).

  • Andrzej Wójcik (Tigase) commented 3 weeks ago

    I think that with those changes, it will resolve the issue which originally was mentioned but should also improve error handling and validation in other places.

  • Andrzej Wójcik (Tigase) changed state to 'In QA' 3 weeks ago
    Previous Value Current Value
    In Progress
    In QA
  • Andrzej Wójcik (Tigase) changed fields 3 weeks ago
    Name Previous Value Current Value
    Assignee
    andrzej.wojcik
    wojtek
  • Wojciech Kapcia (Tigase) commented 3 weeks ago

    Looks OK. There is a small issue under Firefox but IMHO this is Firefox bug (will maybe report it)

  • Wojciech Kapcia (Tigase) changed state to 'Closed' 3 weeks ago
    Previous Value Current Value
    In QA
    Closed
issue 1 of 1
Type
Improvement
Priority
Normal
Assignee
Version
none
Sprints
n/a
Customer
n/a
Iterations
Issue Votes (0)
Watchers (3)
Reference
tigase/_server/tigase-http-api#156
Please wait...
Page is in error, reload to recover