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%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-library-for-objc/pull/232%23discussion_r21712932%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-library-for-objc/pull/232%23discussion_r21712947%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-library-for-objc/pull/232%23discussion_r21712948%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-library-for-objc/pull/232%23discussion_r21712949%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-library-for-objc/pull/232%23discussion_r21713419%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-library-for-objc/pull/232%23issuecomment-66698148%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%2C%20%7B%22body%22%3A%20%22%3A%2B1%3A%3Ca%20href%3D%27%23crh-update-none%27%3E%3C/a%3E%22%2C%20%22created_at%22%3A%20%222014-12-11T21%3A57%3A14Z%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%22General%20Comment%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/AzureAD/azure-activedirectory-library-for-objc/pull/232%23issuecomment-66698148%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22Sign%20off%22%2C%20%22created_at%22%3A%20%222014-12-11T22%3A08%3A31Z%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%22General%20Comment%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%2C%20%7B%22body%22%3A%20%22%3A%2B1%3A%3Ca%20href%3D%27%23crh-update-none%27%3E%3C/a%3E%22%2C%20%22created_at%22%3A%20%222014-12-11T21%3A57%3A14Z%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/ADAuthenticationContext.m%3AL192-245%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%2C%20%7B%22body%22%3A%20%22%3A%2B1%3A%22%2C%20%22created_at%22%3A%20%222014-12-11T22%3A03%3A08Z%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.m%3AL161-204%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%2C%20%7B%22body%22%3A%20%22http%3A//stackoverflow.com/questions/8618632/does-arc-support-dispatch-queues%22%2C%20%22created_at%22%3A%20%222014-12-11T21%3A56%3A52Z%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%3Ca%20href%3D%27%23crh-update-complete%27%3E%3C/a%3E%22%2C%20%22created_at%22%3A%20%222014-12-11T21%3A57%3A15Z%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/XCTestCase%2BTestHelperMethods.m%3AL532-551%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-General Comment'></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#issuecomment-66698148'>General Comment</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> Sign off
- [x] <a href='#crh-comment-Pull b1af80b6a90386ea9a85f16634df8220643b54fe ADALiOS/ADALiOS/ADAuthenticationContext.m 51'></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_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.
- [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.
- [x] <a href='#crh-comment-Pull b1af80b6a90386ea9a85f16634df8220643b54fe ADALiOS/ADALiOS/ADBrokerKeyHelper.m 82'></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_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?
- [x] <a href='#crh-comment-Pull b1af80b6a90386ea9a85f16634df8220643b54fe ADALiOS/ADALiOSTests/XCTestCase%2BTestHelperMethods.m 188'></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_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='https://github.com/kpanwar'><img border=0 src='https://avatars.githubusercontent.com/u/5713915?v=3' height=16 width=16'></a> http://stackoverflow.com/questions/8618632/does-arc-support-dispatch-queues
- [x] <a href='#crh-comment-Pull b1af80b6a90386ea9a85f16634df8220643b54fe ADALiOS/ADALiOSTests/ADAuthenticationContextTests.m 61'></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_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


<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>