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

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

Closed
pcenov opened this issue Nov 17, 2023 · 25 comments · Fixed by #11739
Assignees
Labels
P1 - important Priority: High impact on UX

Comments

@pcenov
Copy link
Member

pcenov commented Nov 17, 2023

Observed behavior

Initially I thought that the import of a learner from the server to the LOD is not functioning anymore after the latest changes in #11525. In reality both the username and the password of the learner I was trying to import were in uppercase while I was inputting them in lowercase. Therefore the issue is about the proper validation for uppercase.

2023-11-17_15-49-30.mp4

Expected behavior

The user should see validation for incorrect username and password. This should be fixed for both the learner credentials form and the admin credentials form.

Steps to reproduce the issue

  1. Install the latest Beta 8 build asset.
  2. Setup a server device with learners and assigned lessons.
  3. On a separate device start the setup wizard and attempt to import an individual learner through the learn-only device user flow by inputting either the password or the username in incorrect letter case.

Logs

https://drive.google.com/drive/folders/1OImtokSocwqMa84Yv4acDRn8e8uylv91

Usage Details

Ubuntu 22 - Chrome, Android 13

@pcenov
Copy link
Member Author

pcenov commented Nov 17, 2023

@radinamatic @bjester

@marcellamaki marcellamaki added the P0 - critical Priority: Release blocker or regression label Nov 17, 2023
@bjester
Copy link
Member

bjester commented Nov 17, 2023

@pcenov The error in the logs indicates an authentication failure. Although, that failure isn't handled properly and creates an error of its own. @marcellamaki this seems similar to what we experienced last Friday

@pcenov
Copy link
Member Author

pcenov commented Nov 17, 2023

@bjester thanks for pointing that out! After further investigation this error is actually caused by me inputting the password of the learner with small letters instead of capital letters... sigh

@marcellamaki
Copy link
Member

Good to know the import isn't totally broken, but this could definitely still use some improved error handling, as with the other issue we ran into @bjester that you mentioned. I'll see if I can find some strings that we might be able to repurpose here, and we can rescope the issue to focus on that.

@pcenov
Copy link
Member Author

pcenov commented Nov 17, 2023

Also if I enter the password with the correct upper case but l enter the username in small case I get this error which we've seen before but it was not clear how to replicate it:

{ "data": { "detail": "`username`, `password`, or `full_name` are missing in `superuser`" }, "status": 400, "statusText": "Bad Request", "headers": { "allow": "POST, OPTIONS", "content-length": "78", "content-type": "application/json", "date": "Fri, 17 Nov 2023 15:14:50 GMT", "server": "Cheroot/8.6.0", "vary": "Accept, Cookie", "x-content-type-options": "nosniff", "x-frame-options": "SAMEORIGIN" }, "config": { "transitional": { "silentJSONParsing": true, "forcedJSONParsing": true, "clarifyTimeoutError": false }, "adapter": [ "xhr", "http" ], "transformRequest": [ null ], "transformResponse": [ null ], "timeout": 0, "xsrfCookieName": "kolibri_csrftoken", "xsrfHeaderName": "X-CSRFToken", "maxContentLength": -1, "maxBodyLength": -1, "env": {}, "headers": { "Accept": "application/json, text/plain, */*", "Content-Type": "application/json", "X-Requested-With": "XMLHttpRequest", "X-CSRFToken": "9kdA89UvDBcxGefIJi2xvmBVARH85cLCbrEUDQkCRzC57kWRqqBT5qbzqhfnrpjX" }, "paramsSerializer": {}, "url": "/api/device/deviceprovision/", "method": "post", "data": "{\"facility_id\":\"13785e243c00fb2b8b0a70b0f2ce0ffe\",\"superuser\":{\"username\":\"m5\",\"password\":\"M5\"},\"settings\":{\"on_my_own_setup\":false},\"preset\":\"nonformal\",\"language_id\":\"en\",\"device_name\":\"fsdfas\",\"allow_guest_access\":false,\"is_provisioned\":true,\"is_soud\":true}" }, "request": {} }
Start over

I'll edit the description and title of the bug report as it has nothing to do with the latest work on the syncing.

@pcenov pcenov changed the title Setup Wizard - The import of a single learner is not working Setup Wizard > LOD > Import user - Missing validation for uppercase in the usernames and passwords Nov 17, 2023
@marcellamaki marcellamaki added P1 - important Priority: High impact on UX and removed P0 - critical Priority: Release blocker or regression labels Nov 20, 2023
@marcellamaki
Copy link
Member

marcellamaki commented Nov 21, 2023

We can reuse the commonCore string invalidCredentialsError with message 'Incorrect username or password' here for a short-term solution, and then revisit the UI if needed in an upcoming major release.

@hustlernik
Copy link

I guess this issue requires form validations to be stitched for username and password.
@marcellamaki Can you assign me this issue?

@thanksameeelian
Copy link
Contributor

hi there, @hustlernik! i've already been assigned this issue but please feel free to look for another issue that's featuring a "help wanted" tag! :)

@hustlernik
Copy link

@thanksameeelian no problem, sure.

@bjester
Copy link
Member

bjester commented Dec 14, 2023

@hustlernik Are you still interested?

@nick2432
Copy link
Contributor

Can I contribute to this issue? I'm new here, but I have experience contributing to many other organizations. Is there a Discord channel or any other channel where I can discuss and get assistance?

@AlexVelezLl
Copy link
Member

Hi @nick2432! I have assigned this issue to you! Please let us know if there is anything we can help with.

@nick2432
Copy link
Contributor

@AlexVelezLl Hey, thank you for assigning me this issue. I successfully ran Kolibri on my local machine, but I have a little confusion on how to reproduce the issue. If anyone can provide assistance, it would be very helpful for me.

@pcenov
Copy link
Member Author

pcenov commented Dec 21, 2023

Hi @nick2432 thank you for your interest in working on this issue. I assume you are following the steps to reproduce this issue from the description above. Which step is not clear to you and what is the nature of your confusion?

@nick2432
Copy link
Contributor

nick2432 commented Dec 21, 2023

@pcenov , The first time, I chose on my own, but now it's not showing me that option; instead, it's directly taking me to the login page.

@pcenov
Copy link
Member Author

pcenov commented Dec 21, 2023

@nick2432 First of all, to reproduce this issue you need to have 2 devices.
On device 1 which will be the server you can go through 'On my own' and then create a class with learners.
On device 2 which will be the LOD (Learn-only device) you have to go through Group learning > Learn-only device > Import one or more existing user accounts from an existing facility. There at the 'Import individual user accounts' step of the setup wizard you will be able to observe the above described issues with the user credentials.

@nick2432
Copy link
Contributor

@nick2432 First of all, to reproduce this issue you need to have 2 devices. On device 1 which will be the server you can go through 'On my own' and then create a class with learners. On device 2 which will be the LOD (Learn-only device) you have to go through Group learning > Learn-only device > Import one or more existing user accounts from an existing facility. There at the 'Import individual user accounts' step of the setup wizard you will be able to observe the above described issues with the user credentials.

ok ok i understand thank you

@nick2432
Copy link
Contributor

@pcenov I found the issue
Screenshot from 2023-12-22 11-32-32

@nick2432 nick2432 mentioned this issue Dec 25, 2023
9 tasks
@nick2432
Copy link
Contributor

nick2432 commented Dec 25, 2023

@AlexVelezLl @pcenov Please review my PR

@akolson
Copy link
Member

akolson commented Dec 29, 2023

Hi @nick2432. Thanks for the heads up.

@nick2432
Copy link
Contributor

@rtibbles @pcenov @bjester
If the letter case is incorrect, what should I do? Should it return an error stating 'username not valid'?

@rtibbles
Copy link
Member

In the case of passwords, I am not sure there is much we can except say the password is incorrect - perhaps in the future we could detect if CAPS LOCK is enabled, but that's about as much as we could do.

For usernames, it seems like we could apply the same logic that we apply to login here, where we allow for case insensitive validation of the username.

This would involve updating the public facility username search endpoint https://github.com/learningequality/kolibri/blob/release-v0.16.x/kolibri/core/public/api.py#L360 to do its searches case insensitively instead.

@nick2432
Copy link
Contributor

nick2432 commented Jan 17, 2024

In the case of passwords, I am not sure there is much we can except say the password is incorrect - perhaps in the future we could detect if CAPS LOCK is enabled, but that's about as much as we could do.

For usernames, it seems like we could apply the same logic that we apply to login here, where we allow for case insensitive validation of the username.

This would involve updating the public facility username search endpoint https://github.com/learningequality/kolibri/blob/release-v0.16.x/kolibri/core/public/api.py#L360 to do its searches case insensitively instead.

@rtibbles
So, it will search case-insensitively for the username, and if it's correct, should it proceed with the login?

@rtibbles
Copy link
Member

This endpoint only allows for username look up on a remote Kolibri for the import flow, so it is just about returning matching users from the endpoint. I think that is what is at play here after discussion with @jredrejo.

@nick2432 nick2432 mentioned this issue Jan 17, 2024
9 tasks
@nick2432
Copy link
Contributor

hey @rtibbles check my new PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 - important Priority: High impact on UX
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants