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

Conversation

sitaram-kalluri
Copy link
Member

- What I did

  • When submitting the enrollment request, checks for the write permissions of the atKeys filepath. If the filePath has write permission, then continues with submission of enrollment request, else logs error and quits.

- How I did it

  • Introduce "isWritable" method which 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.
  • Removed the logic that checks for the existence of the atKeys file from the enroll method, as this verification will now be handled by the isWritable method when creating the atKeys file to confirm write permissions.

- How to verify it

  • Added the below unit tests to assert the behavior of the isWritable method:
    • A test to verify isWritable returns false if directory has only read-only permissions
    • A test to verify isWritable returns false if file already exists in the directory
    • A test verify isWritable returns true if directory does not have a file already

- Description for the changelog

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

Copy link
Contributor

@gkc gkc left a comment

Choose a reason for hiding this comment

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

@sitaram-kalluri I think the logic here is faulty. Consider the following scenario.
. User onboards an atsign using onboard command - standard atKeys file created
. On the same host, the user attempts an enrollment using the enroll command leaving the keys file as default
. isWritable confirms the standard atKeys file is writable, and then deletes it
. the original atKeys file is now gone

@sitaram-kalluri
Copy link
Member Author

@sitaram-kalluri I think the logic here is faulty. Consider the following scenario. . User onboards an atsign using onboard command - standard atKeys file created . On the same host, the user attempts an enrollment using the enroll command leaving the keys file as default . isWritable confirms the standard atKeys file is writable, and then deletes it . the original atKeys file is now gone

@gkc : The keys file is mandatory for the enroll command. Attaching the log snippet below

sitaram@sitaram-ThinkPad-E14:~/IdeaProjects/atsign/core/at_libraries/packages/at_onboarding_cli/bin$ dart activate_cli.dart enroll -s ABC123 -p wavi -d local -n wavi:rw -a @alice🛠 -r vip.ve.atsign.zone
Argument error for command enroll: The --keys option is mandatory for the "enroll" command
Usage: enroll
    -s, --passcode (mandatory)      The passcode to present with this enrollment request.

The below condition in the enroll method ensure the atKeys file path is mandatory:

@visibleForTesting
Future<void> enroll(ArgResults argResults, {AtOnboardingService? svc}) async {
  if (!argResults.wasParsed(AuthCliArgs.argNameAtKeys)) {
    throw ArgumentError('The --${AuthCliArgs.argNameAtKeys} option is'
        ' mandatory for the "enroll" command');
  }

Further, if the user gives the same atKeys filepath for enroll command as of the original atKeys, then it would return an error stating the atKeys file exists but does not delete the original atKeys file.

@sitaram-kalluri sitaram-kalluri requested a review from gkc October 14, 2024 14:51
Copy link
Contributor

@gkc gkc left a comment

Choose a reason for hiding this comment

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

@sitaram-kalluri you state

Further, if the user gives the same atKeys filepath for enroll command as of the original atKeys, then it would return an error stating the atKeys file exists but does not delete the original atKeys file.

However, you have removed that specific check for the file already existing

@@ -1,3 +1,5 @@
## 1.7.1
- When submitting an enrollment request, check for write permissions of AtKeys file path.
Copy link
Contributor

Choose a reason for hiding this comment

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

add 'fix: ' at the start of this comment please

Copy link
Member Author

Choose a reason for hiding this comment

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

Added "fix:" in the changelog.md

@sitaram-kalluri sitaram-kalluri requested a review from gkc October 15, 2024 11:57
// "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.

@sitaram-kalluri sitaram-kalluri requested a review from gkc October 17, 2024 02:59
@gkc gkc merged commit 4ea7cc5 into trunk Oct 17, 2024
11 checks passed
@gkc gkc deleted the 668-ensure-atkeys-file-is-writeable-before-finalizing-pkam-apkam branch October 17, 2024 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ensure atKeys file is writeable before finalizing pkam / apkam
2 participants