Add support for dSTS enpoints #164
Conversation
add whitelist function so dsts hosts pass validation.
Adding unit test to test static instance discovery of dsts enpoints and validating of a dsts endpoint in AuthenticationContext.
|
|
|
Documentation on dSTS https://aka.ms/dsts |
adal/authority.py
Outdated
| @@ -84,11 +90,14 @@ def _parse_authority(self): | |||
| self._tenant = path_parts[1] | |||
| except IndexError: | |||
| raise ValueError("Could not determine tenant.") | |||
|
|
|||
There was a problem hiding this comment.
Nitpicking: would you mind remove the unnecessary space(s) introduced in this PR? See the explanation in another comment below.
There was a problem hiding this comment.
Removed unnecessary space
adal/authority.py
Outdated
| @@ -118,7 +127,7 @@ def _perform_dynamic_instance_discovery(self): | |||
| operation = "Instance Discovery" | |||
| self._log.debug("Attempting instance discover at: %(discovery_endpoint)s", | |||
| {"discovery_endpoint": discovery_endpoint.geturl()}) | |||
|
|
|||
There was a problem hiding this comment.
Nitpicking: would you mind remove the unnecessary space(s) introduced in this PR? Currently they are displayed as changes in this github PR page and then become a distraction during Code Review, twice: one time in this PR review, and another time in future when another person modify this file again and his/her editor is configured to trim trailing spaces automatically.
There was a problem hiding this comment.
Removed unnecessary space
adal/constants.py
Outdated
| @@ -215,6 +215,14 @@ class AADConstants(object): | |||
| 'login.microsoftonline.us', | |||
| 'login.microsoftonline.de', | |||
| ] | |||
| WHITELISTED_DOMAINS = [ | |||
| #add dsts domains to whitelist | |||
There was a problem hiding this comment.
Suggest to modify this comment to include the link to Domain Reference, something like this:
# Define dSTS domains whitelist based on its Supported Environments & National Clouds list here
# https://microsoft.sharepoint.com/teams/AzureSecurityCompliance/Security/SitePages/dSTS%20Fundamentals.aspx
tests/test_authority.py
Outdated
| @@ -186,6 +192,10 @@ def test_url_extra_path_elements(self): | |||
| "https://login.microsoftonline.com/your_tenant"): | |||
| context = AuthenticationContext(self.nonHardCodedAuthority + '/extra/path') | |||
|
|
|||
| @httpretty.activate | |||
| def test_dsts_authority(self): | |||
| context = AuthenticationContext(self.dstsTestEndpoint) | |||
There was a problem hiding this comment.
Please follow the pattern described in the highest-voted answer here to add self.fail(...). The current approach in this PR is not very clear to convey your intention. It would cause the first impression in a reviewer's mind as "was this test case unfinished?".
PS: for the record, your current approach is close to the 2nd-highest voted answer in that Stackoverflow thread. But that answer's vote counts (52) are far behind, and with yet-another-high-voted (30) comment AGAINST such answer. So, the developer comminuty has voted, they don't like this approach.
There was a problem hiding this comment.
Added self.fail().
Remove unnecessary white space, add documentation to whitelist, use self.fail in unit test.
There was a problem hiding this comment.
Implementation wise it looks good.
@navyasric do you have anything to add or comment for this new feature in general?
Add support for dSTS by white-listing dSTS domains so that this library can be used to request Oauth2 tokens from dSTS.