Use CodeVerifier in OAuthTokenRequestBody for better encapsulation
#745
Conversation
| let codeVerifier: String | ||
| let rawCodeVerifier: String |
There was a problem hiding this comment.
I considered keeping this called codeVerifier, but though it be clearer to have "raw" in the name when the type is String.
I also considered adding a computed codeVerifier: PKCE.CodeVerifier property. I decided not to because the init?(value:) method can fail and it was either force unwrap under the assumption that the Google backend will never send back an invalid one (because it wouldn't allow us to send it an invalid one) or return an Optional. Moreover, there is no existing reader of that value outside of the tests... YAGNI.
There was a problem hiding this comment.
If the type were instead ProofKeyForCodeExchange.CodeVerifier, could you keep codeVerifier and just access the rawValue exclusively in asURLEncodedData?
Not a huge deal – mostly just a nit 🙂
There was a problem hiding this comment.
Your comment made me realize I was approaching the type in the context of Decodable (where it's handy to keep String properties to leverage the compiler-generated decoding) but this type is actually used for encoding data. 😅
See 03f377c.
| @@ -587,6 +589,8 @@ | |||
| 3FE8072329365FC20088420C /* GoogleSignIn */ = { | |||
| isa = PBXGroup; | |||
| children = ( | |||
| 3F30A6B9299F12E30004452F /* Character+URLSafeTests.swift */, | |||
There was a problem hiding this comment.
This addition is because I sorted the group alphabetically. See the matching removal below.
mokagio
force-pushed
the
mokagio/opaque-codeverifier-string
branch
from
353e4b1 to
de65b51
Compare
| let codeVerifier: String | ||
| let rawCodeVerifier: String |
There was a problem hiding this comment.
If the type were instead ProofKeyForCodeExchange.CodeVerifier, could you keep codeVerifier and just access the rawValue exclusively in asURLEncodedData?
Not a huge deal – mostly just a nit 🙂
| /// The reason we care about it being deterministic is because we don't want implicit randomness test. | ||
| /// The only place were we want to use random values in the `CodeVerifier` tests which explicitly check the random generation. | ||
| static func fixture() -> Self { | ||
| .init(value: (0..<minimumLength).map { _ in "a" }.joined())! |
There was a problem hiding this comment.
Another nit, but if we want to avoid ! here (in case SwiftLint or something wants to yell at us for it) we could add a private initializer like init(knownGoodValue: that uses a precondition or similar to crash if this API is misused.
Not sure if we care, but it's an option 🤷
There was a problem hiding this comment.
I'd like to park this suggestion for the moment. We don't have a rule to flag force unwraps in the test code ATM and there are arguments in favor of leaving this as an option, trading off the possibility of test-runtime crashes in case of developer error for more compact code.
This is instead of `String`. See conversation at https://github.com/wordpress-mobile/WordPressAuthenticator-iOS/pull/745/files#r1116507824 It also comes at the cost of losing `Encodable` conformance, because `CodeVerifier` does not conform to `Encodable` and, given the capability is not used anywhere, I decided not to write any extra code to satisfy the compiler.
mokagio
force-pushed
the
mokagio/opaque-codeverifier-string
branch
from
03f377c to
2989720
Compare
See discussion at #740 (comment)
CHANGELOG.mdif necessary.