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

Check if the search service is active in account request search test #12901

Closed
jayasting98 opened this issue Mar 15, 2024 · 17 comments · Fixed by #13101
Closed

Check if the search service is active in account request search test #12901

jayasting98 opened this issue Mar 15, 2024 · 17 comments · Fixed by #13101
Labels
a-Testing Testing-related traits such as efficiency, robustness, coverage c.Task Other non-user-facing works, e.g. refactoring, adding tests good first issue Easy; restricted for first-time contributors

Comments

@jayasting98
Copy link
Contributor

Currently, AccountRequestsDbIT::testSqlInjectionSearchAccountRequestsInWholeSystem does not check if the search service is active.

@Test
public void testSqlInjectionSearchAccountRequestsInWholeSystem() throws Exception {
______TS("SQL Injection test in searchAccountRequestsInWholeSystem");
AccountRequest accountRequest = new AccountRequest("[email protected]", "name", "institute");
accountRequestDb.createAccountRequest(accountRequest);
String searchInjection = "institute'; DROP TABLE account_requests; --";
List<AccountRequest> actualInjection = accountRequestDb.searchAccountRequestsInWholeSystem(searchInjection);
assertEquals(0, actualInjection.size());
AccountRequest actual = accountRequestDb.getAccountRequest("[email protected]", "institute");
assertEquals(accountRequest, actual);
}

This leads to the following failing test if you try to test locally without the search service.

image

To fix this, we can check if the search service is active, before running the rest of the test, like in the following.

@Test
public void testSearchAccountRequest_deleteAfterSearch_shouldNotBeSearchable() throws Exception {
if (!TestProperties.isSearchServiceActive()) {
return;
}
AccountRequest ins1InCourse2 = typicalBundle.accountRequests.get("instructor1OfCourse2");

@jayasting98 jayasting98 added good first issue Easy; restricted for first-time contributors a-Testing Testing-related traits such as efficiency, robustness, coverage c.Task Other non-user-facing works, e.g. refactoring, adding tests labels Mar 15, 2024
Copy link

Good First Issue - Notes for Contributors
This issue is for first-time contributors only. If you are new to TEAMMATES, feel free to submit a PR for this issue.

Please note that we allow only one good first issue per contributor. If you have already made a prior contribution to TEAMMATES, you may wish to take a look at issues with the help wanted tag instead.

We do not assign issues to contributors. If you would like to pick up this issue, do post a comment below to express your interest and check if there is anyone else who is already working on the issue. We will do our best to reply and give you the go-ahead, but if we don't, feel free to submit a PR as long as there is no one else working on it.

To get started, do read through our contributing guidelines carefully, and set up a development environment on your local machine before making a PR.

If you need any clarifications on our developer guide, or are facing issues that are not found in our troubleshooting guide, please post a message in our discussion forum.

@jayasting98
Copy link
Contributor Author

We may also want to move the test method into AccountRequestSearchIT.

@techMedMau
Copy link
Contributor

Hi, may I work on this issue?
Besides, it seems like this issue only needs to add a check condition to ensure the search service is on and move it to the right file.
Am I understanding it correctly?

Thank you

@traitsisgiorgos
Copy link

can I take the issue?

@domlimm
Copy link
Contributor

domlimm commented Mar 17, 2024

@techMedMau Hey Maureen, as you've previously contributed before. Let's leave this for a new contributor.

@traitsisgiorgos Feel free to submit a PR for this.

@traitsisgiorgos
Copy link

thanks!!

@AggarwalNeelesh
Copy link

Hey , I am a beginner in open source having experience in java.
I want to solve this issue

@jayasting98
Copy link
Contributor Author

@AggarwalNeelesh Hi, thank you for your interest. Let's give @traitsisgiorgos some time to try to do this. If they decide not to continue with this, you may try to tackle this issue.

@traitsisgiorgos
Copy link

Can I ask you a question ?

@traitsisgiorgos
Copy link

@jayasting98

@traitsisgiorgos
Copy link

We also want to move the method to the correct file?

@simranfarrukh
Copy link

Hello, may I work on this issue?

@jayasting98
Copy link
Contributor Author

We also want to move the method to the correct file?

Hi @traitsisgiorgos, yes, we may want to move it over to AccountRequestSearchIT.

@jayasting98
Copy link
Contributor Author

Hey, @simranfarrukh, thank you for your interest. @traitsisgiorgos is working on this right now. If you want, you can consider handling a different issue.

@EmilyMSong
Copy link

Is this issue still available to be worked on? I would like to work on it with my team for a class project!

@jayasting98
Copy link
Contributor Author

Hello, @EmilyMSong. @traitsisgiorgos is currently working on this right now. Nevertheless, if you would still like to contribute, there are other issues that you can consider working on.

Ashwinn11 added a commit to Ashwinn11/teammates that referenced this issue May 13, 2024
Ashwinn11 added a commit to Ashwinn11/teammates that referenced this issue May 13, 2024
@itstrueitstrueitsrealitsreal
Copy link
Contributor

Hi, @jayasting98. I am looking to work on this project and I am trying to get AccountRequestsDbIT::testSqlInjectionSearchAccountsRequestsInWholeSystem to pass.

I have made the changes locally, and it seems that #13101 also remedies this issue, but has not been merged in.

Would it be possible to work on this issue? Thanks!

weiquu added a commit that referenced this issue Jun 28, 2024
…rch test (#13101)

* added check for active search service in account request search test

* moved account request search test to AccountRequestSearchIT and updated variables and asserts accordingly

* Add caution note to instructor email copies and remove hyperlinks

* updated account request test constructor to be consistent with latest commits

* fixed style errors

* fixed compiler errors

* fixed assertion error and style

* fixed linter errors

* Revert "Add caution note to instructor email copies and remove hyperlinks"

This reverts commit 232c757.

---------

Co-authored-by: Carolyn Liu <[email protected]>
Co-authored-by: Anna Zhang <[email protected]>
Co-authored-by: DS <[email protected]>
Co-authored-by: Wei Qing <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-Testing Testing-related traits such as efficiency, robustness, coverage c.Task Other non-user-facing works, e.g. refactoring, adding tests good first issue Easy; restricted for first-time contributors
Projects
None yet
8 participants