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

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

Open
Tracked by #1557 ...
vinayvenu opened this issue Apr 2, 2024 · 11 comments · Fixed by #725
Open
Tracked by #1557 ...

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

vinayvenu opened this issue Apr 2, 2024 · 11 comments · Fixed by #725

Comments

@vinayvenu
Copy link
Member

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

  • Usernames and emails must be trimmed before validation or adding to the system
  • Database changes should be reverted in case there is an error adding the user to the ID system
  • These validations should work in csv uploads as well, where we have the same problem
@Dashlet26
Copy link

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
Copy link
Member Author

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

@MiirzaBaig
Copy link

@vinayvenu Could you assign me this issue?

@vinayvenu
Copy link
Member Author

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

ombhardwajj added a commit to ombhardwajj/avni-server that referenced this issue Apr 25, 2024
@petmongrels
Copy link
Contributor

  1. Please move this method inside UserContract class. It is dealing with only the state of user contract. ombhardwajj@f5d8cb7#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
Copy link
Contributor

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

@ombhardwajj
Copy link
Contributor

ombhardwajj commented May 2, 2024

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.

master...ombhardwajj:avni-server:spacefix

@petmongrels
Copy link
Contributor

looks good. please send a pull request.

vinayvenu added a commit that referenced this issue May 6, 2024
#707 | Leading spaces when creating users are now trimmed!
@vinayvenu
Copy link
Member Author

Verification required for

  1. POST request to create/update user
  2. csv upload of UsersAndCatchments csv

@vinayvenu vinayvenu reopened this May 6, 2024
@justAbhinav
Copy link

@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
Copy link
Member Author

vinayvenu commented May 24, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: QA Ready
Development

Successfully merging a pull request may close this issue.

7 participants