Broker

<a href='#crh-start'></a><a href='#crh-data-%7B%22processed%22%3A%20%5B%22https%3A//github.com/AzureAD/azure-activedirectory-library-for-objc/pull/232%23discussion_r21695061%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-library-for-objc/pull/232%23discussion_r21695485%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-library-for-objc/pull/232%23discussion_r21695588%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-library-for-objc/pull/232%23discussion_r21695766%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-library-for-objc/pull/232%23discussion_r21696255%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-library-for-objc/pull/232%23discussion_r21698483%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-library-for-objc/pull/232%23discussion_r21712746%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-library-for-objc/pull/232%23discussion_r21712853%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-library-for-objc/pull/232%23discussion_r21712873%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-library-for-objc/pull/232%23discussion_r21712925%22%5D%2C%20%22comments%22%3A%20%7B%22Pull%20b1af80b6a90386ea9a85f16634df8220643b54fe%20ADALiOS/ADALiOSTests/ADAuthenticationContextTests.m%2061%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/AzureAD/azure-activedirectory-library-for-objc/pull/232%23discussion_r21698483%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22When%20you%20call%20dispatch_release%20for%20this%20semaphore%2C%20signal%20and%20wait%20needs%20to%20be%20balanced.%20Reusing%20in%20multiple%20calls%20can%20give%20you%20weird%20error%2C%20but%20each%20call%20looks%20like%20in%20sync.%20I%20guess%20putitng%20the%20dispatch_release%20to%20the%20end%20will%20be%20ok.%20%20%22%2C%20%22created_at%22%3A%20%222014-12-11T18%3A53%3A03Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/466805%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/omercs%22%7D%7D%2C%20%7B%22body%22%3A%20%22you%20do%20not%20need%20to%20call%20dispatch_release%20as%20per%20http%3A//stackoverflow.com/questions/8618632/does-arc-support-dispatch-queues%22%2C%20%22created_at%22%3A%20%222014-12-11T21%3A56%3A48Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/5713915%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/kpanwar%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20ADALiOS/ADALiOSTests/ADAuthenticationContextTests.m%3AL1146-1161%22%7D%2C%20%22Pull%20b1af80b6a90386ea9a85f16634df8220643b54fe%20ADALiOS/ADALiOS/ADBrokerKeyHelper.h%2015%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/AzureAD/azure-activedirectory-library-for-objc/pull/232%23discussion_r21695588%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22Is%20this%20key%20created%20from%20app%3F%22%2C%20%22created_at%22%3A%20%222014-12-11T18%3A16%3A16Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/466805%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/omercs%22%7D%7D%2C%20%7B%22body%22%3A%20%22Yes.%20ADAL%20creates%20the%20key%20and%20stores%20it%20in%20keychain.%20it%20is%20sent%20to%20the%20broker%20and%20and%20it%20uses%20the%20key%20to%20encrypt%20the%20payload.%22%2C%20%22created_at%22%3A%20%222014-12-11T21%3A55%3A41Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/5713915%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/kpanwar%22%7D%7D%2C%20%7B%22body%22%3A%20%22%3A%2B1%3A%22%2C%20%22created_at%22%3A%20%222014-12-11T21%3A56%3A02Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/5713915%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/kpanwar%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20ADALiOS/ADALiOS/ADBrokerKeyHelper.h%3AL37-44%22%7D%2C%20%22Pull%20b1af80b6a90386ea9a85f16634df8220643b54fe%20ADALiOS/ADALiOS/ADAuthenticationContext.m%2051%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/AzureAD/azure-activedirectory-library-for-objc/pull/232%23discussion_r21695061%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22No%20action%20but%20to%20think%20about%3A%20This%20is%20a%20digest%20hash%20check.%20Caller%20could%20replace%20hash%20and%20modify%20message.%20Adal%20will%20say%20hashes%20are%20ok%20and%20move%20to%20decrypt%20step.%20Caller%20can%20make%20up%20multiple%20fake%20calls%20to%20get%20more%20infor%20about%20app%27s%20key.%20hmac%20gives%20more%20security%20since%20it%20will%20fail%20to%20validate%20if%20message%20changed.%22%2C%20%22created_at%22%3A%20%222014-12-11T18%3A07%3A59Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/466805%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/omercs%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20ADALiOS/ADALiOS/ADAuthenticationContext.m%3AL192-245%22%7D%2C%20%22Pull%20b1af80b6a90386ea9a85f16634df8220643b54fe%20ADALiOS/ADALiOSTests/XCTestCase%252BTestHelperMethods.m%20188%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/AzureAD/azure-activedirectory-library-for-objc/pull/232%23discussion_r21696255%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22I%20think%20you%20need%20%5Cr%5Cndispatch_release%28sem%29%5Cr%5Cnafter%20wait%20is%20done.%22%2C%20%22created_at%22%3A%20%222014-12-11T18%3A24%3A59Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/466805%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/omercs%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20ADALiOS/ADALiOSTests/XCTestCase%2BTestHelperMethods.m%3AL532-551%22%7D%2C%20%22Pull%20b1af80b6a90386ea9a85f16634df8220643b54fe%20ADALiOS/ADALiOS/ADBrokerKeyHelper.m%2082%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/AzureAD/azure-activedirectory-library-for-objc/pull/232%23discussion_r21695766%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22i%20suggest%20to%20check%20if%20memory%20allocated%20or%20not%3F%22%2C%20%22created_at%22%3A%20%222014-12-11T18%3A18%3A54Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/466805%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/omercs%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20ADALiOS/ADALiOS/ADBrokerKeyHelper.m%3AL161-204%22%7D%2C%20%22Pull%20b1af80b6a90386ea9a85f16634df8220643b54fe%20ADALiOS/ADALiOS/ADAuthenticationResult%252BInternal.m%2024%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/AzureAD/azure-activedirectory-library-for-objc/pull/232%23discussion_r21695485%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22wrong%20key%22%2C%20%22created_at%22%3A%20%222014-12-11T18%3A14%3A56Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/466805%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/omercs%22%7D%7D%2C%20%7B%22body%22%3A%20%22%3A%2B1%3A%22%2C%20%22created_at%22%3A%20%222014-12-11T21%3A54%3A12Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/5713915%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/kpanwar%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20ADALiOS/ADALiOS/ADAuthenticationResult%2BInternal.m%3AL102-142%22%7D%7D%7D'></a>
<a href='https://www.codereviewhub.com/'><img src='http://www.codereviewhub.com/site/github-bar.png' height=40></a>
- [ ] <a href='#crh-comment-Pull b1af80b6a90386ea9a85f16634df8220643b54fe ADALiOS/ADALiOS/ADAuthenticationContext.m 51'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/AzureAD/azure-activedirectory-library-for-objc/pull/232#discussion_r21695061'>File: ADALiOS/ADALiOS/ADAuthenticationContext.m:L192-245</a></b>
- <a href='https://github.com/omercs'><img border=0 src='https://avatars.githubusercontent.com/u/466805?v=3' height=16 width=16'></a> No action but to think about: This is a digest hash check. Caller could replace hash and modify message. Adal will say hashes are ok and move to decrypt step. Caller can make up multiple fake calls to get more infor about app's key. hmac gives more security since it will fail to validate if message changed.
- [ ] <a href='#crh-comment-Pull b1af80b6a90386ea9a85f16634df8220643b54fe ADALiOS/ADALiOS/ADBrokerKeyHelper.m 82'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/AzureAD/azure-activedirectory-library-for-objc/pull/232#discussion_r21695766'>File: ADALiOS/ADALiOS/ADBrokerKeyHelper.m:L161-204</a></b>
- <a href='https://github.com/omercs'><img border=0 src='https://avatars.githubusercontent.com/u/466805?v=3' height=16 width=16'></a> i suggest to check if memory allocated or not?
- [ ] <a href='#crh-comment-Pull b1af80b6a90386ea9a85f16634df8220643b54fe ADALiOS/ADALiOSTests/XCTestCase%2BTestHelperMethods.m 188'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/AzureAD/azure-activedirectory-library-for-objc/pull/232#discussion_r21696255'>File: ADALiOS/ADALiOSTests/XCTestCase+TestHelperMethods.m:L532-551</a></b>
- <a href='https://github.com/omercs'><img border=0 src='https://avatars.githubusercontent.com/u/466805?v=3' height=16 width=16'></a> I think you need
dispatch_release(sem)
after wait is done.
- [ ] <a href='#crh-comment-Pull b1af80b6a90386ea9a85f16634df8220643b54fe ADALiOS/ADALiOSTests/ADAuthenticationContextTests.m 61'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/AzureAD/azure-activedirectory-library-for-objc/pull/232#discussion_r21698483'>File: ADALiOS/ADALiOSTests/ADAuthenticationContextTests.m:L1146-1161</a></b>
- <a href='https://github.com/omercs'><img border=0 src='https://avatars.githubusercontent.com/u/466805?v=3' height=16 width=16'></a> When you call dispatch_release for this semaphore, signal and wait needs to be balanced. Reusing in multiple calls can give you weird error, but each call looks like in sync. I guess putitng the dispatch_release to the end will be ok.
- <a href='https://github.com/kpanwar'><img border=0 src='https://avatars.githubusercontent.com/u/5713915?v=3' height=16 width=16'></a> you do not need to call dispatch_release as per http://stackoverflow.com/questions/8618632/does-arc-support-dispatch-queues
- [x] <a href='#crh-comment-Pull b1af80b6a90386ea9a85f16634df8220643b54fe ADALiOS/ADALiOS/ADAuthenticationResult%2BInternal.m 24'></a> <img src='http://www.codereviewhub.com/site/github-completed.png' height=16 width=60>&nbsp;<b><a href='https://github.com/AzureAD/azure-activedirectory-library-for-objc/pull/232#discussion_r21695485'>File: ADALiOS/ADALiOS/ADAuthenticationResult+Internal.m:L102-142</a></b>
- <a href='https://github.com/omercs'><img border=0 src='https://avatars.githubusercontent.com/u/466805?v=3' height=16 width=16'></a> wrong key
- [x] <a href='#crh-comment-Pull b1af80b6a90386ea9a85f16634df8220643b54fe ADALiOS/ADALiOS/ADBrokerKeyHelper.h 15'></a> <img src='http://www.codereviewhub.com/site/github-completed.png' height=16 width=60>&nbsp;<b><a href='https://github.com/AzureAD/azure-activedirectory-library-for-objc/pull/232#discussion_r21695588'>File: ADALiOS/ADALiOS/ADBrokerKeyHelper.h:L37-44</a></b>
- <a href='https://github.com/omercs'><img border=0 src='https://avatars.githubusercontent.com/u/466805?v=3' height=16 width=16'></a> Is this key created from app?
- <a href='https://github.com/kpanwar'><img border=0 src='https://avatars.githubusercontent.com/u/5713915?v=3' height=16 width=16'></a> Yes. ADAL creates the key and stores it in keychain. it is sent to the broker and and it uses the key to encrypt the payload.


<a href='https://www.codereviewhub.com/AzureAD/azure-activedirectory-library-for-objc/pull/232?mark_as_completed=1'><img src='http://www.codereviewhub.com/site/github-mark-as-completed.png' height=26></a>&nbsp;<a href='https://www.codereviewhub.com/AzureAD/azure-activedirectory-library-for-objc/pull/232?approve=1'><img src='http://www.codereviewhub.com/site/github-approve.png' height=26></a>&nbsp;<a href='https://github.com/AzureAD/azure-activedirectory-library-for-objc/pull/232'><img src='http://www.codereviewhub.com/site/github-refresh.png' height=26></a>
<a href='#crh-end'></a>
http://stackoverflow.com/questions/8618632/does-arc-support-dispatch-queues