Tests related basic authentication. by lukasz-soszynski-eliatra · Pull Request #2138 · opensearch-project/security

lukasz-soszynski-eliatra · 2022-10-04T11:23:22Z

Signed-off-by: Lukasz Soszynski lukasz.soszynski@eliatra.com

Description

[Describe what this change achieves]

Category (New feature [Tests])
The feature introduces integration tests related to basic authentication. The tests are implemented with the usage of a newly developed test framework.

Issues Resolved

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.

Signed-off-by: Lukasz Soszynski <lukasz.soszynski@eliatra.com>

peternied

Minor comments, thanks for adding these tests

peternied · 2022-10-04T17:52:51Z

src/integrationTest/java/org/opensearch/test/framework/cluster/TestRestClient.java

+			return Arrays.stream(header)
+				.filter(header -> requireNonNull(name, "Header name is mandatory.").equalsIgnoreCase(header.getName()))
+				.findFirst()
+				.orElse(null);


Seems like it should throw if no header is found

Shifting to throwing an exception would prevent tests cases from having to cover this scenario

Code doesn't seem updated

I think the code was updated: https://github.com/opensearch-project/security/pull/2138/files#diff-091a2aa7163b07f68fa3a40034816a92ba8cf178fd765245cbc802eb79216be9R284

peternied · 2022-10-04T17:55:10Z

src/integrationTest/java/org/opensearch/test/framework/cluster/TestRestClient.java

+		public void assertThatBrowserDoesNotAskUserForCredentials() {
+			Header header = getHeader(HttpHeaders.WWW_AUTHENTICATE);
+			assertThat(header, nullValue());
+		}
+
+		public void assertThatBrowserAskUserForCredentials() {
+			Header header = getHeader(HttpHeaders.WWW_AUTHENTICATE);
+			assertThat(header, notNullValue());
+			assertThat(header.getValue(), containsStringIgnoringCase("basic"));
+		}


These feel more special purpose to authentication test cases rather than methods that would be used in all test cases, what do you think about moving them?

Done: I moved these methods to test classes.

Signed-off-by: Lukasz Soszynski <lukasz.soszynski@eliatra.com>

cwperks

This is great @lukasz-soszynski-eliatra! These tests are nice and readable using the new framework. I have a couple of questions, but the changes look good to me.

cwperks · 2022-10-05T13:53:48Z

src/integrationTest/java/org/opensearch/security/http/BasicAuthTests.java

+			HttpResponse response = client.getAuthInfo();
+
+			assertThat(response, is(notNullValue()));
+			response.assertStatusCode(SC_OK);


Would it make sense to add an additional assertion to ensure we can deserialize the response like what is done in testUserShouldNotHaveAssignedCustomAttributes?

Suggested change

response.assertStatusCode(SC_OK);

response.assertStatusCode(SC_OK);

AuthInfo authInfo = response.getBodyAs(AuthInfo.class);

assertThat(authInfo, is(notNullValue()));

cwperks · 2022-10-05T14:00:07Z

src/integrationTest/java/org/opensearch/security/http/BasicAuthTests.java

+
+	@Test
+	public void shouldRespondWith401WhenUserDoesNotExist() {
+		try (TestRestClient client = cluster.getRestClient(NOT_EXISTING_USER, INVALID_PASSWORD)) {


Would all of these tests pass if an exception is thrown in getRestClient? I see this pattern used in a number of places in the codebase and I also see one instance where we add:

} catch (IOException ioe) { fail("unexpected exception") }

See:

security/src/test/java/org/opensearch/security/auditlog/compliance/ComplianceAuditlogTest.java

Lines 234 to 242 in 1904db5

try (RestHighLevelClient restHighLevelClient = getRestClient(clusterInfo, "kirk-keystore.jks", "truststore.jks")) {

for (IndexRequest ir: new DynamicSecurityConfig().setSecurityRoles("roles_2.yml").getDynamicConfig(getResourceFolder())) {

restHighLevelClient.index(ir, RequestOptions.DEFAULT);

GetResponse getDocumentResponse = restHighLevelClient.get(new GetRequest(ir.index(), ir.id()), RequestOptions.DEFAULT);

assertThat(getDocumentResponse.isExists(), equalTo(true));

}

} catch (IOException ioe) {

throw new RuntimeException("Unexpected exception", ioe);

}

+1. In case an exception is thrown, how is it expected to be handled?

Read up more here https://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html

If there is an exception it is bubbled up as if the getRestClient was executed inside the try block, since there is no catch(Exception e) {} block in this test case there is nothing to capture.

TLDR: Exceptions work as expected, the test would fail if there was a problem 🥳

cwperks · 2022-10-05T14:12:23Z

src/integrationTest/java/org/opensearch/test/framework/TestSecurityConfig.java


 	public static class AuthcDomain implements ToXContentObject {

+		private static String PUBLIC_KEY = "MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAoqZbjLUAWc+DZTkinQAdvy1GFjPHPnxheU89hSiWoDD3NOW76H3u3T7cCDdOah2msdxSlBmCBH6wik8qLYkcV8owWukQg3PQmbEhrdPaKo0QCgomWs4nLgtmEYqcZ+QQldd82MdTlQ1QmoQmI9Uxqs1SuaKZASp3Gy19y8su5CV+FZ6BruUw9HELK055sAwl3X7j5ouabXGbcib2goBF3P52LkvbJLuWr5HDZEOeSkwIeqSeMojASM96K5SdotD+HwEyjaTjzRPL2Aa1BEQFWOQ6CFJLyLH7ZStDuPM1mJU1VxIVfMbZrhsUBjAnIhRynmWxML7YlNqkP9j6jyOIYQIDAQAB";


Should we be using the new automatic test certificate generation for this?

Actually, this is not a certificate but just a public key. The public key does not contain any metadata so it never expires.

DarshitChanpura

Ty for this PR @lukasz-soszynski-eliatra !

DarshitChanpura · 2022-10-05T16:47:51Z

src/integrationTest/java/org/opensearch/security/http/BasicAuthTests.java

+
+	@Test
+	public void shouldRespondWith401WhenUserDoesNotExist() {
+		try (TestRestClient client = cluster.getRestClient(NOT_EXISTING_USER, INVALID_PASSWORD)) {


+1. In case an exception is thrown, how is it expected to be handled?

DarshitChanpura · 2022-10-05T16:48:43Z

src/integrationTest/java/org/opensearch/security/http/BasicAuthWithoutChallengeTests.java

+	}
+
+	private void assertThatBrowserDoesNotAskUserForCredentials(HttpResponse response) {
+		String reason = "Browser ask user for credentials what is not expected";


nit: Browser asked user for credentials which is not expected

DarshitChanpura · 2022-10-05T16:49:45Z

src/integrationTest/java/org/opensearch/security/http/BasicAuthTests.java

+	}
+
+	@Test
+	public void browserShouldRequestUserForCredentials() {


nit: testBrowserShouldRequestForCredentials

DarshitChanpura · 2022-10-05T16:50:07Z

src/integrationTest/java/org/opensearch/security/http/BasicAuthTests.java

+	}
+
+	@Test
+	public void userShouldHaveAssignedCustomAttributes() {


nit: testUserShouldHaveAssignedCustomAttributes

DarshitChanpura · 2022-10-05T16:56:11Z

src/integrationTest/java/org/opensearch/test/framework/TestSecurityConfig.java

+			return this;
+		}
+
 		public AuthcDomain challengingAuthenticator(String type) {


nit: this should probly be rename to httpAuthenticatorWithChallenge.

* A few first tests for basic authentication. Signed-off-by: Lukasz Soszynski <lukasz.soszynski@eliatra.com> Co-authored-by: Lukasz Soszynski <lsoszyn@gmail.com> Signed-off-by: Stephen Crawford <steecraw@amazon.com>

A few first tests for basic authentication.

3679392

Signed-off-by: Lukasz Soszynski <lukasz.soszynski@eliatra.com>

lukasz-soszynski-eliatra requested a review from a team October 4, 2022 11:23

peternied previously approved these changes Oct 4, 2022

View reviewed changes

Correction after code review applied to basic authentication tests.

6e37176

Signed-off-by: Lukasz Soszynski <lukasz.soszynski@eliatra.com>

lukasz-soszynski-eliatra dismissed peternied’s stale review via 6e37176 October 5, 2022 09:35

cwperks approved these changes Oct 5, 2022

View reviewed changes

DarshitChanpura reviewed Oct 5, 2022

View reviewed changes

peternied approved these changes Oct 5, 2022

View reviewed changes

peternied merged commit ca4917c into opensearch-project:main Oct 5, 2022

DarshitChanpura mentioned this pull request Aug 22, 2023

Backport the test framework to 2.x branch #3230

Closed

6 tasks

	try (RestHighLevelClient restHighLevelClient = getRestClient(clusterInfo, "kirk-keystore.jks", "truststore.jks")) {
	for (IndexRequest ir: new DynamicSecurityConfig().setSecurityRoles("roles_2.yml").getDynamicConfig(getResourceFolder())) {
	restHighLevelClient.index(ir, RequestOptions.DEFAULT);
	GetResponse getDocumentResponse = restHighLevelClient.get(new GetRequest(ir.index(), ir.id()), RequestOptions.DEFAULT);
	assertThat(getDocumentResponse.isExists(), equalTo(true));
	}
	} catch (IOException ioe) {
	throw new RuntimeException("Unexpected exception", ioe);
	}


		public static class AuthcDomain implements ToXContentObject {

		private static String PUBLIC_KEY = "MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAoqZbjLUAWc+DZTkinQAdvy1GFjPHPnxheU89hSiWoDD3NOW76H3u3T7cCDdOah2msdxSlBmCBH6wik8qLYkcV8owWukQg3PQmbEhrdPaKo0QCgomWs4nLgtmEYqcZ+QQldd82MdTlQ1QmoQmI9Uxqs1SuaKZASp3Gy19y8su5CV+FZ6BruUw9HELK055sAwl3X7j5ouabXGbcib2goBF3P52LkvbJLuWr5HDZEOeSkwIeqSeMojASM96K5SdotD+HwEyjaTjzRPL2Aa1BEQFWOQ6CFJLyLH7ZStDuPM1mJU1VxIVfMbZrhsUBjAnIhRynmWxML7YlNqkP9j6jyOIYQIDAQAB";

Conversation

lukasz-soszynski-eliatra commented Oct 4, 2022

Description

Issues Resolved

Testing

Check List

Uh oh!

peternied left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cwperks left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DarshitChanpura left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants