Add supplemental regex setting to validate username

[mishaschwartz]

Create an additional settings/environment variable MAGPIE_SUPPLEMENTAL_USERNAME_REGEX that acts as an additional check for whether a username is valid. This creates a further restriction on this value which is useful when there are additional limits on the username that should be enforced by Magpie.

Resolves #497

[mishaschwartz]

@fmigneault I'd like to add some tests to this as well.

I was unable to find the current tests that check the user creation routes. I'm probably just looking in the wrong place. Any help would be greatly appreciated.

[fmigneault]

3.37 instead

[tlvu]

Is this msg_on_fail error message displayed also to the web UI or only in the logs?

Can I have a sample content of this msg_on_fail error message? Is is understandable by someone not well versed with Magpie, like a new node admin.

[mishaschwartz]

The value is defined here: https://github.com/mishaschwartz/Magpie/blob/a1bd0e15c88d353df51f5ca7988e7ffbc34d202f/magpie/api/schemas.py#L1961C1-L1961C57

and says: "Invalid 'user_name' value specified."

[tlvu]

Right, it does not say "why" and there is no way to add supplemental info that the reason is the new regex. I hope the node admin will be able to guess it.

[mishaschwartz]

and there is no way to add supplemental info that the reason is the new regex

I can make or respond with a different message. How about: "Invalid 'user_name' specified. Does not match the supplemental user name regex."

[tlvu]

Sure that would be better if it does not take to much of your time. Creating a new sub-class with a new hardcoded message?

[fmigneault]

msg_on_fail is only the generic message value in the API body.
For the "why", that would be reported in the API response body. It provides the param_name, param_compare, the input value and so on with more details about the specific check that failed.

On the UI, it would simply be a red message next to the input text field because the contents are limited in the code. That could be improved.

[fmigneault]

The message in the UI I mentioned is here:

Magpie/magpie/ui/management/templates/add_user.mako

Lines 39 to 41 in 92ff2d2

	%if invalid_user_name:
	${reason_user_name}
	%endif

It uses the property obtained from this:

Magpie/magpie/ui/management/views.py

Lines 87 to 88 in 92ff2d2

	@view_config(route_name="add_user", renderer="templates/add_user.mako")
	def add_user(self):

Which default to this:

Magpie/magpie/ui/utils.py

Lines 304 to 308 in 92ff2d2

	# plain message 'Invalid' used as default in case pre-checks did not find anything, but API returned 400
	"reason_user_name": "Invalid",
	"reason_group_name": "Invalid",
	"reason_user_email": "Invalid",
	"reason_password": "Invalid",

That value could be overridden with more explicit details according to the contents parsed from the API response obtained here before returning the UI response:

Magpie/magpie/ui/management/views.py

Lines 106 to 108 in 92ff2d2

	return_data = self.create_user(return_data)
	if return_data["is_error"]:
	return self.add_template_data(return_data)

[mishaschwartz]

@fmigneault @tlvu with the current update to msg_on_fail the UI looks like this:

Is this sufficient or are there other changes you would suggest?

[fmigneault]

Still need rename of the variable/setting to MAGPIE_USER_NAME_EXTRA_REGEX.
This is to align with other variable names, such as MAGPIE_USER_NAME_MAX_LENGTH, for similar checks.

[fmigneault]

@mishaschwartz

I was unable to find the current tests that check the user creation routes. I'm probably just looking in the wrong place. Any help would be greatly appreciated.

Should be in https://github.com/Ouranosinc/Magpie/blob/master/tests/interfaces.py.
Classes in there are reused by https://github.com/Ouranosinc/Magpie/blob/master/tests/test_magpie_api.py and https://github.com/Ouranosinc/Magpie/blob/master/tests/test_magpie_ui.py for various local/remote admin/public testing combinations.

[tlvu]

LGTM, thanks to both of you. This would relieve new users from a long time annoyance.

[fmigneault]

Just a few editorial fixes to apply. Code feature looks good.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (36208d8) 80.84% compared to head (436c7f9) 80.92%.
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #592      +/-   ##
==========================================
+ Coverage   80.84%   80.92%   +0.07%     
==========================================
  Files          73       73              
  Lines       10188    10194       +6     
  Branches     1823     1824       +1     
==========================================
+ Hits         8236     8249      +13     
+ Misses       1630     1622       -8     
- Partials      322      323       +1     

Files	Coverage Δ
magpie/api/management/user/user_utils.py	97.23% <100.00%> (+0.02%)	⬆️
magpie/api/schemas.py	99.69% <100.00%> (+<0.01%)	⬆️

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[fmigneault]

@mishaschwartz Thanks for the PR and tests! https://github.com/Ouranosinc/Magpie/tree/3.37.0 pushed 🚀