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.

Sign up for GitHub

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

Jump to bottom

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

Open
jayasting98 opened this issue Mar 15, 2024 · 16 comments · May be fixed by #13101
Open

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

jayasting98 opened this issue Mar 15, 2024 · 16 comments · May be 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.

teammates/src/it/java/teammates/it/storage/sqlapi/AccountRequestsDbIT.java

Lines 196 to 209 in 2342189

@Test
public void testSqlInjectionSearchAccountRequestsInWholeSystem() throws Exception {
______TS("SQL Injection test in searchAccountRequestsInWholeSystem");
AccountRequest accountRequest = new AccountRequest("test@gmail.com", "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("test@gmail.com", "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.

teammates/src/it/java/teammates/it/storage/sqlsearch/AccountRequestSearchIT.java

Lines 117 to 123 in 2342189

@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
@github-actions GitHub Actions
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.

traitsisgiorgos added a commit to traitsisgiorgos/teammates that referenced this issue Apr 12, 2024
@traitsisgiorgos
This is the fix for TEAMMATES#12901. I added the check to enusre the …
12d1d20 
…search service is on and i moved the method to the AccountRequestSearchIT.java
@traitsisgiorgos traitsisgiorgos mentioned this issue Apr 14, 2024
Open
@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!

@cliu585 cliu585 mentioned this issue Apr 28, 2024
Merged
cliu585 added a commit to cliu585/teammates that referenced this issue Apr 28, 2024
@cliu585
Merge pull request #1 from cliu585/test-search-service-active
e33e88b 
[TEAMMATES#12901] Check if the search service is active in account request search test
@cliu585 cliu585 linked a pull request Apr 29, 2024 that will close this issue
Open
@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
[fix TEAMMATES#12901] moved testSqlInjectionSearchAccountRequestsInWh…
2367769 
…oleSystem to AccountRequestSearchIT class
Ashwinn11 added a commit to Ashwinn11/teammates that referenced this issue May 13, 2024
@Ashwinn11
[TEAMMATES#12901] added a check for searchService
68d5734
@Ashwinn11 Ashwinn11 mentioned this issue May 13, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
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
Milestone
No milestone
7 participants
@EmilyMSong @domlimm @jayasting98 @techMedMau @simranfarrukh @AggarwalNeelesh @traitsisgiorgos