From 1dd2f63eb54befa7b347051c509d33dd8448bff0 Mon Sep 17 00:00:00 2001 From: Christian Kollmann Date: Fri, 18 Dec 2020 10:22:59 +0100 Subject: Review code --- .../modules/auth/eidas/v2/ernb/DummyErnbClient.java | 1 + .../eidas/v2/handler/DeSpecificDetailSearchProcessor.java | 1 + .../v2/handler/ICountrySpecificDetailSearchProcessor.java | 1 + .../modules/auth/eidas/v2/tasks/InitialSearchTask.java | 11 ++++++++++- .../specific/modules/auth/eidas/v2/zmr/DummyZmrClient.java | 1 + .../eidas/v2/test/tasks/InitialSearchTaskFirstTest.java | 13 ++++++++++++- 6 files changed, 26 insertions(+), 2 deletions(-) diff --git a/eidas_modules/authmodule-eIDAS-v2/src/main/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/ernb/DummyErnbClient.java b/eidas_modules/authmodule-eIDAS-v2/src/main/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/ernb/DummyErnbClient.java index 0c8a2f59..978be4d0 100644 --- a/eidas_modules/authmodule-eIDAS-v2/src/main/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/ernb/DummyErnbClient.java +++ b/eidas_modules/authmodule-eIDAS-v2/src/main/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/ernb/DummyErnbClient.java @@ -11,6 +11,7 @@ public class DummyErnbClient implements IErnbClient { @Override public ArrayList searchWithPersonIdentifer(String personIdentifer) { + // NOTE: Are we using Mockito or these fixed strings for testing, why are those defined here? switch (personIdentifer) { case "a12345": case "a12345-": diff --git a/eidas_modules/authmodule-eIDAS-v2/src/main/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/handler/DeSpecificDetailSearchProcessor.java b/eidas_modules/authmodule-eIDAS-v2/src/main/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/handler/DeSpecificDetailSearchProcessor.java index 51d6952f..b5e8551b 100644 --- a/eidas_modules/authmodule-eIDAS-v2/src/main/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/handler/DeSpecificDetailSearchProcessor.java +++ b/eidas_modules/authmodule-eIDAS-v2/src/main/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/handler/DeSpecificDetailSearchProcessor.java @@ -10,6 +10,7 @@ public class DeSpecificDetailSearchProcessor extends ICountrySpecificDetailSearc @Override public boolean canHandle(String countryCode, SimpleEidasData eidData) { + // NOTE: Please extract constant for "de" if (!countryCode.equalsIgnoreCase("de")) { return false; } diff --git a/eidas_modules/authmodule-eIDAS-v2/src/main/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/handler/ICountrySpecificDetailSearchProcessor.java b/eidas_modules/authmodule-eIDAS-v2/src/main/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/handler/ICountrySpecificDetailSearchProcessor.java index 3d6b35e9..b9ab2ceb 100644 --- a/eidas_modules/authmodule-eIDAS-v2/src/main/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/handler/ICountrySpecificDetailSearchProcessor.java +++ b/eidas_modules/authmodule-eIDAS-v2/src/main/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/handler/ICountrySpecificDetailSearchProcessor.java @@ -31,6 +31,7 @@ import org.springframework.beans.factory.annotation.Autowired; public abstract class ICountrySpecificDetailSearchProcessor { + // NOTE: Please use constructor injection protected IErnbClient ernbClient; protected IZmrClient zmrClient; diff --git a/eidas_modules/authmodule-eIDAS-v2/src/main/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/tasks/InitialSearchTask.java b/eidas_modules/authmodule-eIDAS-v2/src/main/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/tasks/InitialSearchTask.java index 5066ac85..4142b68b 100644 --- a/eidas_modules/authmodule-eIDAS-v2/src/main/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/tasks/InitialSearchTask.java +++ b/eidas_modules/authmodule-eIDAS-v2/src/main/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/tasks/InitialSearchTask.java @@ -70,6 +70,8 @@ import lombok.extern.slf4j.Slf4j; */ @Slf4j @Component("InitialSearchTask") +// NOTE: General: Please rebase git commit and squash them where useful, i.e. "remove unused import" should +// not be a separate commit. public class InitialSearchTask extends AbstractAuthServletTask { private List handlers = new ArrayList<>(); @@ -96,6 +98,7 @@ public class InitialSearchTask extends AbstractAuthServletTask { final ILightResponse eidasResponse = authProcessData .getGenericDataFromSession(Constants.DATA_FULL_EIDAS_RESPONSE, ILightResponse.class); + // NOTE: Why is eidas first converted to a map, and then to a SimpleEidasData? final Map simpleAttrMap = convertEidasAttrToSimpleMap( eidasResponse.getAttributes().getAttributeMap()); @@ -110,14 +113,17 @@ public class InitialSearchTask extends AbstractAuthServletTask { } } + // NOTE: Please rename methods ... "step2" doesn't tell the reader anything private String step2(SimpleEidasData eidData) throws TaskExecutionException { String personIdentifier = eidData.getPseudonym(); + // NOTE: Is that comment really necessary? //search in register(step 2) MergedRegisterSearchResult result = searchInZmrAndErnp(personIdentifier); if (result.getResultCount() == 0) { return step5(result, eidData); } else if (result.getResultCount() == 1) { return step3(result, eidData); + // NOTE: Why is that code commented-out? } //else if (result.getResultCount() > 1) { throw new TaskExecutionException(pendingReq, "Initial search - Kitt Process necessary.", new ManualFixNecessaryException(personIdentifier)); @@ -183,7 +189,7 @@ public class InitialSearchTask extends AbstractAuthServletTask { log.debug("Update " + result + " with " + eidData); //TODO - + // NOTE: Sometimes the bpk is returned, sometimes "105"? return result.getBpk(); } @@ -269,6 +275,7 @@ public class InitialSearchTask extends AbstractAuthServletTask { log.debug("Automerge " + initialSearchResult + " with " + eidData + " " + mdsSearchResult); //TODO + // NOTE: Sometimes the bpk is returned, sometimes "105"? return "105"; } @@ -336,6 +343,7 @@ public class InitialSearchTask extends AbstractAuthServletTask { //just for testing //TODO is there a nicer solution? + // There is: Constructor Injection see https://reflectoring.io/constructor-injection/ or https://www.baeldung.com/constructor-injection-in-spring @Autowired public void setErnbClient(IErnbClient ernbClient) { this.ernbClient = ernbClient; @@ -349,6 +357,7 @@ public class InitialSearchTask extends AbstractAuthServletTask { @Autowired public void setHandlers(List handlers) { this.handlers = handlers; + // NOTE: There's a typo in "registrated" log.info("# " + handlers.size() + " country specific detail search services are registrated"); } diff --git a/eidas_modules/authmodule-eIDAS-v2/src/main/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/zmr/DummyZmrClient.java b/eidas_modules/authmodule-eIDAS-v2/src/main/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/zmr/DummyZmrClient.java index 0f3436d8..3af2e39e 100644 --- a/eidas_modules/authmodule-eIDAS-v2/src/main/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/zmr/DummyZmrClient.java +++ b/eidas_modules/authmodule-eIDAS-v2/src/main/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/zmr/DummyZmrClient.java @@ -11,6 +11,7 @@ public class DummyZmrClient implements IZmrClient { @Override public ArrayList searchWithPersonIdentifer(String personIdentifer) { + // NOTE: Are we using Mockito or these fixed strings for testing, why are those defined here? switch (personIdentifer) { case "a12345": case "a12345-": diff --git a/eidas_modules/authmodule-eIDAS-v2/src/test/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/test/tasks/InitialSearchTaskFirstTest.java b/eidas_modules/authmodule-eIDAS-v2/src/test/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/test/tasks/InitialSearchTaskFirstTest.java index 64a73bda..d366fefc 100644 --- a/eidas_modules/authmodule-eIDAS-v2/src/test/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/test/tasks/InitialSearchTaskFirstTest.java +++ b/eidas_modules/authmodule-eIDAS-v2/src/test/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/test/tasks/InitialSearchTaskFirstTest.java @@ -62,9 +62,11 @@ public class InitialSearchTaskFirstTest { @InjectMocks private InitialSearchTask task; + // NOTE: Is defined as @Mock, but also manually mocked in "testNode100a" etc -- why? @Mock private IZmrClient zmrClient; + // NOTE: Is defined as @Mock, but also manually mocked in "testNode100a" etc -- why? @Mock private IErnbClient ernbClient; @@ -92,7 +94,7 @@ public class InitialSearchTaskFirstTest { */ @Before public void setUp() throws URISyntaxException, EaafStorageException { - + // NOTE: PowerMockito should not be needed, as we don't want to test static and private methods task = PowerMockito.spy(task); httpReq = new MockHttpServletRequest("POST", "https://localhost/authhandler"); @@ -112,6 +114,7 @@ public class InitialSearchTaskFirstTest { /** * One match, but register update needed */ + // NOTE: Why is the method named "testNode100a"? public void testNode100a() throws Exception { //Mock ZMR @@ -119,6 +122,9 @@ public class InitialSearchTaskFirstTest { String randomBpk = RandomStringUtils.randomNumeric(6); zmrResult.add(new RegisterResult(randomBpk,"de/st/max123", "Max_new", "Mustermann", "2011-01-01")); + // NOTE: Are we using Mockito or these fixed strings in DummyZmrClient? + // NOTE: Please mock an interface, not a concrete class + // NOTE: But DummyZmrClient is also defined as a bean "ZmrClientForeIDAS" in "eidas_v2_auth.beans.xml"? zmrClient = Mockito.mock(DummyZmrClient.class); Mockito.when(zmrClient.searchWithPersonIdentifer("max123")).thenReturn(zmrResult);//"de/st/max123"??? task.setZmrClient(zmrClient); @@ -137,11 +143,13 @@ public class InitialSearchTaskFirstTest { Assert.assertTrue("Wrong bpk", bPk.equals(randomBpk)); } catch (final TaskExecutionException e) { + // NOTE: assertTrue is probably the wrong method to use ... why catch the exception anyway? Assert.assertTrue("Wrong workflow, should not reach this point", false); } } @Test + // NOTE: Why is @DirtiesContext after each test necessary? What is changed in the context and why? @DirtiesContext /** * One match, but register update needed @@ -563,6 +571,7 @@ public class InitialSearchTaskFirstTest { String bPk = (String) pendingReq.getSessionData(AuthProcessDataWrapper.class).getGenericDataFromSession(Constants.DATA_RESULT_MATCHING_BPK); + // NOTE: Why "105"? Extract in a constant Assert.assertTrue("Wrong bpk", bPk.equals("105")); } catch (final TaskExecutionException e) { Assert.assertTrue("Wrong workflow, should not reach this point", false); @@ -578,6 +587,8 @@ public class InitialSearchTaskFirstTest { @NotNull private AuthenticationResponse buildDummyAuthResponseMaxMustermann() throws URISyntaxException { + // NOTE: Those strings "de/st/max123" seem to be somehow relevant, but where do we need to use that exact string again? + // NOTE: If not, why not using random strings? return buildDummyAuthResponse("Max", "Mustermann", "de/st/max123", "2011-01-01"); } -- cgit v1.2.3