Conversation
In order for this pull request to be merged, make sure you test whether your changes work.If the changes are working as intended, remove the
Untested
label from the pull request. Contributors, please review this pull request! |
marchermans
left a comment
marchermans
left a comment
There was a problem hiding this comment.
This is a very bad idea.
You store the secret key in a publicly available source.
It should suffice to cache the key for the session and nothing more, you don't need to write that to disk.
|
I actually made a similar class in another PR I was working on which I think gets the same idea across in a nicer solution: https://gist.github.com/Thodor12/e66f68dbb22f1487c9d6c32e44c9d9d5 It uses a LoadingCache which is a persistant map which keeps the feature unlocks available for an hour before recycling the information out of the map |
| String os = System.getProperty("os.name").toLowerCase(); | ||
| String home = System.getProperty("user.home"); | ||
|
|
||
| if (os.contains("win")) |
There was a problem hiding this comment.
This feels like a lot of extra conditions that aren't necessary. Just dump it in the user.home folder always, who cares where the hell we place this file?
Users don't really need to touch this file so it doesn't matter at all whether it lives in appdata or wherever
| return Paths.get(home, ".config", "minecolonies"); | ||
| } | ||
|
|
||
| private static String base64(byte[] data) |
There was a problem hiding this comment.
Can't we inline the few usages of these methods, it's not enough to justify an entirely new method in my eyes
| /** | ||
| * Delete the token | ||
| */ | ||
| public static void delete() |
| { | ||
| e.printStackTrace(); | ||
| // Checks the token file when the api has issues | ||
| isFeatureUnlocked.set(UnlockToken.isFeatureEnabledFor(playerUuid, SKIN)); |
There was a problem hiding this comment.
Wait, this isn't even caching, caching implies you read the cache before you make the external call, which is not even what happens. This needs to be actual caching by not reading the API, or change the wording of the description of the PR that this is a local fallback.
| /** | ||
| * Unlockable features | ||
| */ | ||
| public static final String SKIN = "skins"; |
There was a problem hiding this comment.
Something that hasn't been considered at all, even though it's not an issue per sé right now. If we have multiple feature types, there can only be 1 file, and this data isn't aggregated into the file.
So the implementation with being able to define multiple feature types right now is practically useless, as the files will continually overwrite one another and invalidate one another. (Because the signature contains the unlock type which was requested, when you save the file this signature (thus unlock type) is stored in the file, if another feature type comes along and wants to save the file, it's going to overwrite the local cache file with a different signature referring to another feature type)
| } | ||
|
|
||
| // Checks the token file incase the api returned false, to buffer auth issues until the token runs out | ||
| isFeatureUnlocked.set(isFeatureUnlocked.get() || UnlockToken.isFeatureEnabledFor(playerUuid, SKIN)); |
There was a problem hiding this comment.
Shouldn't we treat the API as a higher priority no matter what? If we get a valid response from the API we should treat that as the truth, and not allow fallbacks to be used in that case.
4 participants
Closes #
Closes #
Changes proposed in this pull request
Review please(in discord)