heyzec / pe

0 stars 0 forks source link

Possible to bypass conflict checks after some editing #3

Open heyzec opened 1 year ago

heyzec commented 1 year ago

In the user guide, it is stated that "You cannot add duplicate buyers that have the same phone number or email.". The spirit of this conflict checks can be bypassed, as shown:

  1. Start with default data
  2. editbuyer 3 -n John Doe -e alexyeoh@example.com -r 40000-50000 -pr HIGH
  3. Now, both index 1 and 3 have the same email address.

I noticed that app checks to prevent names, numbers and emails duplicates, although this example is only for emails. I believe it also extendable to names and numbers.

nus-se-script commented 1 year ago

Team's Response

No details provided by team.

The 'Original' Bug

[The team marked this bug as a duplicate of the following bug]

Editting a buyer detail does not check for duplicate phone number

A buyer can be editted to have the same phone number as another buyer.

image.png

image.png

If user will to exit the app, subsequent opening of the app will not be able to read the data due to duplicate values, causing all buyer data to be lost.


[original: nus-cs2103-AY2223S1/pe-interim#4848] [original labels: type.FunctionalityBug severity.High]

Their Response to the 'Original' Bug

[This is the team's response to the above 'original' bug]

Unfortunately, the team did not manage to catch this bug when we were testing.

Original Implementation

The problem arises in this statement.

Screenshot 2022-11-14 at 7.21.20 PM.png

We throw an exception if:

  1. The edited buyer is not the same as the buyer to be edited in the list, AND
  2. We can still find the edited buyer in the buyer list.

This is only valid based on the assumption that:

  1. If the edited buyer is the same as the buyer to be edited,
  2. then the edited buyer will not be the same as other buyers, and thus no duplicates are formed.

Problems due to changes made to isSameBuyer

However, we changed the implementation of isSamebuyer. Now, isSameBuyer returns true if either phone number OR email is the same.

Screenshot 2022-11-14 at 7.23.43 PM.png

Hence, the previous assumption made is false. It is now possible for an edited buyer to be considered the same as more than 1 buyer (consider a buyer with the same phone number as 1 buyer and the same email as another buyer).

Thus, there could be a case where the edited buyer IS the same as the buyer to be edited in the list (hence failing condition 1 and not throwing the exception), but is also the same as another buyer in the list, thereby introducing a duplicate buyer.

  • Edit either a buyer's email or phone number only such that they are the same as a different buyer in the buyer list
    • !buyerToEdit.isSameBuyer(editedBuyer): returns false, as there is still an unedited field (either email or phone number) that is is the same as the original buyer

Solution

The main idea of duplicate checking is that we should only be checking whether our newly edited buyer is the same as all the other buyers in the list. It does not matter if it is the same as the buyer to be edited.

What we could have done is modify the hasBuyer method to include an extra argument that takes in an index to be excluded from consideration. In this way, we can exclude the buyer to be edited from being checked if it is the same as the edited buyer.

Screenshot 2022-11-14 at 8.03.48 PM.png

Hence, this change would basically solve all problems with buyer duplicates (both phone numbers and emails).

Items for the Tester to Verify

:question: Issue duplicate status

Team chose to mark this issue as a duplicate of another issue (as explained in the Team's response above)

Reason for disagreement: [replace this with your explanation]


:question: Issue severity

Team chose [severity.High] Originally [severity.Low]

Reason for disagreement: [replace this with your explanation]