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

Fix incorrect maximum length of webadmin and webclient form fields #539

Merged
merged 4 commits into from
Mar 12, 2024

Conversation

chris-allan
Copy link
Member

Fixes incorrect maximum length of webadmin and webclient form fields. Having incorrect lengths causes scenarios where certain strings are truncated on save or action when used with these forms.

@chris-allan chris-allan force-pushed the remove-length-restrictions branch from 1e9cb7e to 4a4d31c Compare March 6, 2024 10:26
Copy link
Member

@sbesson sbesson left a comment

Choose a reason for hiding this comment

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

One question apart, all the updated length limitations in OMERO.web now match the size limits in the database with the current schema, namely:

  • Experimenter.omeName, Experimenter.firstname, Experimenter.middlename, Experimenter.lastname, Experimenter.email, Experimenter.institution are columns with a VARCHAR(255) data type
  • all container names e.g. Project.name, Dataset.name are columns with a TEXT data type
  • LogicalChannel.name is a column with a VARCHAR(255) data type
  • LogicalChannel.fluor is a column with a TEXT data type
  • Dichroic.lotnumber, Dichroic.serialNumber, Dichroic.manufacturer, Dichroic.model are columns with a VARCHAR(255) data type
  • Microscope.lotnumber, Microscope.serialNumber, Microscope.manufacturer, Microscope.model are columns with a VARCHAR(255) data type
  • Objective.lotnumber, Objective.serialNumber, Objective.manufacturer, Objective.model are columns with a VARCHAR(255) data type
  • Filter.lotnumber, Filter.serialNumber, Filter.manufacturer, Filter.model, Filter.filterwheel` are columns with a VARCHAR(255) data type
  • Detector.lotnumber, Detector.serialNumber, Detector.manufacturer, Detector.model are columns with a VARCHAR(255) data type
  • Lightsource.lotnumber, Lightsource.serialNumber, Lightsource.manufacturer, Lightsource.model are columns with a VARCHAR(255) data type

Finally Password.hash is a column with a VARCHAR(255) data type but that only applies to experiments which password is managed in the database. For all external authentication mechanisms like LDAP, this restriction does not necessary apply so dropping completely the max_length imposed in the OMERO.web form is the correct approach.

omeroweb/webadmin/forms.py Show resolved Hide resolved
@chris-allan
Copy link
Member Author

One question apart

Did you have a question beyond the query about the username field in ForgottonPasswordForm?

Finally Password.hash is a column with a VARCHAR(255) data type but that only applies to experiments which password is managed in the database.

Sort of. Its length is irrelevant regardless of the password source as the hash is of fixed length regardless of the size of the input data being hashed.

Copy link
Member

@sbesson sbesson left a comment

Choose a reason for hiding this comment

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

Did you have a question beyond the query about the username field in ForgottonPasswordForm?

No, the question on the ForgottenPassword form username length was my only outstanding question.

Sort of. Its length is irrelevant regardless of the password source as the hash is of fixed length regardless of the size of the input data being hashed.

True i.e. Yet-another-reason to lift all length restrictions imposed on the user password.

From my side, all looks good from a code review.

Copy link
Member

@will-moore will-moore left a comment

Choose a reason for hiding this comment

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

I wasn't sure if this needs any functional testing (or what's the easiest way to show the original errors)? If you exceed max_length when filling in a form, the form is invalid, so that should fail at validation (if we do it) rather than populating the DB with truncated values?

But the code changes look good. 👍

@chris-allan
Copy link
Member Author

I wasn't sure if this needs any functional testing (or what's the easiest way to show the original errors)?

The easiest way to see it is to create something like this:

omero obj new Project name=$(python -c 'print("a" * 250 + "b", end="")')

You will now not be able to modify the name of this object in due to form validation. However, since the field actually has 251 characters on the client, if you copy and paste the name back into the name field or into another name field it will be silently truncated. There are lots of other weird examples if you modify text in the middle of the long string, etc.

The worst example is the password field where you cannot actually see what's typed in, people are used to visually truncated password fields, and silent truncation happens underneath on paste.

@will-moore
Copy link
Member

Thanks. Tested locally with this branch and could update e.g. Project name to a string longer than 250 characters.
Editing the same project when I reverted to master branch wasn't possible with the full name. Looks good.

@will-moore
Copy link
Member

This seems to be included in merged PRs today at https://merge-ci.openmicroscopy.org/jenkins/job/OMERO-python-superbuild-push/478/console but I'm not seeing it at https://merge-ci.openmicroscopy.org/web/webclient/
E.g. Edit Project <input> still has maxlength="250". Something up with the build...?

@knabar knabar added this to the 5.25.0 milestone Mar 12, 2024
@will-moore
Copy link
Member

This is being deployed on merge-ci now and is working fine.

@knabar knabar merged commit 61f19bb into ome:master Mar 12, 2024
10 checks passed
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.

4 participants