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
3 changes: 2 additions & 1 deletion packages/at_onboarding_cli/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
## 1.7.1
- When submitting an enrollment request, check for write permissions of AtKeys file path.

- 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
13 changes: 6 additions & 7 deletions packages/at_onboarding_cli/lib/src/cli/auth_cli.dart
Original file line number Diff line number Diff line change
Expand Up @@ -351,6 +351,11 @@ 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');
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.
//
Expand Down Expand Up @@ -430,27 +435,21 @@ Future<void> enroll(ArgResults argResults, {AtOnboardingService? svc}) async {
/// [file]: The [File] instance to check for write access.
///
/// Exceptions:
/// - If the file already exists, a [PathExistsException] is caught, and an error message is printed to stderr.
/// - 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.
// "exclusive" is set to true to check if the file already exists; if it does,
// an error will be logged and returned.
file.createSync(recursive: true, exclusive: true);
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 PathExistsException {
stderr.writeln('Error: atKeys file ${file.path} already exists');
return false;
} on PathAccessException {
stderr.writeln(
'Error : atKeys file ${file.path} does not have write permissions');
Expand Down
9 changes: 0 additions & 9 deletions packages/at_onboarding_cli/test/auth_cli_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,6 @@ void main() {
expect(isWritable(File(dirPath)), false);
});

test(
'A test to verify isWritable returns false if file already exists in the directory',
() async {
final directory = Directory(dirPath);
// Create the directory first to ensure it exists before calling isWritable.
await directory.create(recursive: true);
expect(isWritable(File(dirPath)), false);
});

test(
'A test verify isWritable returns true if directory does not have a file already',
() {
Expand Down