AEROGEAR-2092 Add OIDCCredentials implementation #35

aidenkeating · 2018-03-05T16:06:18Z

Motivation

We need a way to store and manage credentials.

JIRA: AEROGEAR-2092

Description

Create an OIDCCredentials object which is backed by an OIDAuthState.

Create a CredentialManager which takes the OIDAuthState from the OIDCCredentials and stores it using a Keychain storage library.

Progress

OIDCCredentials
CredentialManager
OIDCCredentialsTest
CredentialManagerTest
Make OIDCCredentials NSCoding compliant (so the CredentialManager doesn't need to know about authState)

wtrocki · 2018-03-05T16:10:28Z

modules/auth/credentials/OIDCCredentials.swift

@@ -5,14 +5,47 @@
 import AppAuth
 import Foundation

-class OIDCCredentials {
+/// A class to represent the OpenID Connect state for an entity.


let's use /** format everywhere. (Rest of the changes use /** */)

wtrocki · 2018-03-05T16:11:03Z

modules/auth/credentials/OIDCCredentials.swift

-    init(state: OIDAuthState) {
-        authState = state
+    public init(state: OIDAuthState) {
+        self.authState = state


That will break lint. Default rules are preventing from using self.

wtrocki · 2018-03-05T16:14:15Z

modules/auth/agsauth.podspec

@@ -18,5 +18,6 @@ Pod::Spec.new do |s|
  s.dependency 'AGSCore'
  ## Add other dependencies if needed
  s.dependency 'AppAuth'
+  s.dependency 'SwiftKeychainWrapper'


We probably need to lock version as this third party library can be possible security threat if we will always get latest.

[Optional] Actually library seems to be just changing the api, but delivering no extra capabilities. It may make sense to just use plain apple api to simplify IOS versions support etc.

I'm not fully aware of the scope of this change so leaving decision to discussion.

@wtrocki The library makes it a lot simpler to set/read/delete data in the Keychain. The Keychain API isn't the easiest one to use in the world. What is the downside of using the lib?

@wei-lee Not a problem. Just better to specify the version range for dependency.

@wtrocki agreed.

wtrocki · 2018-03-05T16:15:35Z

example/AeroGearSdkExampleTests/auth/credentials/OIDCCredentialsTest.swift

+    }
+
+    func testGetAccessToken() {
+        XCTAssert(testCredentials?.getAccessToken() == OIDCCredentialsTest.paramAccessTokenVal)


[Optional] XCTAssertEqual may look better

wtrocki · 2018-03-05T16:18:24Z

@aidenkeating Awesome changes! You need to run ./scripts/develop.sh to fix some formatting/style.

wei-lee · 2018-03-05T16:23:04Z

modules/auth/credentials/CredentialsManager.swift

-    func load() -> OIDCCredentials? {
-        return nil
+public class CredentialsManager: CredentialManagerProtocol {
+    let AUTH_STATE_KEY = "authState"


use reverse domain style key to ensure uniqueness of the key

wei-lee · 2018-03-05T16:43:42Z

modules/auth/credentials/OIDCCredentials.swift

+    ///
+    /// - Returns: true if the credential is authorized.
+    public func isAuthorized() -> Bool {
+        return authState.isAuthorized
    }


So in swift, when you need to get a value from a class, if there is no parameter required to get the value, you normally have 2 options:

Use computed properties, or

Use a function that takes no parameters and return a value

Functionality-wise, they are the same, but there is subtle differences semantically.

Using computed properties means the value is a property of the class, while using functions means the value is an method/action of the class.

For this class, I think most of the values returned by the getters are actually properties (they are properties on the AuthState class as well). So from that point of view, it's better to use computed properties instead. It is also more concise to write.

WDYT?

Yes this makes a lot of sense. Good point.

wtrocki · 2018-03-06T08:10:45Z

modules/auth/credentials/CredentialsManager.swift


-protocol CredentialManagerProtocol {
+public protocol CredentialManagerProtocol {
    func load() -> OIDCCredentials?


[Optional] Comments are usually more important on the protocol.
Could we move comments from implementation to protocol

Good catch, moved

…edentialManager

aidenkeating · 2018-03-06T09:20:52Z

@wtrocki @wei-lee Mind taking another look, just ensured the OIDCCredentials itself is encodable instead of having the CredentialManager be aware of stuff it shouldn't be.

wei-lee

LGTM

AEROGEAR-2092 Add OIDCCredentials implementation

f199564

aidenkeating requested review from danielpassos and wtrocki as code owners March 5, 2018 16:06

wtrocki reviewed Mar 5, 2018

View reviewed changes

wei-lee reviewed Mar 5, 2018

View reviewed changes

aidenkeating force-pushed the AEROGEAR-2092-oidccredentials branch from 9396f40 to ca222e8 Compare March 5, 2018 16:44

Aiden Keating added 2 commits March 5, 2018 17:14

AEROGEAR-2092 swiftlint autocorrect

b9caade

AEROGEAR-2092 Test updates

62f2ad5

aidenkeating force-pushed the AEROGEAR-2092-oidccredentials branch from ca222e8 to 62f2ad5 Compare March 5, 2018 17:14

AEROGEAR-2092 Update comment style

cb437e0

danielpassos approved these changes Mar 5, 2018

View reviewed changes

Aiden Keating added 3 commits March 5, 2018 22:02

AEROGEAR-2092 Use computed properties

9915763

AEROGEAR-2092 Version dependencies for auth module

e2f9d9c

AEROGEAR-2092 Update keychain storage key

5239ca4

wtrocki approved these changes Mar 6, 2018

View reviewed changes

wtrocki reviewed Mar 6, 2018

View reviewed changes

Aiden Keating added 2 commits March 6, 2018 08:48

AEROGEAR-2092 Update documentation for credential manager protocol

c4f42d7

AEROGEAR-2092 Make OIDCCredentials NSCoding compliant for use with Cr…

1e29c6b

…edentialManager

wei-lee approved these changes Mar 6, 2018

View reviewed changes

aidenkeating merged commit 7013dd5 into aerogear:master Mar 6, 2018

aidenkeating deleted the AEROGEAR-2092-oidccredentials branch March 6, 2018 09:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AEROGEAR-2092 Add OIDCCredentials implementation #35

AEROGEAR-2092 Add OIDCCredentials implementation #35

aidenkeating commented Mar 5, 2018 •

edited

wtrocki Mar 5, 2018 •

edited

wtrocki Mar 5, 2018

wtrocki Mar 5, 2018

wtrocki Mar 5, 2018 •

edited

wei-lee Mar 5, 2018 •

edited

wtrocki Mar 5, 2018 •

edited

wei-lee Mar 5, 2018

wtrocki Mar 5, 2018 •

edited

wtrocki commented Mar 5, 2018

wei-lee Mar 5, 2018

wei-lee Mar 5, 2018

aidenkeating Mar 5, 2018

wtrocki Mar 6, 2018 •

edited

aidenkeating Mar 6, 2018

aidenkeating commented Mar 6, 2018

wei-lee left a comment

AEROGEAR-2092 Add OIDCCredentials implementation #35

AEROGEAR-2092 Add OIDCCredentials implementation #35

Conversation

aidenkeating commented Mar 5, 2018 • edited

Motivation

Description

Progress

wtrocki Mar 5, 2018 • edited

Choose a reason for hiding this comment

wtrocki Mar 5, 2018

Choose a reason for hiding this comment

wtrocki Mar 5, 2018

Choose a reason for hiding this comment

wtrocki Mar 5, 2018 • edited

Choose a reason for hiding this comment

wei-lee Mar 5, 2018 • edited

Choose a reason for hiding this comment

wtrocki Mar 5, 2018 • edited

Choose a reason for hiding this comment

wei-lee Mar 5, 2018

Choose a reason for hiding this comment

wtrocki Mar 5, 2018 • edited

Choose a reason for hiding this comment

wtrocki commented Mar 5, 2018

wei-lee Mar 5, 2018

Choose a reason for hiding this comment

wei-lee Mar 5, 2018

Choose a reason for hiding this comment

aidenkeating Mar 5, 2018

Choose a reason for hiding this comment

wtrocki Mar 6, 2018 • edited

Choose a reason for hiding this comment

aidenkeating Mar 6, 2018

Choose a reason for hiding this comment

aidenkeating commented Mar 6, 2018

wei-lee left a comment

Choose a reason for hiding this comment

aidenkeating commented Mar 5, 2018 •

edited

wtrocki Mar 5, 2018 •

edited

wtrocki Mar 5, 2018 •

edited

wei-lee Mar 5, 2018 •

edited

wtrocki Mar 5, 2018 •

edited

wtrocki Mar 5, 2018 •

edited

wtrocki Mar 6, 2018 •

edited