Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AeroGear-2093 Update the Security Template app to use the IDM SDK #18

Merged
merged 6 commits into from
Apr 18, 2018
Merged

AeroGear-2093 Update the Security Template app to use the IDM SDK #18

merged 6 commits into from
Apr 18, 2018

Conversation

twnolan
Copy link
Contributor

@twnolan twnolan commented Mar 29, 2018

Motivation:
To upgrade the ios security template application to use the Aerogear AGSAuth library.

To Test:
Build and run the application in an iOS Simulator

Work Completed:

  • Replaced authentication service built with APPAuth with AGSAuth
  • Removed Identity model and replaced it with AGSAuth User model
  • Created new file to house RealRole model
  • Updated configuration and add mobile-services.json
  • Add AsciiDoc Tags
  • Remove commented out lines of code
  • Check Tests for failures
  • Fix failing tests
  • Update podfile and cocoapods

… and replaced it with AGSAuth User model, added mobile-services file
@@ -26,9 +28,18 @@ class AppComponents {
self.kcWrapper = KeychainWrapper.standard
}

func resolveAuthService() -> AuthenticationService {

func resolveAuthService() -> AgsAuth {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for docs referencing, can you add asciidoc tags around this function? eg

// tag::initAuthService[]
func resolveAuthService() -> AgsAuth {
 ...
}
// end::initAuthService[]

do {
try self.authService.login(presentingViewController: presentingViewController, onCompleted: onLoginCompleted)
} catch {
fatalError("Unexpected error: \(error)")
}
}

func logout() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also here:

// tag::logout[]
func logout() {
...
}
// end::logout[]

let certPinningService: CertPinningService
var router: AuthenticationRouter?

init(authService: AuthenticationService, certPinningService: CertPinningService) {
init(authService: AgsAuth, certPinningService: CertPinningService) {
self.authService = authService
self.certPinningService = certPinningService
}

func startAuth(presentingViewController: UIViewController) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also here:

// tag::login[]
func startAuth(presentingViewController: UIViewController) {
...
}
// end::login[]

@twnolan
Copy link
Contributor Author

twnolan commented Mar 29, 2018

@TommyJ1994 Tags have been added as requested


let accessControlRouter = AccessControlRouterImpl(viewController: viewController)
return accessControlRouter
}

func resolveCurrentUser() -> User? {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@twnolan Can you also add a docs tag to this function. resolveCurrentUser should be ok for naming.

@@ -49,9 +50,9 @@ class AccessControlViewController: UIViewController, UITableViewDataSource, UITa
// Dispose of any resources that can be recreated.
}

func highlightUserRealmRoles(identity: Identity) {
func highlightUserRealmRoles(user: User) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@twnolan Also possible to have asciidoc tags for this one too? highlightUserRealmRoles

"url": "https://www.mocky.io/v2/5a6b59fb31000088191b8ac6",
"config": {
"auth-server-url": "https://keycloak.security.feedhenry.org/auth",
"clientId": "client-app",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@twnolan We can remove clientId from the config block as it's no longer included in the configuration since aerogearcatalog/keycloak-apb#66.

@twnolan
Copy link
Contributor Author

twnolan commented Apr 10, 2018

@TommyJ1994 @aidenkeating The code has been updated as per your requests

Copy link
Contributor

@tomjackman tomjackman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small comment. Looks good to me otherwise, tested on emulator and all auth/access control functions work as expected.

@@ -0,0 +1,35 @@
{
"version": "1.0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the latest aerogear-ios-sdk expects this to be a integer now rather than a String. The app crashed for me using the latest SDK with the version defined as a String.

@twnolan
Copy link
Contributor Author

twnolan commented Apr 11, 2018

@TommyJ1994 updated as per your recommendation

@twnolan twnolan changed the title [WIP - Do not Merge] AeroGear-2093 Update the Security Template app to use the IDM SDK AeroGear-2093 Update the Security Template app to use the IDM SDK Apr 18, 2018
@twnolan
Copy link
Contributor Author

twnolan commented Apr 18, 2018

@wei-lee @aidenkeating @TommyJ1994 This PR is fully ready now for final review and testing

return nil
}
return currentUser
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can just do return authService.currentUser()

return nil
}
return currentUser
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the guard statement is not required here.

Copy link
Contributor

@wei-lee wei-lee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good. 2 small things:

  1. can we remove the .DS_Store file from the repo
  2. the guard request in resolveCurrentUser method is not required.

Once these 2 things are fixed, we are good to merge.

Good job!

@twnolan
Copy link
Contributor Author

twnolan commented Apr 18, 2018

@wei-lee updated as requested

@twnolan twnolan merged commit 03e7b16 into feedhenry:master Apr 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants