[RORDEV-1502] Fixed ror-tools file permissions #1125
Conversation
mgoworko
changed the title
📝 WalkthroughWalkthroughThe changes refactor file modification logic across several components to consistently preserve original file permissions and ownership. The Sequence Diagram(s)sequenceDiagram
participant Caller
participant FileUtils
participant FileSystem
Caller->>FileUtils: modifyFileWithMaintainingOriginalPermissionsAndOwner(file, modifyFn)
FileUtils->>FileSystem: Read file owner and permissions
FileUtils->>FileUtils: Call modifyFn(file)
FileUtils->>FileSystem: Restore file owner and permissions
FileUtils-->>Caller: Return (Unit or new file reference)
sequenceDiagram
participant RorPluginDirectory
participant FileUtils
participant FileSystem
RorPluginDirectory->>FileUtils: modifyFileWithMaintainingOriginalPermissionsAndOwner(file, copyFn)
FileUtils->>FileSystem: Read file owner and permissions
FileUtils->>FileUtils: copyFn(file) (e.g., os.copy with copyAttributes)
FileUtils->>FileSystem: Restore file owner and permissions
FileUtils-->>RorPluginDirectory: Complete backup/restore
sequenceDiagram
participant JarManifestModifier
participant FileUtils
participant FileSystem
JarManifestModifier->>FileUtils: modifyFileWithMaintainingOriginalPermissionsAndOwner(file, patchManifestFn)
FileUtils->>FileSystem: Read file owner and permissions
FileUtils->>FileUtils: patchManifestFn(file) (modify manifest, use temp files, etc.)
FileUtils->>FileSystem: Restore file owner and permissions
FileUtils-->>JarManifestModifier: Complete manifest modification
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/utils/FileUtils.scala (1)
38-44: Consider exception safety during permission restoration.The generalized method design is solid, but there's no exception handling around the permission restoration. If
modifyJarsucceeds but permission restoration fails, the file could be left in an inconsistent state.Consider wrapping the permission restoration in a try-catch:
def modifyFileWithMaintainingOriginalPermissionsAndOwner[FILE](file: FILE, toJavaFile: FILE => File)(modifyJar: FILE => FILE): Unit = { val originalFileOwner = Files.getOwner(toJavaFile(file).toPath) val originalFilePermissions = getOriginalPermissions(toJavaFile(file).toPath) val resultFile = modifyJar(file) - Files.setOwner(toJavaFile(resultFile).toPath, originalFileOwner) - setOriginalPermissions(toJavaFile(resultFile).toPath, originalFilePermissions) + try { + Files.setOwner(toJavaFile(resultFile).toPath, originalFileOwner) + setOriginalPermissions(toJavaFile(resultFile).toPath, originalFilePermissions) + } catch { + case ex: Exception => + // Log warning but don't fail the operation + System.err.println(s"Warning: Could not restore file permissions: ${ex.getMessage}") + } }ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/internal/filePatchers/JarManifestModifier.scala (1)
32-50: Excellent: JAR modification now preserves file permissions.The refactoring properly wraps the JAR modification logic to preserve original file permissions and ownership. This should resolve the permission issues mentioned in the comment on line 36.
The temp file cleanup happens automatically via
moveTo, but consider adding explicit cleanup in failure scenarios:+ try { tempJarFile.moveTo(file)(File.CopyOptions(overwrite = true)) + file + } catch { + case ex: Exception => + tempJarFile.delete(swallowIOExceptions = true) + throw ex + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/internal/RorPluginDirectory.scala(2 hunks)ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/internal/filePatchers/JarManifestModifier.scala(2 hunks)ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/utils/FileUtils.scala(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/internal/filePatchers/JarManifestModifier.scala (1)
ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/utils/FileUtils.scala (3)
FileUtils(25-78)modifyFileWithMaintainingOriginalPermissionsAndOwner(34-36)modifyFileWithMaintainingOriginalPermissionsAndOwner(38-44)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: ror (Run all tests IT_es67x)
- GitHub Check: ror (Run all tests IT_es80x)
- GitHub Check: ror (Run all tests IT_es70x)
- GitHub Check: ror (Run all tests IT_es710x)
- GitHub Check: ror (Run all tests IT_es810x)
- GitHub Check: ror (CVE check Job)
- GitHub Check: ror (Run all tests IT_es90x)
- GitHub Check: ror (Run all tests IT_es717x)
- GitHub Check: ror (Run all tests IT_es818x)
- GitHub Check: ror (Run all tests UNIT)
- GitHub Check: ror (Run all tests LICENSE)
🔇 Additional comments (5)
ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/utils/FileUtils.scala (1)
34-36: Clean backward compatibility preservation.The wrapper method maintains the existing API while delegating to the generalized implementation.
ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/internal/RorPluginDirectory.scala (3)
26-26: LGTM: Proper import for the utility function.
50-55: Excellent: File permissions preserved during backup.The backup operation now correctly preserves the original file's permissions and ownership when creating the backup copy.
58-63: Excellent: File permissions preserved during restore.The restore operation now correctly preserves the backup file's permissions and ownership when restoring to the original location.
ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/internal/filePatchers/JarManifestModifier.scala (1)
21-21: LGTM: Proper import for the utility function.
| modifyFileWithMaintainingOriginalPermissionsAndOwner[File](file, identity) { f => modifyFile(f); f } | ||
| } | ||
|
|
||
| def modifyFileWithMaintainingOriginalPermissionsAndOwner[FILE](file: FILE, toJavaFile: FILE => File)(modifyJar: FILE => FILE): Unit = { |
There was a problem hiding this comment.
TBH, I'd be nice if we could get rid of java.io.File from our public methods. Better files lib is so nice, it'd be better to use it instead. Mixing these two worlds doesn't look nice.
So, let's modify the previous interface of the method and adapt other places where we are not so consistent with using better files.
| @@ -28,7 +29,8 @@ object JarManifestModifier { | |||
|
|
|||
| private val patchedByRorVersionPropertyName = "Patched-By-Ror-Version" | |||
|
|
|||
| def addPatchedByRorVersionProperty(file: File, rorVersion: String): Unit = { | |||
| def addPatchedByRorVersionProperty(file: File, | |||
| rorVersion: String): Unit = modifyFileWithMaintainingOriginalPermissionsAndOwner[File](file, _.toJava) { file => | |||
There was a problem hiding this comment.
Ah, it works here, but don't you think it's hard to reason about? Maybe the modifyFileWithMaintainingOriginalPermissionsAndOwner method's interface is not so good?
I mean, the method suggests we modify the jar file, but TBH, we don't have to modify it.
Do you have an idea how to do it better?
| os.copy(from = file, to = backupFolderPath / file.last, replaceExisting = true) | ||
| modifyFileWithMaintainingOriginalPermissionsAndOwner[Path](file, _.toIO) { originalFile => | ||
| val backedUpFile = backupFolderPath / originalFile.last | ||
| os.copy(from = originalFile, to = backedUpFile, replaceExisting = true, copyAttributes = true) |
There was a problem hiding this comment.
we have os.copy in copyToPluginPath too. But it's not used. Maybe we should remove it?
| @@ -46,11 +47,19 @@ private[patches] class RorPluginDirectory(val esDirectory: EsDirectory) { | |||
| } | |||
|
|
|||
| def backup(file: Path): Unit = { | |||
| os.copy(from = file, to = backupFolderPath / file.last, replaceExisting = true) | |||
| modifyFileWithMaintainingOriginalPermissionsAndOwner[Path](file, _.toIO) { originalFile => | |||
There was a problem hiding this comment.
here the usage of modifyFileWithMaintainingOriginalPermissionsAndOwner looks strange too. Becase we don't modify the file
🐞Fix (ES) Preserve file owner and permissions during ror-tools patch and unpatch operations