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

fix: Check of write permission of atKeys file path when submitting enroll request #690

Merged
1 change: 1 addition & 0 deletions packages/at_onboarding_cli/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
## 1.8.0
- feat: add `unrevoke` command to the activate CLI
- feat: add `delete` command to the activate CLI
- fix: When submitting an enrollment request, check for write permissions of AtKeys file path.
## 1.7.0
- feat: add `auto` command to the activate CLI
## 1.6.4
Expand Down
53 changes: 53 additions & 0 deletions packages/at_onboarding_cli/lib/src/cli/auth_cli.dart
Original file line number Diff line number Diff line change
Expand Up @@ -358,11 +358,24 @@ Future<void> enroll(ArgResults argResults, {AtOnboardingService? svc}) async {
}

File f = File(argResults[AuthCliArgs.argNameAtKeys]);

if (f.existsSync()) {
stderr.writeln('Error: atKeys file ${f.path} already exists');
gkc marked this conversation as resolved.
Show resolved Hide resolved
return;
}

// "_isWritable" attempts to create the directories for the given [file] if they do not exist,
// and then tries to open the file in write mode to verify write permissions.
//
// If the file can be opened for writing, it is immediately closed and deleted.
// In [AtOnboardingServiceImpl._generateAtKeysFile] method, there is a check which returns error
// if the file already exists. Therefore, delete the file here after checking for write permissions.
//
// Incase of any exceptions, the error is logged and returned.
if (isWritable(f) == false) {
return;
}

svc ??= createOnboardingService(argResults);

Map<String, String> namespaces = {};
Expand Down Expand Up @@ -416,6 +429,46 @@ Future<void> enroll(ArgResults argResults, {AtOnboardingService? svc}) async {
}
}

/// Checks if the specified [file] is writable.
///
/// This function attempts to create the directories for the given [file] if they do not exist,
/// and then tries to open the file in write mode to verify write permissions.
/// If the file can be opened for writing, it is immediately closed and deleted.
///
/// Returns:
/// - `true` if the file is writable, or
/// - `false` if the file is not writable due to existing files, lack of permissions,
/// or any other exceptions encountered during the process.
///
/// [file]: The [File] instance to check for write access.
///
/// Exceptions:
/// - If the file does not have write permissions, a [PathAccessException] is caught, and an error message is printed to stderr.
/// - Any other exceptions are caught and logged, indicating a failure to determine write access.
@visibleForTesting
bool isWritable(File file) {
try {
// If the directories do not exist, create them.
// "recursive" is set to true to ensure that any missing parent directories are created.
file.createSync(recursive: true);
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we should keep the exclusive:true here also, but allow the PathExistsException to be thrown. I'm a bit uncomfortable with a method called 'isWritable' possibly deleting a file

Maybe a better approach would be for this method to handle each case (file exists, file doesn't exist) differently

  • if file exists, try opening it for FileMode.writeOnlyAppend
  • if file doesn't already exist, try creating it and then removing it

Copy link
Member Author

Choose a reason for hiding this comment

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

@gkc :

  • Renamed "isWritable" method name to "canCreateFile".
  • Added exclusive:true when creating a file and rethrowing the PathExistsException.

Please review and let me know if any further changes are required.

// Try opening the file in write mode, which requires write permissions
RandomAccessFile raf = file.openSync(mode: FileMode.write);
raf.closeSync();
// In [AtOnboardingServiceImpl._generateAtKeysFile] method, there is a check which returns error
// if the file already exists. Therefore, delete the file here after checking for write permissions.
file.deleteSync();
return true;
} on PathAccessException {
stderr.writeln(
'Error : atKeys file ${file.path} does not have write permissions');
return false;
} catch (e) {
// If any exception occurs, we assume the file is not writable
stderr.writeln('Error in writing to atKeys file: ${e.toString()}');
return false;
}
}

@visibleForTesting
Future<void> setSpp(ArgResults argResults, AtClient atClient) async {
String spp = argResults[AuthCliArgs.argNameSpp];
Expand Down
34 changes: 34 additions & 0 deletions packages/at_onboarding_cli/test/auth_cli_test.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
import 'dart:io';

import 'package:at_onboarding_cli/src/cli/auth_cli.dart';
import 'package:test/test.dart';

void main() {
final baseDirPath = 'test/keys';
group('A group of tests to verify write permission of apkam file path', () {
final dirPath = '$baseDirPath/@alice-apkam-keys.atKeys';

test(
'A test to verify isWritable returns false if directory has read-only permissions',
() async {
final directory = Directory(dirPath);
// Create the directory first to ensure it exists before calling isWritable.
await directory.create(recursive: true);
// Set permission to read only.
await Process.run('chmod', ['444', baseDirPath]);
expect(isWritable(File(dirPath)), false);
});

test(
'A test verify isWritable returns true if directory does not have a file already',
() {
expect(isWritable(File(dirPath)), true);
});
});

tearDown(() async {
// Set full permissions to delete the directory.
await Process.run('chmod', ['777', baseDirPath]);
Directory(baseDirPath).deleteSync(recursive: true);
});
}