search

AzureAD / azure-activedirectory-library-for-objc

The ADAL SDK for Objective C gives you the ability to add support for Work Accounts to your iOS and macOS applications with just a few lines of additional code. This SDK gives your application the full functionality of Microsoft Azure AD, including industry standard protocol support for OAuth2, Web API integration with user level consent, and two factor authentication support.
MIT License
178 stars 113 forks source link

ADWebAuthController must not overwrite new #1030

Open efalkenberg opened 7 years ago

efalkenberg commented 7 years ago

ADWebAuthController overwrites the new method that is defined in NSObject in https://github.com/AzureAD/azure-activedirectory-library-for-objc/blob/dev/ADAL/src/ui/ADWebAuthController.m#L87-L90

NSObject's new method is defined as Allocates a new instance of the receiving class, sends it an init message, and returns the initialized object. This method is a combination of alloc and init. Like alloc, it initializes the isa instance variable of the new object so it points to the class data structure. It then invokes the init method to complete the initialization process.

The overwritten method in ADWebAuthController does not send the init method but returns only the allocated memory of an object that is not fully initialized as it used to be. This changes the default behavior of the Objective-C object model and could lead into memory leaks or crashes. It also does not provide any value since calling [ADWebAuthController new] ends up being the same as [ADWebAuthController new] where [ADWebAuthController new] is supposed to be a shortcut for the default initializer [[ADWebAuthController alloc] init]

macblazer commented 7 years ago

This is part of the ADAL framework's attempt to enforce private classes and singleton objects. The implementation of [ADWebAuthController new] calls [ADWebAuthController alloc] which has a hard-coded failing NSAssert, @throws an exception, and returns nil.

There are many other instances of the same sort of thing throughout the codebase. For example, ADAuthenticationError cannot be easily instantiated outside of the framework code because the -init and -initWithDomain:code:userInfo: methods both call through to -doesNotRecognizeSelector:.

I do not know why the decision was made to go this route. It makes unit testing code that accesses the ADAL very difficult because most of the ADAL cannot be easily mocked out. For example, testing my code for the proper handling of ADAuthenticationError is difficult because I can't create an ADAuthenticationError object to test with.