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

Add an option for disabling encryption for a cache file name #1140

Merged
merged 3 commits into from
Mar 22, 2019

Conversation

hasankose
Copy link
Contributor

This option allows disabling md5 encryption for creation cache file name in DiskStorage.

@onevcat
Copy link
Owner

onevcat commented Mar 22, 2019

@hasan-monument Thanks for it.

The change looks good, but may I know what's the use case of it? The cache folder is visible to your users and storing the files with hashed names would protect privacy, that is especially important when you are trying save files with some personal information (like ID or names) in the URLs.

@hasankose
Copy link
Contributor Author

I was storing images like id.jpg by overriding this func cachePath(forComputedKey key: String) cause of some decision in our team. Now it is deprecated. If I use cache filename with md5 encryption, I need to re-download again with the new update.

My suggestion is you should give an option to developers for that. Kingfisher also has the resource ImageResource(downloadURL: URL, cacheKey: key), If developers want to use md5 encryption, they can pass the encrypted key with cacheKey or they can use different type encryption than md5.

This is just my suggestion, I respect your decision. Thanks for the review.

@@ -310,6 +317,9 @@ extension DiskStorage {
/// Default is `nil`, means that the cache file does not contain a file extension.
public var pathExtension: String? = nil

/// Default is `true`, means that md5 encryption will be used for creation the cache file name.
public var useEncryptionForCacheFileName: Bool = true
Copy link
Owner

@onevcat onevcat Mar 22, 2019

Choose a reason for hiding this comment

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

Suggested change
public var useEncryptionForCacheFileName: Bool = true
public var usesHashedFileName = true

@@ -310,6 +317,9 @@ extension DiskStorage {
/// Default is `nil`, means that the cache file does not contain a file extension.
public var pathExtension: String? = nil

/// Default is `true`, means that md5 encryption will be used for creation the cache file name.
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
/// Default is `true`, means that md5 encryption will be used for creation the cache file name.
/// Default is `true`, means that the cache file name will be hashed before storing.

let hashedKey = key.kf.md5
if let ext = config.pathExtension {
return "\(hashedKey).\(ext)"
if self.config.useEncryptionForCacheFileName {
Copy link
Owner

@onevcat onevcat Mar 22, 2019

Choose a reason for hiding this comment

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

Suggested change
if self.config.useEncryptionForCacheFileName {
if self.config.usesHashedFileName {

@onevcat
Copy link
Owner

onevcat commented Mar 22, 2019

@hasan-monument Thanks for the explaination. I understood your case and I agree that we can provide an option for it.

May I ask you to change the name to usesHashedName, since MD5 is not an encryption but just a digest. It will be more accurate to say "hash".

And it would be great if you can also add some tests for this property too. After that, we can merge and release a new version for it.

Thank you!

Add test case for config which is usesHashedFileName
@hasankose
Copy link
Contributor Author

I didn't add a test case for validation md5 string, because I didn't want to import the library CommonCrypto to the file. If the test cases aren't enough, I can add it as well.

@onevcat
Copy link
Owner

onevcat commented Mar 22, 2019

Let’s add it. May you can use kf.md5 from this https://github.com/onevcat/Kingfisher/blob/master/Sources/Utility/String%2BMD5.swift to avoid introducing the library importing.

@hasankose
Copy link
Contributor Author

Thanks for the hint. Added.

@onevcat onevcat merged commit afbee5e into onevcat:master Mar 22, 2019
skoduricg pushed a commit to rentpath/Kingfisher that referenced this pull request Sep 24, 2021
Add an option for disabling encryption for a cache file name
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.

2 participants