Closed metodimilevqa closed 3 years ago
@metodimilevqa , is the juan
username the same one you made during the Setup Wizard? It's a known issue that if you try to create a new user with the superuser's name, you will not see an error message inside the form.
However, Kolibri should show some kind of full-page error message, instead of nothing happening after you click Continue.
@metodimilevqa , is the
juan
username the same one you made during the Setup Wizard? It's a known issue that if you try to create a new user with the superuser's name, you will not see an error message inside the form.However, Kolibri should show some kind of full-page error message, instead of nothing happening after you click Continue.
No, "juan" isn't the username I used during setup. For the setup I used metodimilev :)
Yeah, I just reproduced this. What should be happening is that after creating the account fails, the user gets taken back to the "Step 1" form so they can see this error under the username field:
@jonboiser There is already a check for a duplicate username in the front end:
But the back end filters are very strict and they do not return anything.
Is this because you do not want the user details to be exposed and the queryset isn't always FacilityUser.objects.all()
? In that case, the viewset could return a status code of 200 in case the user name exists or 204 in case it wasn't found and can be used. And in the special case for app context, it would return the list as it does now.
Ah, maybe it's the dataset__learner_can_login_with_no_password=True
filtering that is causing so many false negatives.
It's possible this form wasn't tested with a facility where that setting was turned on and we had just assumed FacilityUsernameViewset
did return a query akin to FacilityUser.objects.all()
(although I did note in a comment above checkForDuplicateUsername
that the superuser was not in there, which makes sense with that second sets of filters).
I think to implement a fix to this issue, we would need to make a separate endpoint with your suggested design just to test for duplicate usernames and not expose too much info. So something like this
GET /api/usernameexists/jonboiser
-> returns 200 or 204 with no payload
I think we should implement the separate endpoint that @jonboiser suggests or we should just abandon the pre-emptive duplicate username flagging, and just set this error when we attempt to create the user, and the backend returns with the error.
and just set this error when we attempt to create the user, and the backend returns with the error.
This is what I had in mind as a short-term fix to this issue, where we handle the backend error by going to the Step 1 form. I think the UX is not great because you will go
But this might be enough of a rare case that this is okay for now.
Another possibility is that we could create the user, and then do a PATCH with the demographic data afterwards.
Another possibility is that we could create the user, and then do a PATCH with the demographic data afterwards.
I don't think this is an option because you are allowed to go back and forth from the username/password section and the demographic section. Actually creating the user after Step 1 would change this to a one-way process.
I feel that the adding a new "check if username exists" endpoint might be the best solution and would be useful in the Facility > Users, and Edit Profile page as well.
I think the main issue is probably more the expectation of being able to cancel at any stage in the process without having created the account, so yes, having the endpoint for checking does seem simplest, and less of a security risk than returning all user names from the user name endpoint.
So I think this is the approach we should take to update this form:
checkForDuplicateUsername
method to use this endpoint. If it returns an OK, continue to the next page, otherwise stay put and show the duplicate-username error.We also need to handle the edge case where a user might be added after the the initial username check, and before the user clicks "Finish" on the demographic info form. In this case, we would need to take the user back to Step 1 and show the duplicate-username error
Currently the issue is also experienced with Kolibri 0.14.6rc1, Windows 7 vm, IE 11.0.9600.
I would like to contribute pls review pr and suggest changes or improvements.
@jonboiser If this issue is still open, I would like to work on it. Having a separate endpoint to check for duplicate username can solve #7602 too.
Hi @kmrinal19 , yes this issue is still open. Before you submit a PR, could you check in and share the API design you have in mind? I think the basic strategy if have here
- Add a very simple endpoint that only accepts GET requests and returns a 200/204 (or 404 to take advantage of it being interpreted as an error by convention in axios
- Update the
checkForDuplicateUsername
method to use this endpoint. If it returns an OK, continue to the next page, otherwise stay put and show the duplicate-username error.
Should work, but we haven't worked out the details yet (e.g. the request/response payload, error handling, etc.). So some details on how the new "duplicate username" endpoint works would be helpful.
Hi @kmrinal19 , yes this issue is still open. Before you submit a PR, could you check in and share the API design you have in mind? I think the basic strategy if have here
- Add a very simple endpoint that only accepts GET requests and returns a 200/204 (or 404 to take advantage of it being interpreted as an error by convention in axios
- Update the
checkForDuplicateUsername
method to use this endpoint. If it returns an OK, continue to the next page, otherwise stay put and show the duplicate-username error.Should work, but we haven't worked out the details yet (e.g. the request/response payload, error handling, etc.). So some details on how the new "duplicate username" endpoint works would be helpful.
@jonboiser As you suggested earlier, we can have an endpoint like GET /api/usernameexists/{username}
which returns a 200
response with a payload like { username_exists : false}
. If username_exists
is false, we can continue to the next page, or if username_exists
is false, we can show the duplicate-username error. Having an endpoint like this will also help solve #7602, where we have to ensure that the username exists in the sign-in flow.
Sounds like a good plan! I think this new endpoint could be placed in the the core
app in this module:
When doing this username_exists
check, make sure to keep the facility
in mind. A Kolibri device can have multiple facilities in it, and it is okay for different facilities to have usernames
in common.
So if I try to create an account in Facility A with username user_1
, I should not get blocked if Facility B already has that username reserved.
@jonboiser I was wondering if we can have an endpoint like GET api/auth/usernameexists
with username
and facility
as GET parameters in place of GET /api/usernameexists/{username}
.
Something like this?
GET /api/usernameexists?username=...&facility=...
I think that would be okay. I think if you implement this endpoint, I would want to query it from the client using a HEAD
request instead of GET
, so make sure that works too.
Something like this?
GET /api/usernameexists?username=...&facility=...
I think that would be okay. I think if you implement this endpoint, I would want to query it from the client using a
HEAD
request instead ofGET
, so make sure that works too.
@jonboiser The endpoint mentioned above will return a response with payload like { username_exists : false}
, so why do we need to query the endpoint using a HEAD
request instead of GET
, as HEAD
request does not allow payload response?
Ah okay, that makes sense since the "username doesn't exist" case is actually the "good"/non-error case.
Observed behavior
Scenario: Username is already taken - I can fill the same name of already existing username (juan) (Step 1) and I don't see error message, and I can continue to the next screen with Gender and Birth Year (Step 2) and from there if I click Continue, nothing happens and if I click the browser Back button, just then the error message for already existing username is seen.
Expected behavior
The error message for already existing username is seen on Step 1, after the username field has the same input as an already existing user.
User-facing consequences
On Step 2 screen it is not clear for the user what exactly is happening.
Errors and logs
…
Steps to reproduce
Preconditions: There must be an already existing account.
Context
Kolibri 0.14.3 OS: Raspbian GNU, Linux 10 Raspberry Pi 4 Chrome (Last version) + FF (Last version)