Fix OIDC B2C problems #220

lovemaths · 2016-09-26T23:58:57Z

(1) Rewrote the metadata loading and the configuration code. Restructured OIDCStrategy.
(2) #188 B2C mocha tests (partially done, waiting for the AAD fix of missing nonce to add test for hybrid/code flow)
(3) #165 rename 'callbackURL' and 'returnURL' to 'redirectUrl'.
(4) #189 Extensibility to allow issuer validation when going against commend end point
(5) #194 error message for 'sub' mismatch is incorrect after redeeming 'code'
(6) #218 missing email claim for B2C

(1) Rewrote the metadata loading and the configuration code. Restructured OIDCStrategy. (2) #188 B2C mocha tests (partially done, waiting for the AAD fix of missing nonce to add test for hybrid/code flow) (3) #165 rename 'callbackURL' and 'returnURL' to 'redirectUrl'. (4) #189 Extensibility to allow issuer validation when going against commend end point (5) #194 error message for 'sub' mismatch is incorrect after redeeming 'code' (6) #218 missing email claim for B2C

polita · 2016-09-27T16:57:16Z

lib/oidcstrategy.js

@@ -69,18 +68,22 @@ const NONCE_MAX_AMOUNT = 10;
 const NONCE_LIFE_TIME = 3600; // second

 function makeProfileObject(src, raw) {
+  var emails = [];
+  if (src.upn)
+    emails = [src.upn];


@tushargupta51 Should we be using upn as emails?

upn is always the email

upn is not necessarily the email of the user. It might have @ in it. I think this property should be called username instead of emails.

For AAD, upn is always in the email format like '[email protected]'

You are right. It is always in email format. But I am not sure if we can use that as email of the user. I saw you updated the code to set emails: src.emails. What is src here? json for id token? Is emails a new property?

polita · 2016-09-27T16:57:39Z

lib/oidcstrategy.js

+  if (src.upn)
+    emails = [src.upn];
+  else if (src.emails)
+    emails = src.emails;


Is it possible to get both upn and emails back? What happens if we do? Do we ignore emails?

no. For B2C we get emails but not upn; for non B2C we get upn but no emails

I think for non B2c, we get email claim in the id token.

Confirmed with Danny, AAD won't return email claim in id_token. To get email claim, one needs to call graph api.

polita · 2016-09-27T17:08:53Z

lib/oidcstrategy.js

-      return self.fail('invalid nonce');
+    // For B2C, check the policy
+    if (self._options.isB2C) {
+      if (!jwtClaims.acr || jwtClaims.acr.length <= 6 || jwtClaims.acr.substring(0,6).toUpperCase() !== 'B2C_1_')


Please define the B2C_1_ string as a constant and then use value.length.

ok, will do it.

polita · 2016-09-27T17:13:31Z

lib/oidcstrategy.js

 * @params {Object} req
- * @params {Function} next
+ * @params {Function} nextt


typo? Are we actually using this variable?

we used to, but not anymore, will remove the 'next'

polita · 2016-09-27T17:16:08Z

lib/oidcstrategy.js

    }

-    log.info('received id_token: ', id_token);
+    log.info('received id_token: ', items.id_token);


Are we logging the contents of the id_token? I don't think we want to do that.

why not? logging is only seen by the developers, with the id_token it is much easier for them to debug issues

I spoke to Brent, and we assume for Wilson that the log can contain PII. This seems fine.

polita · 2016-09-27T17:17:04Z

lib/oidcstrategy.js

+      return self.failWithLog('In _flowInitializationHandler: missing policy in the request for B2C');
+    // policy is not case sensitive. AAD turns policy to lower case.
+    policy = req.query.p.toLowerCase();
+    if (!policy.startsWith('b2c_1_'))


Use the same constant as above.

brentschmaltz · 2016-09-27T21:58:02Z

lib/oidcstrategy.js

+ *                          (3) Description:
+ *                          the metadata endpoint provided by the Microsoft Identity Portal that provides 
+ *                          the keys and other important info at runtime. Examples:
+ *                          <1> v1 non-common endpoint


should we say: tenant endpoint

I think it is already very clear here with the examples.

Agree with Brent. We should call this "v1 tenant-specific (not common) endpoint".

@lovemaths Is this still an open issue?

changed it in the latest commit

brentschmaltz · 2016-09-27T21:59:58Z

lib/oidcstrategy.js

+ *                          (3) Description:
+ *                          How you get the authorization code and tokens back
+ *
+ *   - `redirectUrl`          (1) Required


indented too far

1. define B2C_1_ as a const 2. remove the 'next' parameters in a couple of function since it is no longer used 3. Fix a indentation problem

polita · 2016-10-05T17:41:34Z

tushargupta51 · 2016-10-05T17:52:24Z

(I only reviewed makeProfileObject method)

msftclas added the cla-not-required label Sep 26, 2016

lovemaths assigned tushargupta51, brentschmaltz and polita Sep 26, 2016

lovemaths force-pushed the sijliu/v3.0.0b2c branch from 53f4fdc to d60bdb6 Compare September 27, 2016 17:12

polita reviewed Sep 27, 2016

View reviewed changes

brentschmaltz reviewed Sep 27, 2016

View reviewed changes

add-fix

1f34a05

1. define B2C_1_ as a const 2. remove the 'next' parameters in a couple of function since it is no longer used 3. Fix a indentation problem

lovemaths assigned SomkaPe Sep 28, 2016

lovemaths added 4 commits September 28, 2016 13:29

changes to claims

bc4e638

fix a issuer validation bug

5d3e4c4

Issue #164: By default redirectUrl is https

33a971a

changed name: from non-common to tenant-specific

763a6cc

lovemaths merged commit 1759847 into sijliu/v3.0.0 Oct 6, 2016

lovemaths deleted the sijliu/v3.0.0b2c branch October 6, 2016 19:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix OIDC B2C problems #220

Fix OIDC B2C problems #220

lovemaths commented Sep 26, 2016 •

edited

Loading

polita Sep 27, 2016

lovemaths Sep 27, 2016

tushargupta51 Sep 28, 2016

lovemaths Sep 28, 2016

tushargupta51 Oct 3, 2016

polita Sep 27, 2016 •

edited

Loading

lovemaths Sep 27, 2016

tushargupta51 Sep 28, 2016

lovemaths Sep 28, 2016

polita Sep 27, 2016

lovemaths Sep 27, 2016

lovemaths Sep 28, 2016

polita Sep 27, 2016 •

edited

Loading

lovemaths Sep 27, 2016

lovemaths Sep 28, 2016

polita Sep 27, 2016

lovemaths Sep 27, 2016

polita Sep 28, 2016

polita Sep 27, 2016

lovemaths Sep 28, 2016

brentschmaltz Sep 27, 2016

lovemaths Sep 28, 2016

polita Sep 28, 2016

polita Oct 3, 2016

lovemaths Oct 3, 2016

polita Oct 5, 2016

brentschmaltz Sep 27, 2016

lovemaths Sep 28, 2016

polita commented Oct 5, 2016

tushargupta51 commented Oct 5, 2016

Fix OIDC B2C problems #220

Fix OIDC B2C problems #220

Conversation

lovemaths commented Sep 26, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

polita Sep 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

polita Sep 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

polita commented Oct 5, 2016

tushargupta51 commented Oct 5, 2016

lovemaths commented Sep 26, 2016 •

edited

Loading

polita Sep 27, 2016 •

edited

Loading

polita Sep 27, 2016 •

edited

Loading