Skip to content

Enhance RestLayerPrivilegesEvaluator Code Coverage #3218

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

Merged
peternied merged 13 commits into from
Aug 23, 2023
Merged

Enhance RestLayerPrivilegesEvaluator Code Coverage #3218

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
64bdf87
PathMatches
stephen-crawford Aug 21, 2023
8efb311
coverage
stephen-crawford Aug 21, 2023
d4d15ec
Spotless
stephen-crawford Aug 21, 2023
e1b6c7b
Merge branch 'opensearch-project:main' into boostedCoverage
stephen-crawford Aug 21, 2023
b46f25c
Merge branch 'opensearch-project:main' into boostedCoverage
stephen-crawford Aug 21, 2023
f9756f3
Merge branch 'opensearch-project:main' into boostedCoverage
stephen-crawford Aug 22, 2023
acb23d2
Fix naming
stephen-crawford Aug 22, 2023
8bb213c
Increase coverage target
peternied Aug 22, 2023
b1ac98b
Merge remote-tracking branch 'steven/boostedCoverage' into boostedCov…
peternied Aug 22, 2023
3fb77a7
Merge pull request #4 from peternied/boostedCoverage
stephen-crawford Aug 22, 2023
e408e40
Fix mock
stephen-crawford Aug 22, 2023
c762490
Cleans up RestLayerPrivilegesEvaluatorTest
DarshitChanpura Aug 23, 2023
b58e3bc
Merge pull request #5 from DarshitChanpura/cleanup-restevaluatortest
stephen-crawford Aug 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions src/test/java/org/opensearch/security/filter/RestPathMatchesTest.java
View file
Open in desktop

@peternied peternied Aug 21, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If code cov says that there is a no change, are you sure these tests are executed and their coverage data submitted? Do you see these tests executed from the artifacts of the check runs?

image

@stephen-crawford stephen-crawford Aug 21, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It does not look like it ran. I am not sure how to fix that though. I don't think it is something related to this change--I triggered a re-run.

@willyborankin willyborankin Aug 21, 2023 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@scrawfor99 I think you need to call it RestPathMatchesTests.java not RestPathMatchesTest.java.

@willyborankin willyborankin Aug 21, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Maybe it is not only your case but others as well.

@peternied peternied Aug 21, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice catch @willyborankin . I've got a change that will expect 100% code coverage in the tests (e.g. all test code should be executed) that should help us catch this kind of issue

@willyborankin willyborankin Aug 21, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yes I did the same mistake :-D. Here is PR: #3224. Lets see.

@stephen-crawford stephen-crawford Aug 22, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Huh, who knew. I will swap over the name.

Original file line number Diff line number Diff line change
Expand Up @@ -63,4 +63,25 @@ public void testDifferentPathSegments() throws InvocationTargetException, Illega
String handlerPath = "_plugins/security/api/x/y";
assertFalse((Boolean) restPathMatches.invoke(securityRestFilter, requestPath, handlerPath));
}

@Test
public void testRequestPathWithNamedParam() throws InvocationTargetException, IllegalAccessException {
String requestPath = "_plugins/security/api/123/y";
String handlerPath = "_plugins/security/api/{id}/z";
assertFalse((Boolean) restPathMatches.invoke(securityRestFilter, requestPath, handlerPath));
}

@Test
public void testRequestPathMismatch() throws InvocationTargetException, IllegalAccessException {
String requestPath = "_plugins/security/api/x/y";
String handlerPath = "_plugins/security/api/z/y";
assertFalse((Boolean) restPathMatches.invoke(securityRestFilter, requestPath, handlerPath));
}

@Test
public void testRequestPathWithExtraSegments() throws InvocationTargetException, IllegalAccessException {
String requestPath = "_plugins/security/api/x/y/z";
String handlerPath = "_plugins/security/api/x/y";
assertFalse((Boolean) restPathMatches.invoke(securityRestFilter, requestPath, handlerPath));
}
}
14 changes: 13 additions & 1 deletion src/test/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluatorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ public void testEvaluate_Successful_NewPermission() {
when(securityRoles.impliesClusterPermissionPermission(action)).thenReturn(true);

PrivilegesEvaluatorResponse response = privilegesEvaluator.evaluate(TEST_USER, Set.of(action));

assertTrue(privilegesEvaluator.isInitialized());
assertTrue(response.allowed);
verify(securityRoles).impliesClusterPermissionPermission(any());
}
Expand Down Expand Up @@ -160,4 +160,16 @@ public void testEvaluate_Unsuccessful() {
assertFalse(response.allowed);
verify(securityRoles).impliesClusterPermissionPermission(any());
}

@Test
public void testIsInitialized_False() {
privilegesEvaluator = new RestLayerPrivilegesEvaluator(
clusterService,
threadPool,
mock(AuditLog.class),
mock(ClusterInfoHolder.class),
namedXContentRegistry
);
assertFalse(privilegesEvaluator.isInitialized());
}
}