Skip to content

B2c implementation with policy parameter #110

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.

Sign up for GitHub

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

Closed
abhidnya13 wants to merge 9 commits into from
Closed

B2c implementation with policy parameter #110

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
a3b2af8
Basic draft
abhidnya13 Oct 14, 2019
3d9e805
Merging recent changes
abhidnya13 Oct 15, 2019
605fe48
Adding policy as a separate parameter
abhidnya13 Oct 17, 2019
4837da6
Iterating
abhidnya13 Oct 17, 2019
4cf4311
Minor test changes
abhidnya13 Oct 18, 2019
1d1d8ac
Fixing tests and reverting instance discovery to return tenant discov…
abhidnya13 Oct 18, 2019
017d9d3
Fixing typo in parameter name
abhidnya13 Oct 18, 2019
997ece3
Updating B2C test cases to use new api
abhidnya13 Oct 18, 2019
2c26be5
Making changes on how userflow is passed to authority
abhidnya13 Oct 23, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Fixing tests and reverting instance discovery to return tenant discov… 
…ery endpoint
  • Loading branch information
@abhidnya13
abhidnya13 committed Oct 18, 2019
commit 1d1d8ac892a539fa3881b73b58b4442521ac779b
13 changes: 8 additions & 5 deletions msal/authority.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,11 +46,10 @@ def __init__(self, authority_url, validate_authority=True,
self.is_b2c = True if trust_framework_policy else False
if (tenant != "adfs" and (not self.is_b2c) and validate_authority
and self.instance not in WELL_KNOWN_AUTHORITY_HOSTS):
payload = instance_discovery(
tenant_discovery_endpoint = instance_discovery(
"https://{}{}/oauth2/v2.0/authorize".format(
self.instance, authority.path),
verify=verify, proxies=proxies, timeout=timeout)
tenant_discovery_endpoint = payload['tenant_discovery_endpoint']
else:
tenant_discovery_endpoint = (
'https://{}{}{}{}/.well-known/openid-configuration'.format(
Expand Down Expand Up @@ -97,15 +96,19 @@ def canonicalize(authority_url):
% authority_url)
return authority, authority.netloc, parts[1]

def instance_discovery(url, **kwargs):
return requests.get( # Note: This URL seemingly returns V1 endpoint only
def instance_discovery(url, response=None, **kwargs):
Copy link
Contributor

@rayluo rayluo Oct 21, 2019

Choose a reason for hiding this comment

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

Since B2C does not do Instance Discovery at all, this PR should not need to change this helper.

Regardless of the B2C api interface, this helper was recently updated as part of an improvement for non-B2C scenarios. Now that change was lost, possibly caused by an improper merge between the dev branch and your local branch? We can fix that, by either cherry-pick those lost commits back, or it might actually be easier to branch off the latest dev, and then add the needed changes on top of it.

resp = requests.get( # Note: This URL seemingly returns V1 endpoint only
'https://{}/common/discovery/instance'.format(
WORLD_WIDE # Historically using WORLD_WIDE. Could use self.instance too
# See https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/blob/4.0.0/src/Microsoft.Identity.Client/Instance/AadInstanceDiscovery.cs#L101-L103
# and https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/blob/4.0.0/src/Microsoft.Identity.Client/Instance/AadAuthority.cs#L19-L33
),
params={'authorization_endpoint': url, 'api-version': '1.0'},
**kwargs).json()
**kwargs)
payload = response or resp.json()
if 'tenant_discovery_endpoint' not in payload:
raise MsalServiceError(status_code=resp.status_code, **payload)
return payload['tenant_discovery_endpoint']

def tenant_discovery(tenant_discovery_endpoint, **kwargs):
# Returns Openid Configuration
Expand Down
2 changes: 1 addition & 1 deletion tests/test_authority.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ def test_lessknown_host_will_return_a_set_of_v1_endpoints(self):
self.assertNotIn('v2.0', a.token_endpoint)

def test_unknown_host_wont_pass_instance_discovery(self):
with self.assertRaisesRegexp(ValueError, "invalid_instance"):
with self.assertRaisesRegexp(MsalServiceError, "invalid_instance"):
Authority('https://unknown.host/tenant_doesnt_matter_in_this_case')

def test_invalid_host_skipping_validation_meets_connection_error_down_the_road(self):
Expand Down
2 changes: 1 addition & 1 deletion tests/test_e2e.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ def assertCacheWorksForApp(self, result_from_wire, scope):
"We should get a cached AT")

def _test_username_password(self,
authority=None, client_id=None, username=None, password=None, scope=None, trust_framework_policy=None
authority=None, client_id=None, username=None, password=None, scope=None, trust_framework_policy=None,
**ignored):
assert authority and client_id and username and password and scope
self.app = msal.PublicClientApplication(client_id, authority=authority, trust_framework_policy= trust_framework_policy)
Expand Down