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

Enable Credentials Manager for tvOS and Mac Platforms #206

Merged
merged 4 commits into from
Jul 20, 2018

Conversation

cocojoe
Copy link
Member

@cocojoe cocojoe commented Jul 16, 2018

BioMetrics only available for iOS Platform Only
Remove restriction in CocoaPod Spec
Added tvOS, Mac Apps for Hosted Testing (Required for Keychain)

Enable BioMetrics for iOS Platform Only
Remove restriction in CocoaPod Spec
Added tvOS, Mac Apps for Hosted Testing (Required for Keychain)
@cocojoe cocojoe changed the title Enable Credentials Manager for tvOS,Mac,watchOS Platforms Enable Credentials Manager for tvOS and Mac Platforms Jul 16, 2018
Copy link
Contributor

@lbalmaceda lbalmaceda left a comment

Choose a reason for hiding this comment

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

What if instead of adding # checks everywhere in the existing code, you just do that once at the instantiation of the CredentialsManager and instead, create a TvCredentialsManager or similar which would override the original's methods. Those methods not being override can still call super, so no need to re-write everything and code would remain much more clearer IMO.

BioMetrics only available for iOS Platform Only

e.g. A no-op TvBiometrics that could still be called.

How have you tested this? Is there a GIF or video you can post?

@@ -16,11 +16,30 @@ web_auth_files = [
'Auth0/SilentSafariViewController.swift',
'Auth0/NativeAuth.swift',
'Auth0/AuthProvider.swift',
'Auth0/CredentialsManager.swift',
Copy link
Contributor

Choose a reason for hiding this comment

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

this are referenced in s.osx.exclude_files. Why did you remove them?

Copy link
Member Author

Choose a reason for hiding this comment

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

Enabling it for tvOS also allows it for Mac

'Auth0/BioAuthentication.swift'
]

watchos_exclude_files = [
Copy link
Contributor

Choose a reason for hiding this comment

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

this looks like web_auth_files plus 2 entries. Is there a nicer way to define this array? i.e. joining the already defined base array.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sadly not, i've tried joining arrays etc it will just throw out an error in cocoapods when linting the lib.

}

func applicationWillResignActive(_ application: UIApplication) {
// Sent when the application is about to move from active to inactive state. This can occur for certain types of temporary interruptions (such as an incoming phone call or SMS message) or when the user quits the application and it begins the transition to the background state.
Copy link
Contributor

Choose a reason for hiding this comment

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

(such as an incoming phone call or SMS message)

how can this happen on a TV?

Copy link
Member Author

Choose a reason for hiding this comment

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

shrug this is default boiler plate apple app creation.


import UIKit

class ViewController: UIViewController {
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the point on keeping this class if it only overrides 2 methods that call the super? as opposed to OAuth2Mac/ViewController.swift which does change one of the methods logic.

Copy link
Member Author

Choose a reason for hiding this comment

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

Again this is Apple's boilerplate application creation.

@cocojoe
Copy link
Member Author

cocojoe commented Jul 20, 2018

@lbalmaceda these are pre-processor macros, not run-time. There is no need to create a pointless no-op method, in the above the method will not be visible in any way when building a tvOS app. From the perspective of the developer they only see the methods available to that platform.

I've tested locally for tvOS, fastlane doesn't support tvOS so I can't easily add to the current CI setup.
The tenant who requested this feature has also tested and confirmed all working for their use case.

Copy link
Contributor

@lbalmaceda lbalmaceda left a comment

Choose a reason for hiding this comment

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

Then GLMT ⚔️

@cocojoe cocojoe merged commit 38c6bc3 into master Jul 20, 2018
@lbalmaceda lbalmaceda deleted the added-credentialsmanager-tvos-mac branch July 20, 2018 14:14
@cocojoe cocojoe added this to the vNext milestone Jul 26, 2018
@cocojoe cocojoe mentioned this pull request Jul 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants