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

Added the junit test cases for CustomAuthorizationInterceptor #71

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

vsinghht
Copy link

@vsinghht vsinghht commented Aug 24, 2023

Overview

  • Added the tests for CustomAuthorizationInterceptor.

How it was tested

  • Tested by running the tests locally.

Checklist

  • The title contains a short meaningful summary
  • I have added a link to this PR in the Jira issue
  • I have described how this was tested
  • I have included screen shots for changes that affect the user interface
  • I have updated unit tests
  • I have run unit tests locally
  • I have updated documentation (including README)

@vsinghht vsinghht marked this pull request as ready for review August 24, 2023 12:41
@vsinghht vsinghht changed the title Junit tests for custom authorization interceptor Junit tests for CustomAuthorizationInterceptor Aug 24, 2023
@vsinghht vsinghht changed the title Junit tests for CustomAuthorizationInterceptor Added the junit test cases for CustomAuthorizationInterceptor Aug 24, 2023
@@ -87,7 +87,7 @@ private List<IAuthRule> unauthorizedRule() {
.build();
}

private List<IAuthRule> authorizeOAuth(RequestDetails theRequest) throws AuthenticationException {
protected List<IAuthRule> authorizeOAuth(RequestDetails theRequest) throws AuthenticationException {

Choose a reason for hiding this comment

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

No - tests shouldn't require changes to the underlying methods.

Copy link

@hankwallace hankwallace left a comment

Choose a reason for hiding this comment

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

Copy link

@hankwallace hankwallace left a comment

Choose a reason for hiding this comment

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

Did you review how the upstream repos implement interceptor tests?

Ex: https://github.com/hapifhir/hapi-fhir/blob/e2ca967fd987ceb506a75b85dd764895e0a98f14/hapi-fhir-server-openapi/src/test/java/ca/uhn/fhir/rest/openapi/OpenApiInterceptorWithAuthorizationInterceptorTest.java#L35

Please, do the following:

  1. Remove the tests for API_KEY. We'll deprecate that very soon.
  2. Separate the tests into different ones for remaining authorization methods.

private static final String ROLE_INVALID_USER = "invalid-user";

@BeforeEach
public void before() throws IOException {

Choose a reason for hiding this comment

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

Why would a before() throw an exception?

private static Server ourServer;
private static final FhirContext ourCtx = FhirContext.forR4();

private static final String TOKEN = "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiYWRtaW4iOnRydWUsImlhdCI6MTUxNjIzOTAyMn0.NHVaYe26MbtOYhSKkoKYdFVomg4i8ZJd8_-RU8VNbftc4TSMb4bXP3l3YlNWACwyXPGffz5aXHc6lty1Y2t4SWRqGteragsVdZufDn5BlnJl9pdR_kdVFUsra2rWKEofkZeIC4yWytE58sMIihvo9H1ScmmVwBcQP6XETqYd0aSHp1gOa9RdUPDvoXQ5oqygTqVtxaDr6wUFKrKItgBMzWIdNZ6y7O9E0DhEPTbE9rfBo6KTFsHAZnMg4k68CDp2woYIaXbmYTWcvbzIuHO7_37GT79XdIwkm95QJ7hYC9RiwrV7mesbY4PAahERJawntho0my942XheVLmGwLMBkQ";

Choose a reason for hiding this comment

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

Why is there a single hard-coded token?

Copy link
Author

Choose a reason for hiding this comment

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

This is test token we have return below, as
DecodedJWT jwt = JWT.decode(token);
method in CustomAuthorizationInterceptor requires a valid JWT to decode or we should mock this method as well.

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.

3 participants