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.

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

fix: Add retry logic in "_waitForPkamAuthSuccess" to retry when secondary is temporarily not reachable #684

Merged
merged 4 commits into from
Oct 8, 2024
Merged

fix: Add retry logic in "_waitForPkamAuthSuccess" to retry when secondary is temporarily not reachable #684

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
502d708
fix: Add retry logic in "_waitForPkamAuthSuccess" to retry when secon…
sitaram-kalluri Oct 7, 2024
f5e2b3f
fix: Add dart docs and modify the code
sitaram-kalluri Oct 8, 2024
a612f70
fix: Modify error message to fix failing functional test
sitaram-kalluri Oct 8, 2024
cf24f66
fix: Remove _attemptPkamAuth method and invoke pkamAuthenticate in _w…
sitaram-kalluri Oct 8, 2024
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
66 changes: 34 additions & 32 deletions packages/at_onboarding_cli/lib/src/onboard/at_onboarding_service_impl.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ class AtOnboardingServiceImpl implements AtOnboardingService {
AtSignLogger logger = AtSignLogger('OnboardingCli');
AtOnboardingPreference atOnboardingPreference;
AtLookUp? _atLookUp;
final _maxActivationRetries = 5;

/// The object which controls what types of AtClients, NotificationServices
/// and SyncServices get created when we call [AtClientManager.setCurrentAtSign].
Expand Down Expand Up @@ -321,15 +322,44 @@ class AtOnboardingServiceImpl implements AtOnboardingService {
Duration retryInterval, {
bool logProgress = true,
}) async {
int retryAttempt = 1;
while (true) {
logger.info('Attempting pkam auth');
if (logProgress) {
stderr.write('Checking ... ');
}
bool pkamAuthSucceeded = await _attemptPkamAuth(
atLookUp,
enrollmentIdFromServer,
);
bool pkamAuthSucceeded = false;
try {
// _attemptPkamAuth returns boolean value true when authentication is successful.
// Returns UnAuthenticatedException when authentication fails.
pkamAuthSucceeded = await atLookUp.pkamAuthenticate(
enrollmentId: enrollmentIdFromServer);
} on UnAuthenticatedException catch (e) {
// Error codes AT0401 and AT0026 indicate authentication failure due to unapproved enrollment. Retry until the enrollment is approved.
// The variable _pkamAuthSucceeded is false, allowing for PKAM authentication retries.
// Avoid checking "retryAttempt > _maxActivationRetries" here, as we want to continue retrying until enrollment is approved.
// The check for "retryAttempt > _maxActivationRetries" should only occur when the secondary server is unreachable due to network issues.
if (e.message.contains('error:AT0401') ||
e.message.contains('error:AT0026')) {
logger.info('Pkam auth failed: ${e.message}');
}
// Error code AT0025 represents Enrollment denied. Therefore, no need to retry; throw exception.
else if (e.message.contains('error:AT0025')) {
throw AtEnrollmentException(
'The enrollment: $enrollmentIdFromServer is denied');
}
} catch (e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does _attemptPkamAuth throw an exception if PKAM auth fails in a "normal" way (i.e. the enrollment has not yet been approved)?

Some code comments explaining what exactly is going on here would be useful

Copy link
Member Author

@sitaram-kalluri sitaram-kalluri Oct 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In the _attemptPkamAuth method, if enrollment is denied, it returns an AtEnrollmentException. This exception is caught in the catch block of _waitForPkamAuthSuccess (which calls _attemptPkamAuth),
triggering the retry logic that we want to avoid. To prevent this, we need to handle AtEnrollmentException separately here, which introduces code duplication.

To handle all the exceptions in one place, moved the existing exception handing logic in _attemptPkamAuth here. With the changes, _attemptPkamAuth do not handle exception. It rethrows and all the exceptions are handled in the _waitForPkamAuthSuccess.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK - I'd suggest we now remove _attemptPkamAuth and move its code inline, as _attemptPkamAuth isn't used anywhere else

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed the "_attemptPkamAuth" and moved its code inline.

String message =
'Exception occurred when authenticating the atSign: $_atSign caused by ${e.toString()}';
if (retryAttempt > _maxActivationRetries) {
message += ' Activation failed after $_maxActivationRetries attempts';
logger.severe(message);
rethrow;
}
logger
.severe('$message. Attempting to retry for $retryAttempt attempt');
retryAttempt++;
}
if (pkamAuthSucceeded) {
if (logProgress) {
stderr.writeln(' approved.');
Expand All @@ -347,34 +377,6 @@ class AtOnboardingServiceImpl implements AtOnboardingService {
}
}

/// Try a single PKAM auth
Future<bool> _attemptPkamAuth(AtLookUp atLookUp, String enrollmentId) async {
try {
logger.finer('_attemptPkamAuth: Calling atLookUp.pkamAuthenticate');
var pkamResult =
await atLookUp.pkamAuthenticate(enrollmentId: enrollmentId);
logger.finer(
'_attemptPkamAuth: atLookUp.pkamAuthenticate returned $pkamResult');
if (pkamResult) {
return true;
}
} on UnAuthenticatedException catch (e) {
if (e.message.contains('error:AT0401') ||
e.message.contains('error:AT0026')) {
logger.info('Pkam auth failed: ${e.message}');
return false;
} else if (e.message.contains('error:AT0025')) {
throw AtEnrollmentException('enrollment denied');
}
} catch (e) {
logger.shout('Unexpected exception: $e');
rethrow;
} finally {
logger.finer('_attemptPkamAuth: complete');
}
return false;
}

///write newly created encryption keypairs into atKeys file
Future<File> _generateAtKeysFile(
at_auth.AtAuthKeys atAuthKeys, {
Expand Down
10 changes: 7 additions & 3 deletions tests/at_onboarding_cli_functional_tests/test/enrollment_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,10 +237,14 @@ void main() {
var completer = Completer<void>(); // Create a Completer

//4. Subscribe to enrollment notifications; we will deny it when it arrives
String enrollmentId = '';
onboardingService_1.atClient!.notificationService
.subscribe(regex: '.__manage')
.listen(expectAsync1((notification) async {
logger.finer('got enroll notification');
final notificationKey = notification.key;
enrollmentId = notificationKey.substring(
0, notificationKey.indexOf('.new.enrollments'));
expect(notification.value, isNotNull);
var notificationValueJson = jsonDecode(notification.value!);
expect(notificationValueJson['encryptedApkamSymmetricKey'],
Expand All @@ -258,7 +262,7 @@ void main() {
AtOnboardingServiceImpl? onboardingService_2 =
AtOnboardingServiceImpl(atSign, preference_1);

Future<dynamic> expectLaterFuture = expectLater(
expectLater(
onboardingService_2.enroll(
'buzz',
'iphone',
Expand All @@ -267,8 +271,8 @@ void main() {
retryInterval: Duration(seconds: 5),
),
throwsA(predicate((dynamic e) =>
e is AtEnrollmentException && e.message == 'enrollment denied')));
print(await expectLaterFuture);
e is AtEnrollmentException &&
e.message == 'The enrollment: $enrollmentId is denied')));
await completer.future;

await onboardingService_1.close();
Expand Down