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

chore: cleanup exception messaging #409

Merged
merged 3 commits into from
Oct 31, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
import com.aws.greengrass.dependency.State;
import com.aws.greengrass.clientdevices.auth.ClientDevicesAuthService;
import com.aws.greengrass.clientdevices.auth.exception.AuthenticationException;
import com.aws.greengrass.clientdevices.auth.exception.CloudServiceInteractionException;
import com.aws.greengrass.clientdevices.auth.session.SessionManager;
import com.aws.greengrass.lifecyclemanager.GlobalStateChangeListener;
import com.aws.greengrass.lifecyclemanager.GreengrassService;
Expand Down Expand Up @@ -254,13 +253,13 @@ void GIVEN_brokerWithInvalidSession_WHEN_GetClientDeviceAuthToken_THEN_throwInva
void GIVEN_brokerWithValidCredentials_WHEN_GetClientDeviceAuthTokenFails_THEN_throwServiceError(
ExtensionContext context) throws Exception {
kernel.getContext().put(SessionManager.class, sessionManager);
ignoreExceptionOfType(context, CloudServiceInteractionException.class);
ignoreExceptionOfType(context, AuthenticationException.class);
startNucleusWithConfig("cda.yaml");
Map<String, String> mqttCredentialMap =
ImmutableMap.of("clientId", "some-client-id", "username", null, "password", null, "certificatePem",
"-----BEGIN CERTIFICATE-----" + System.lineSeparator() + "PEM=" + System.lineSeparator()
+ "-----END CERTIFICATE-----" + System.lineSeparator());
when(sessionManager.createSession("mqtt", mqttCredentialMap)).thenThrow(CloudServiceInteractionException.class);
when(sessionManager.createSession("mqtt", mqttCredentialMap)).thenThrow(AuthenticationException.class);

try (EventStreamRPCConnection connection = IPCTestUtils.getEventStreamRpcConnection(kernel,
"BrokerWithGetClientDeviceAuthTokenPermission")) {
Expand All @@ -273,7 +272,7 @@ void GIVEN_brokerWithValidCredentials_WHEN_GetClientDeviceAuthTokenFails_THEN_th
Exception err = assertThrows(Exception.class, () -> {
clientDeviceAuthToken(ipcClient, request, null);
});
assertEquals(err.getCause().getClass(), ServiceError.class);
assertEquals(InvalidCredentialError.class, err.getCause().getClass());
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -348,14 +348,16 @@ public void configureCustomCA(CAConfiguration configuration) throws InvalidConfi
*
* @param thingName Core device name
* @param certificate the CA to upload to IoT core. Which will be provided by cloud discovery
* @throws CertificateEncodingException If unable to get certificate encoding
* @throws KeyStoreException If unable to retrieve the certificate
* @throws IOException If unable to read certificate
* @throws DeviceConfigurationException If unable to retrieve Greengrass V2 Data client
* @throws CertificateEncodingException If unable to get certificate encoding
* @throws KeyStoreException If unable to retrieve the certificate
* @throws IOException If unable to read certificate
* @throws DeviceConfigurationException If unable to retrieve Greengrass V2 Data client
* @throws CloudServiceInteractionException If cloud call fails
*/
@SuppressWarnings({"PMD.AvoidCatchingGenericException", "PMD.AvoidRethrowingException"})
public void uploadCoreDeviceCAs(String thingName, X509Certificate certificate)
throws CertificateEncodingException, KeyStoreException, IOException, DeviceConfigurationException {
throws CertificateEncodingException, KeyStoreException, IOException,
DeviceConfigurationException, CloudServiceInteractionException {
String certificatePem = CertificateHelper.toPem(certificate);

List<Class> retryAbleExceptions =
Expand Down Expand Up @@ -386,4 +388,4 @@ public void uploadCoreDeviceCAs(String thingName, X509Certificate certificate)
+ "core device's IoT policy grants the greengrass:PutCertificateAuthorities permission.", e);
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

package com.aws.greengrass.clientdevices.auth.exception;

public class CloudServiceInteractionException extends RuntimeException {
public class CloudServiceInteractionException extends Exception {

private static final long serialVersionUID = -1L;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ public static void updateMetadataTrustDurationMinutes(int newTrustDuration) {
metadataTrustDurationMinutes.set(newTrustDuration);
}

private boolean isStatusTrusted() {
public boolean isStatusTrusted() {
Instant validTill = statusLastUpdated.plus(metadataTrustDurationMinutes.get(), ChronoUnit.MINUTES);
return validTill.isAfter(Instant.now());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,13 +41,13 @@
import javax.inject.Inject;

public interface IotAuthClient {
Optional<String> getActiveCertificateId(String certificatePem);
Optional<String> getActiveCertificateId(String certificatePem) throws CloudServiceInteractionException;
Copy link
Member

Choose a reason for hiding this comment

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

this breaks out interface doesn't it? Isn't moquette using these methods? If it is, then this is going to break consumers.

Copy link
Member Author

Choose a reason for hiding this comment

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

consumers just interact with ClientDevicesAuthServiceApi and AuthenticationExceptions from what i can see, so that should be fine?


Optional<Certificate> getIotCertificate(String certificatePem) throws InvalidCertificateException;

boolean isThingAttachedToCertificate(Thing thing, Certificate certificate);
boolean isThingAttachedToCertificate(Thing thing, Certificate certificate) throws CloudServiceInteractionException;

boolean isThingAttachedToCertificate(Thing thing, String certificateId);
boolean isThingAttachedToCertificate(Thing thing, String certificateId) throws CloudServiceInteractionException;


Stream<List<AssociatedClientDevice>> getThingsAssociatedWithCoreDevice();
Expand Down Expand Up @@ -77,7 +77,7 @@ class Default implements IotAuthClient {

@Override
@SuppressWarnings("PMD.AvoidCatchingGenericException")
public Optional<String> getActiveCertificateId(String certificatePem) {
public Optional<String> getActiveCertificateId(String certificatePem) throws CloudServiceInteractionException {
if (Utils.isEmpty(certificatePem)) {
throw new IllegalArgumentException("Certificate PEM is empty");
}
Expand Down Expand Up @@ -135,7 +135,8 @@ public Optional<Certificate> getIotCertificate(String certificatePem) throws Inv
}

@Override
public boolean isThingAttachedToCertificate(Thing thing, Certificate certificate) {
public boolean isThingAttachedToCertificate(Thing thing, Certificate certificate)
throws CloudServiceInteractionException {
if (Objects.isNull(certificate)) {
return false;
}
Expand All @@ -144,7 +145,8 @@ public boolean isThingAttachedToCertificate(Thing thing, Certificate certificate

@Override
@SuppressWarnings({"PMD.AvoidCatchingGenericException", "PMD.AvoidDuplicateLiterals"})
public boolean isThingAttachedToCertificate(Thing thing, String certificateId) {
public boolean isThingAttachedToCertificate(Thing thing, String certificateId)
throws CloudServiceInteractionException {
if (thing == null || Utils.isEmpty(thing.getThingName())) {
throw new IllegalArgumentException("No thing name available to validate");
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@

import com.aws.greengrass.clientdevices.auth.api.UseCases;
import com.aws.greengrass.clientdevices.auth.exception.AuthenticationException;
import com.aws.greengrass.clientdevices.auth.exception.CloudServiceInteractionException;
import com.aws.greengrass.clientdevices.auth.iot.Certificate;
import com.aws.greengrass.clientdevices.auth.iot.CertificateRegistry;
import com.aws.greengrass.clientdevices.auth.iot.InvalidCertificateException;
Expand All @@ -20,7 +19,6 @@
import com.aws.greengrass.logging.api.Logger;
import com.aws.greengrass.logging.impl.LogManager;

import java.util.Optional;
import javax.inject.Inject;

public class CreateIoTThingSession implements UseCases.UseCase<Session, CreateSessionDTO> {
Expand Down Expand Up @@ -53,37 +51,44 @@ public CreateIoTThingSession(ThingRegistry thingRegistry, CertificateRegistry ce
*/
@Override
public Session apply(CreateSessionDTO dto) throws AuthenticationException {
String certificatePem = dto.getCertificatePem();
Certificate certificate = getActiveCertificateFromRegistry(dto);
String thingName = dto.getThingName();
Optional<Certificate> certificate;
Thing thing = thingRegistry.getOrCreateThing(thingName);

try {
certificate = certificateRegistry.getCertificateFromPem(certificatePem);

if (!certificate.isPresent() || !certificate.get().isActive()) {
throw new AuthenticationException("Certificate isn't active");
}

Thing thing = thingRegistry.getOrCreateThing(thingName);
VerifyThingAttachedToCertificate.Result result =
useCases.get(VerifyThingAttachedToCertificate.class)
.apply(new VerifyThingAttachedToCertificateDTO(thingName, certificate.getCertificateId()));

VerifyThingAttachedToCertificate verify = useCases.get(VerifyThingAttachedToCertificate.class);
VerifyThingAttachedToCertificate.Result result = verify.apply(
new VerifyThingAttachedToCertificateDTO(thingName, certificate.get().getCertificateId()));
logger.atDebug()
.kv("thingName", thingName)
.kv("thingHasValidAttachment", result.isThingHasValidAttachmentToCertificate())
.kv("lastAttachedOn", result.getLastAttached())
.kv("attachmentExpiration", result.getAttachmentExpiration())
.kv("source", result.getVerificationSource())
.log("Attachment verification result");

logger.atDebug()
.kv("thingHasValidAttachment", result.isThingHasValidAttachmentToCertificate())
.kv("lastAttachedOn", result.getLastAttached())
.kv("attachmentExpiration", result.getAttachmentExpiration())
.kv("source", result.getVerificationSource())
.log("Attachment verification result");

if (result.isThingHasValidAttachmentToCertificate()) {
return new SessionImpl(certificate.get(), thing);
}
} catch (CloudServiceInteractionException | InvalidCertificateException e) {
throw new AuthenticationException("Failed to verify certificate with cloud", e);
if (result.isThingHasValidAttachmentToCertificate()) {
return new SessionImpl(certificate, thing);
}

throw new AuthenticationException("Failed to verify certificate attached to thing");
}

private Certificate getActiveCertificateFromRegistry(CreateSessionDTO dto) throws AuthenticationException {
Certificate certificate;
try {
certificate = certificateRegistry.getCertificateFromPem(dto.getCertificatePem())
.orElseThrow(() -> new AuthenticationException("Certificate not in local registry"));
} catch (InvalidCertificateException e) {
throw new AuthenticationException("Certificate is invalid", e);
}
if (certificate.isActive()) {
return certificate;
}
if (certificate.isStatusTrusted()) {
throw new AuthenticationException("Certificate is not active");
} else {
throw new AuthenticationException("Certificate is not trusted");
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ private Result verifyLocally(Thing thing, String certificateId) {
.build();
}

private Result verifyFromCloud(Thing thing, String certificateId) {
private Result verifyFromCloud(Thing thing, String certificateId) throws CloudServiceInteractionException {
logger.atDebug().kv("thing", thing.getThingName()).kv("certificate", certificateId)
.log("Network up, verifying thing attached to certificate from cloud");

Expand Down Expand Up @@ -111,7 +111,6 @@ public Result apply(VerifyThingAttachedToCertificateDTO dto) {
if (isNetworkUp()) {
return verifyFromCloud(thing, dto.getCertificateId());
}

return verifyLocally(thing, dto.getCertificateId());
} catch (CloudServiceInteractionException e) {
return verifyLocally(thing, dto.getCertificateId());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -365,8 +365,8 @@ void GIVEN_cloudFailure_WHEN_verifyingAThingCertificateAttachment_THEN_refreshTa
ArgumentCaptor<VerifyThingAttachedToCertificateDTO> doCaptor =
ArgumentCaptor.forClass(VerifyThingAttachedToCertificateDTO.class);

when(verifyThingAttachedToCertificateMock.apply(doCaptor.capture())).thenThrow(
new CloudServiceInteractionException("Failed to verify association")).thenReturn(
when(verifyThingAttachedToCertificateMock.apply(doCaptor.capture())).thenReturn(
VerifyThingAttachedToCertificate.Result.builder().thingHasValidAttachmentToCertificate(false).build()).thenReturn(
VerifyThingAttachedToCertificate.Result.builder().thingHasValidAttachmentToCertificate(true).build());
Instant twentyFourHoursLater = now.plus(Duration.ofHours(24));
mockInstant(twentyFourHoursLater.toEpochMilli());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ void beforeEach() throws DeviceConfigurationException {
}

@Test
void GIVEN_certificatePem_and_cloudProperResponse_WHEN_getActiveCertificateId_THEN_certificateIdReturned() {
void GIVEN_certificatePem_and_cloudProperResponse_WHEN_getActiveCertificateId_THEN_certificateIdReturned() throws CloudServiceInteractionException {
VerifyClientDeviceIdentityResponse response =
VerifyClientDeviceIdentityResponse.builder().clientDeviceCertificateId("certificateId").build();
when(client.verifyClientDeviceIdentity(any(VerifyClientDeviceIdentityRequest.class))).thenReturn(response);
Expand All @@ -83,7 +83,7 @@ void GIVEN_certificatePem_and_cloudProperResponse_WHEN_getActiveCertificateId_TH
}

@Test
void GIVEN_cloudThrowValidationException_WHEN_getActiveCertificateId_THEN_returnNull(ExtensionContext context) {
void GIVEN_cloudThrowValidationException_WHEN_getActiveCertificateId_THEN_returnNull(ExtensionContext context) throws CloudServiceInteractionException {
ignoreExceptionOfType(context, ValidationException.class);
when(client.verifyClientDeviceIdentity(any(VerifyClientDeviceIdentityRequest.class))).thenThrow(
ValidationException.class);
Expand Down Expand Up @@ -130,7 +130,7 @@ void GIVEN_threadGotInterrupted_WHEN_getActiveCertificateId_THEN_throwCloudInter
}

@Test
void GIVEN_certificateAndThing_and_cloudVerificationSuccess_WHEN_isThingAttachedToCertificate_THEN_returnTrue() {
void GIVEN_certificateAndThing_and_cloudVerificationSuccess_WHEN_isThingAttachedToCertificate_THEN_returnTrue() throws CloudServiceInteractionException {
when(thing.getThingName()).thenReturn("thingName");
when(certificate.getCertificateId()).thenReturn("certificateId");
when(client.verifyClientDeviceIoTCertificateAssociation(
Expand All @@ -145,8 +145,7 @@ void GIVEN_certificateAndThing_and_cloudVerificationSuccess_WHEN_isThingAttached
}

@Test
void GIVEN_cloudThrowValidationException_WHEN_isThingAttachedToCertificate_THEN_returnFalse(
ExtensionContext context) {
void GIVEN_cloudThrowValidationException_WHEN_isThingAttachedToCertificate_THEN_returnFalse(ExtensionContext context) throws CloudServiceInteractionException{
ignoreExceptionOfType(context, ValidationException.class);
when(thing.getThingName()).thenReturn("thingName");
when(certificate.getCertificateId()).thenReturn("certificateId");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ void afterEach() throws IOException {

@Test
void GIVEN_credentialsWithUnknownClientId_WHEN_createSession_THEN_throwsAuthenticationException()
throws InvalidCertificateException {
throws InvalidCertificateException, CloudServiceInteractionException {
when(mockNetworkState.getConnectionState()).thenReturn(NetworkStateProvider.ConnectionState.NETWORK_UP);
when(mockCertificateRegistry.getCertificateFromPem(any())).thenReturn(
Optional.of(CertificateFake.activeCertificate()));
Expand All @@ -102,7 +102,7 @@ void GIVEN_credentialsWithInvalidCertificate_WHEN_createSession_THEN_throwsAuthe

@Test
void GIVEN_credentialsWithCertificate_WHEN_createSession_AND_cloudError_THEN_throwsAuthenticationException()
throws InvalidCertificateException {
throws InvalidCertificateException, CloudServiceInteractionException {
when(mockNetworkState.getConnectionState()).thenReturn(NetworkStateProvider.ConnectionState.NETWORK_UP);
when(mockCertificateRegistry.getCertificateFromPem(any())).thenReturn(
Optional.of(CertificateFake.activeCertificate()));
Expand All @@ -115,7 +115,7 @@ void GIVEN_credentialsWithCertificate_WHEN_createSession_AND_cloudError_THEN_thr

@Test
void GIVEN_credentialsWithValidClientId_WHEN_createSession_THEN_returnsSession()
throws AuthenticationException, InvalidCertificateException {
throws AuthenticationException, InvalidCertificateException, CloudServiceInteractionException {
when(mockNetworkState.getConnectionState()).thenReturn(NetworkStateProvider.ConnectionState.NETWORK_UP);
when(mockThingRegistry.getOrCreateThing("clientId")).thenReturn(Thing.of("clientId"));
when(mockThingRegistry.getThing(any())).thenReturn(Thing.of("clientId"));
Expand Down
Loading