Added obfuscation for keys #67

rmirabelli · 2019-07-23T20:33:05Z

Simple obfuscation, documentation included.

let key = ObfuscationKey().a.b.c.n1.n2.n3.value

CLAassistant · 2019-07-23T20:33:18Z

All committers have signed the CLA.

tylermilner

Looking good. Mostly minor suggestions. Also need to get a CHANGELOG entry added.

tylermilner · 2019-07-23T20:37:03Z

Sources/UtiliKit/Obfuscation/ObfuscatedKey.swift

+//  UtiliKit-iOS
+//
+//  Created by Russell Mirabelli on 7/23/19.
+//  Copyright © 2019 CocoaPods. All rights reserved.


Looks like we need to update the copyright here to "Bottle Rocket Studios". Could you go ahead and update this comment and make the change in the project file?

tylermilner · 2019-07-23T20:41:07Z

Sources/UtiliKit/Obfuscation/ObfuscatedKey.swift

+
+    var value: String {
+        get {
+            return _value


We can use the implicit getter here instead of manually specifying the get block.

var value: String { return _value }

tylermilner · 2019-07-23T20:47:52Z

Tests/ObfuscationTests.swift

+//
+
+import XCTest
+@testable import UtiliKit


I'm thinking a regular import might work just as well here since we're testing public APIs (also, we might need to make much ObfuscatedKey.swift public so that it's usable in client code).

tylermilner · 2019-07-23T20:49:00Z

Tests/ObfuscationTests.swift

+    func testSimpleObfuscatedString() {
+        let key = ObfuscatedKey().A.B.A.B.value
+        let expected = "ABAB"
+        XCTAssert(key == expected, "Keys do not match: \(key) is not \(expected)")


Very minor, but you might consider using XCTAssertEqual() instead of the more generic XCTAssert().

tylermilner · 2019-07-23T20:52:39Z

Tests/ObfuscationTests.swift

+    }
+
+    func testExtras() {
+        let key = ObfuscatedKey().dot.dash.underscore.anything("=").value


Very minor, but we might consider renaming anything(_:) to read a little better here at the call site. Some potential suggestions:

appending(_:)

explicit(_:)

extra(_:)

additional(_:)

What about literal(_:) ?

I kinda like additional. What about calling it add?
add(_:)

I used literal.

codecov-io · 2019-07-23T21:08:39Z

Codecov Report

Merging #67 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master    #67   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           4      5    +1     
  Lines         413    433   +20     
=====================================
+ Hits          413    433   +20

Impacted Files	Coverage Δ
Tests/ObfuscationTests.swift	`100% <100%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2a4f4dc...5d7aca1. Read the comment docs.

wmcginty

Looks good, a few minor changes. Seems like a very useful addition.

wmcginty · 2019-07-23T21:12:34Z

Sources/UtiliKit/Obfuscation/ObfuscatedKey.swift

+/// By using the ObfuscatedKey struct, you can build a human-readable key that nonetheless
+/// will not appear simply by running "strings" against your compiled code, and will even
+/// not appear as a string within your source code.
+struct ObfuscatedKey {


This will need to be public.

wmcginty · 2019-07-23T21:13:24Z

Sources/UtiliKit/Obfuscation/ObfuscatedKey.swift

+    }
+
+    // codebeat:disable[TOO_MANY_FUNCTIONS]
+    var A: ObfuscatedKey { return ObfuscatedKey(_value + "A") }


It might make sense to throw these into a public extension to alleviate the need to define each of these as public var ...

Nice way to avoid all the publics. Done, and thanks!

wmcginty · 2019-07-23T21:14:08Z

Tests/ObfuscationTests.swift

+    }
+
+    func testExtras() {
+        let key = ObfuscatedKey().dot.dash.underscore.anything("=").value


What about literal(_:) ?

wmcginty · 2019-07-23T21:16:03Z

Sources/UtiliKit/Obfuscation/ObfuscatedKey.swift

+/// will not appear simply by running "strings" against your compiled code, and will even
+/// not appear as a string within your source code.
+struct ObfuscatedKey {
+    private let _value: String


Can we lose the internal variable and use public private(set) value: String ?

That's not available on a "let"; I believe that the immutability of a "let" is important to make the general builder technique make sense.

Fair point - withdrawn.

I can't help but feel like I'm missing something here - what does the private let _value: String and public var value { return _value } get us here over a single immutable public let value. As far as I can tell, we aren't mutating it or validating internally and the same-file extensions should have the same access regardless.

Fair point. Since my merge is still blocked by the license (ugh), I'll think about this and see if there is a good reason I can find.

Reviewed, changed.

tylermilner · 2019-07-23T22:45:48Z

CHANGELOG.md

@@ -2,6 +2,10 @@

 ##### Enhancements

+* Added obfuscation for keys/passwords to ensure that they don't appear in plaintext.


Very minor, maybe just mention the "Obfuscation" subspec and the ObfuscatedKey class so that clients can get a general sense of how they can make use of this new feature when they see these release notes.

Added obfuscation for keys

877dda3

rmirabelli requested review from achappell, ganttastic, tylermilner, wmcginty and earlgaspard July 23, 2019 20:33

tylermilner suggested changes Jul 23, 2019

View reviewed changes

update codebeat warnings

8e6d2dc

wmcginty requested changes Jul 23, 2019

View reviewed changes

rmirabelli added 3 commits July 23, 2019 17:35

Made class public, used extension to open variables, fixed copyright

78cf028

added changelog

d5e06c3

used assertequal

24569f9

rmirabelli requested review from wmcginty and tylermilner July 23, 2019 22:40

wmcginty previously approved these changes Jul 23, 2019

View reviewed changes

tylermilner previously approved these changes Jul 23, 2019

View reviewed changes

earlgaspard previously approved these changes Jul 23, 2019

View reviewed changes

updated changelog to better describe what ObfuscatedKey does

5d7aca1

rmirabelli dismissed stale reviews from earlgaspard, tylermilner, and wmcginty via 5d7aca1 July 23, 2019 22:55

change to public variable, no value in private let

e3c366c

rmirabelli requested review from wmcginty, earlgaspard and tylermilner July 24, 2019 15:46

wmcginty approved these changes Jul 24, 2019

View reviewed changes

tylermilner approved these changes Jul 24, 2019

View reviewed changes

rmirabelli merged commit 264d7eb into BottleRocketStudios:master Jul 24, 2019

rmirabelli deleted the feature/obfuscation branch July 24, 2019 17:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added obfuscation for keys #67

Added obfuscation for keys #67

rmirabelli commented Jul 23, 2019

CLAassistant commented Jul 23, 2019 •

edited

tylermilner left a comment

tylermilner Jul 23, 2019

rmirabelli Jul 23, 2019

tylermilner Jul 23, 2019

rmirabelli Jul 23, 2019

tylermilner Jul 23, 2019

rmirabelli Jul 23, 2019

tylermilner Jul 23, 2019

rmirabelli Jul 23, 2019

tylermilner Jul 23, 2019 •

edited

wmcginty Jul 23, 2019

earlgaspard Jul 23, 2019

rmirabelli Jul 23, 2019

codecov-io commented Jul 23, 2019 •

edited

wmcginty left a comment

wmcginty Jul 23, 2019

rmirabelli Jul 23, 2019

wmcginty Jul 23, 2019

rmirabelli Jul 23, 2019

wmcginty Jul 23, 2019

wmcginty Jul 23, 2019

rmirabelli Jul 23, 2019

wmcginty Jul 23, 2019

wmcginty Jul 23, 2019

rmirabelli Jul 24, 2019

rmirabelli Jul 24, 2019

tylermilner Jul 23, 2019

rmirabelli Jul 23, 2019

		@@ -2,6 +2,10 @@

		##### Enhancements

		* Added obfuscation for keys/passwords to ensure that they don't appear in plaintext.

Added obfuscation for keys #67

Added obfuscation for keys #67

Conversation

rmirabelli commented Jul 23, 2019

CLAassistant commented Jul 23, 2019 • edited

tylermilner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tylermilner Jul 23, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jul 23, 2019 • edited

Codecov Report

wmcginty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CLAassistant commented Jul 23, 2019 •

edited

tylermilner Jul 23, 2019 •

edited

codecov-io commented Jul 23, 2019 •

edited