Skip to content
New issue

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

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

Already on GitHub? # to your account

Username validation #11739

Merged

Conversation

nick2432
Copy link
Contributor

@nick2432 nick2432 commented Jan 17, 2024

Summary

  • Modified the FacilitySearchUsernameViewSet to use username__iexact for case-insensitive username search.
  • Updated the SET_FIRST_LOD action in the handleSubmit method to use the username from the task.extra_metadata instead of the component's this.username. This change ensures that the correct username is set when initializing the firstImportedLodUser in the wizard context

References

Fixes #11540

Screenshots

I created a user "cincO" and logged in with the username "cinco," and it worked
Uploading 2024-01-19 16-33-50.mp4…

Reviewer guidance


Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@github-actions github-actions bot added DEV: backend Python, databases, networking, filesystem... SIZE: very small labels Jan 17, 2024
Copy link
Member

@jredrejo jredrejo left a comment

Choose a reason for hiding this comment

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

Hello @nick2432 I don't think that DeviceProvisionSerializer is the proper place to do this. In the related issue this PR tries to fix there's a comment from @rtibbles
suggesting to do it in the public api that responds with the data of the username if it exists https://github.com/learningequality/kolibri/blob/release-v0.16.x/kolibri/core/public/api.py#L360
That's a safer place in the code (it does not create anything) and covers a more general case than the device provisioning (more users could be synced in the future not being superusers so you would not find them)

@nick2432 nick2432 requested a review from jredrejo January 18, 2024 10:08
Copy link
Member

@jredrejo jredrejo left a comment

Choose a reason for hiding this comment

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

Hello @nick2432
I'm afraid this is not working properly because the frontend is not using the username info this api provides, but the username the user typed in the text input. For this reason, the last post the frontend does to the server is not using the right capitalization but the wrong the user typed.

This is the last endpoint the setup uses:
/api/device/deviceprovision/

in my tests, the username was cincO and with your api changes, it's correctly returned, but you can see the payload of the endpoint POST has not it corrected:
{"facility_id":"440eb55977625f4e82f1e9eef75769db","superuser":{"username":"cinco","password":"NOT_SPECIFIED"},"settings":{"on_my_own_setup":false},"preset":"nonformal","language_id":"en","device_name":"ddd","allow_guest_access":false,"is_provisioned":true,"is_soud":true}
So this is is the response to the browser:
{"detail":"username, password, or full_nameare missing insuperuser"}

I understand this is the reason why you modified the provisioning serializer first, but, as previously mentioned, that didn't solve a general case. I'm afraid this issue requires both backend and frontend work to be finished. Please let us know if you need help with the frontend or you can achieve it.
Thanks!

@nick2432
Copy link
Contributor Author

nick2432 commented Jan 19, 2024

@jredrejo ,
I will try it.

@github-actions github-actions bot added APP: Setup Wizard Re: Setup Wizard (facility import, superuser creation, settings, etc.) DEV: frontend labels Jan 19, 2024
@nick2432 nick2432 requested a review from jredrejo January 19, 2024 10:57
@nick2432
Copy link
Contributor Author

Hey, @jredrejo , please check out my latest changes.

Copy link
Member

@jredrejo jredrejo left a comment

Choose a reason for hiding this comment

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

Great work @nick2432

@jredrejo jredrejo merged commit fc94f27 into learningequality:release-v0.16.x Jan 22, 2024
33 of 34 checks passed
@nick2432 nick2432 deleted the username-validation branch January 23, 2024 06:05
@pcenov
Copy link
Member

pcenov commented Jan 23, 2024

Hi @nick2432 and @jredrejo I just tried this and I am still getting an error when trying to import a learner with the incorrect letter case in the username. My question is - since this was merged without requiring any QA verification - is this a partial fix of the issue described in #11540?

2024-01-23_15-46-57.mp4

@jredrejo
Copy link
Member

Hi @nick2432 and @jredrejo I just tried this and I am still getting an error when trying to import a learner with the incorrect letter case in the username. My question is - since this was merged without requiring any QA verification - is this a partial fix of the issue described in #11540?

2024-01-23_15-46-57.mp4

@pcenov do you mean you want the password to be accepted when capitalization does not match? that's not possible. This PR fixes username validation when capitalization does not match.
In this new screencast I see other error messages that are not the described in #11540. If there are other validation issues that are not included in the validation follow up issue #11711 , please add them there, so everything can be done at once.

@nick2432
Copy link
Contributor Author

Hi @nick2432 and @jredrejo I just tried this and I am still getting an error when trying to import a learner with the incorrect letter case in the username. My question is - since this was merged without requiring any QA verification - is this a partial fix of the issue described in #11540?

2024-01-23_15-46-57.mp4

Hey @pcenov , I believe it's not a case issue. It's not working with both capital and lowercase letters; every username is encountering an error.

2024-01-23.22-34-13.mp4

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
APP: Setup Wizard Re: Setup Wizard (facility import, superuser creation, settings, etc.) DEV: backend Python, databases, networking, filesystem... DEV: frontend SIZE: very small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setup Wizard > LOD > Import user - Missing validation for uppercase in the usernames and passwords
3 participants