Skip to content
This repository has been archived by the owner on Jan 5, 2024. It is now read-only.

Added a decryption closure and value #62

Merged
merged 8 commits into from
Nov 30, 2021

Conversation

fadyyecob
Copy link

@fadyyecob fadyyecob commented Nov 26, 2021

Description

  • Added a decryptionClosure variable to the TweakProvider protocol
  • Added a decryptedValue let to Tweak
  • Added a mutatedCopy function to Tweak to be able to duplicate it and set the decrypted value

Motivation and Context

Copied from the design doc:

A closure property decryptionClosure is added to TweakProvider with the signature

(Tweak) -> TweakValue

A new property decryptedValue is added to Tweak

public var decryptedValue: TweakValue

This will either return the decrypted TweakValue (passed through decryptionClosure) or just
the existing value if no decryptionClosure is specified.

Usage would then look like this

func testSecureTweakValueIsDescryptedToCorrectValue() {
    let tweak = Tweak(feature: "some_encrypted_api_keys",
                      variable: "some_super_secret_encrypted_key",
                      value: "54321",
                      decryptionClosure: { String($0.stringValue!.reversed()) }
    )
    XCTAssertNotEqual("12345", tweak.value.stringValue)
    XCTAssertEqual("12345", tweak.decryptedValue.stringValue)
}

How Has This Been Tested?

Added a unit test that sets the tweak value to a base64 encoded string. And set a decryptionClosure that decodes the base64 encoded string, which is then checked

Types of changes

  • New feature (non-breaking change which adds functionality)

Checklist:

  • My code follows the code style of this project.

@fadyyecob fadyyecob changed the title Added decryption closure Draft: Added decryption closure Nov 26, 2021
@fadyyecob fadyyecob changed the title Draft: Added decryption closure Added a decryption closure and value Nov 29, 2021
@albertodebortoli
Copy link
Member

The PR description mentions

New feature (non-breaking change which adds functionality)

but the TweakProvider protocol has been modified and since it's public, the addition of a new property technically makes the new version of JustLog not backwards compatible and clients will have to update any implementation of custom TweakProviders. Not sure we can/want to try to avoid this with a protocol extension.

Different story for the Tweak's init as the new parameter has a default value.

I also noticed that the demo app has 2 tweak providers that have been removed from the target (and therefore are not compiled) and still inherit from Configuration (the old name of TweakProvider). Not a problem to be addressed, just a consideration.

@fadyyecob
Copy link
Author

@albertodebortoli @dchakarov I've updated it with Alberto's feedback. If all is good after Dimi's review, you can merge it (I don't think I have the permission myself)

@fadyyecob
Copy link
Author

fadyyecob commented Nov 29, 2021

@albertodebortoli @dchakarov Added a few more tests. Just to make sure the implementations of TweakProvider works as expected

@albertodebortoli albertodebortoli merged commit 6a0fd22 into justeat:master Nov 30, 2021
@fadyyecob fadyyecob deleted the feature/decrypt-closure branch November 30, 2021 12:29
@albertodebortoli albertodebortoli added this to the V8 milestone Nov 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants