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

Wide range of integration tests. #2139

Merged

Conversation

lukasz-soszynski-eliatra
Copy link
Contributor

@lukasz-soszynski-eliatra lukasz-soszynski-eliatra commented Oct 4, 2022

Signed-off-by: Lukasz Soszynski [email protected]

Description

Adds:

  1. Search requests test
  2. Basic auth tests
  3. A package containing matchers that can be used for future tests
  • Category (Enhancement)
  • Wide variety of integration tests which uses a new test framework

Issues Resolved

[List any issues this PR will resolve]

Testing

Production code can be treated as a test for tests

Check List

  • New functionality includes testing
  • New functionality has been documented
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Copy link
Member

@peternied peternied left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like the format of the Search Operation tests, but I don't think the matchers belong in this project, or at least if we merge them we will immediately want to migrate them into the high level rest client or a OpenSearch centric test framework - what do you think of this?


import static java.util.Objects.requireNonNull;

class BulkResponseContainExceptionsAtIndexMatcher extends TypeSafeDiagnosingMatcher<BulkResponse> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These matchers are really great, but I don't think they belong in the security plugin - I think they would be better suited as part of the high-level rest client, what do you think about moving them to that project?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking back at this comment, its major scope creep! Lets move forward with the great work you've got here and we can enhance the HLRC in another effort

Signed-off-by: Lukasz Soszynski <[email protected]>
peternied
peternied previously approved these changes Oct 6, 2022

class ClusterContainsDocumentWithFieldValueMatcher extends TypeSafeDiagnosingMatcher<Client> {

private static final Logger log = LogManager.getLogger(LocalCluster.class);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Can we change the logger to be a different class?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done :)

@codecov-commenter
Copy link

codecov-commenter commented Oct 7, 2022

Codecov Report

Merging #2139 (eb3180b) into main (ca4917c) will increase coverage by 0.01%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##               main    #2139      +/-   ##
============================================
+ Coverage     61.01%   61.02%   +0.01%     
- Complexity     3229     3232       +3     
============================================
  Files           256      257       +1     
  Lines         18101    18110       +9     
  Branches       3229     3229              
============================================
+ Hits          11044    11052       +8     
- Misses         5475     5476       +1     
  Partials       1582     1582              
Impacted Files Coverage Δ
...pensearch/security/setting/DeprecatedSettings.java 80.00% <0.00%> (ø)
...c/auth/ldap/backend/LDAPAuthenticationBackend.java 81.57% <0.00%> (+0.32%) ⬆️
.../dlic/auth/ldap2/LDAPConnectionFactoryFactory.java 57.46% <0.00%> (+0.64%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@DarshitChanpura DarshitChanpura left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ty @lukasz-soszynski-eliatra for this PR. SearchOperationTest looks really good and ty for adding the Matchers. I have one change to request to make SearchRequestFactory class final.


SearchScrollRequest scrollRequest = getSearchScrollRequest(searchResponse);

assertThatThrownBy(() -> restHighLevelClient.scroll(scrollRequest, DEFAULT), statusException(FORBIDDEN));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For my understanding, why does this request fail even though DOUBLE_READER_USER has slightly broader set of permissions than LIMITED_READ_USER.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The result of request restHighLevelClient.scroll(scrollRequest, DEFAULT) is HTTP response with 403 status code because the DOUBLE_READER_USER is lacking cluster permission indices:data/read/scroll.

@@ -380,7 +380,7 @@ public AuthcDomain jwtHttpAuthenticator(String headerName, String signingKey) {
return this;
}

public AuthcDomain challengingAuthenticator(String type) {
public AuthcDomain httpAuthenticatorWithChallenge(String type) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ty 👍


import static java.util.concurrent.TimeUnit.MINUTES;

public class SearchRequestFactory {
Copy link
Member

@DarshitChanpura DarshitChanpura Oct 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should make this class final since it contains static methods only

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

} catch (URISyntaxException ex) {
throw new RuntimeException("Incorrect URI syntax", ex);
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice

@DarshitChanpura
Copy link
Member

DarshitChanpura commented Oct 7, 2022

@lukasz-soszynski-eliatra I'm ready to approve it. Can you please sign-off your commits, so we can get DCO check to be successful to have passing CI.

@lukasz-soszynski-eliatra
Copy link
Contributor Author

Thanks, commits corrected.

@DarshitChanpura DarshitChanpura merged commit f09af90 into opensearch-project:main Oct 10, 2022
stephen-crawford pushed a commit to stephen-crawford/security that referenced this pull request Nov 10, 2022
Adds:
1. Search requests test
2. Basic auth tests
3. A package containing matchers that can be used for future tests

Signed-off-by: Lukasz Soszynski <[email protected]>

Signed-off-by: Lukasz Soszynski <[email protected]>
Signed-off-by: Stephen Crawford <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants