avniproject / avni-server

Backend APIs for Avni
https://avniproject.org
GNU Affero General Public License v3.0
5 stars 25 forks source link

Leading spaces when creating users lead to bad data in system #707

Open vinayvenu opened 3 months ago

vinayvenu commented 3 months ago

Users with usernames starting with spaces are created in

POST /user
{
  "operatingIndividualScope": "ByCatchment",
  "username": " testvinay1@gdgsgom",
  "ignored": " testvinay1",
  "name": "test",
  "email": "tas@asd.com",
  "phoneNumber": "+919693939393",
  "catchmentId": 21002
}

Response

{
  "message": "2 validation errors detected: Value at 'temporaryPassword' failed to satisfy constraint: Member must satisfy regular expression pattern: ^[\\S]+.*[\\S]+$; Value at 'username' failed to satisfy constraint: Member must satisfy regular expression pattern: [\\p{L}\\p{M}\\p{S}\\p{N}\\p{P}]+ (Service: AWSCognitoIdentityProvider; Status Code: 400; Error Code: InvalidParameterException; Request ID: 7addf14f-e99c-4d77-8c2c-2278e1589436)"
}

Problem The user is not created in Cognito, but is created on the database. The next time this user is created, the app mentions that the user already exists

Acceptance criteria

Dashlet26 commented 3 months ago

Here is my solution regarding this problem , If we have to resolve the issue then

  1. we must ensure that username value does not start with space meets regular expression pattern Cognito
  2. Modify request to provide a valid username that meets criteria by Cognito .
vinayvenu commented 3 months ago

@Dashlet26 the solution is outlined in the Acceptance criteria section.

MiirzaBaig commented 2 months ago

@vinayvenu Could you assign me this issue?

vinayvenu commented 2 months ago

@MiirzaBaig there is no assignment. You can work on it, raise a PR when you are ready.

petmongrels commented 2 months ago
  1. Please move this method inside UserContract class. It is dealing with only the state of user contract. https://github.com/ombhardwajj/avni-server/commit/f5d8cb7c74f60ec2f643ce44a6e627a10f20d341#diff-4c6e9031b146d5df5d1f3276fa07838ea5e1d0158cb3aee6348f91b656b888baR77
  2. The file uploaded by the user should not be modified and stored in S3. Since it is the source file, it should be uploaded as it is. The removal of spaces should be handled in the code when reading the file.
mahalakshme commented 2 months ago

@AchalaBelokar why have you moved the card to QA Ready? moving back for now since looks like the comments are not fixed.

ombhardwajj commented 2 months ago

Hey @petmongrels and @mahalakshme,

Apologies for the delay in addressing your comment; I was caught up with exams this week. I've made the necessary adjustments based on your feedback. Specifically, I've reverted the changes made to the S3Service, IdpService, and Factory files back to their original state. Additionally, I've modified the UserContract.java files to handle trimming.

Could you please review the changes and let me know if they meet your expectations? If any further adjustments are needed, please point them out.

https://github.com/avniproject/avni-server/compare/master...ombhardwajj:avni-server:spacefix

petmongrels commented 2 months ago

looks good. please send a pull request.

vinayvenu commented 2 months ago

Verification required for

  1. POST request to create/update user
  2. csv upload of UsersAndCatchments csv
justAbhinav commented 1 month ago

@vinayvenu reading from this issue timeline i have understood that the issue has been solved, so why is this re opened, and in that case what issues remain?

vinayvenu commented 1 month ago

@justAbhinav it was reopened because sufficient QA was not done on it yet. Will be closed once the QA says ok