Skip to content
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.

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

Added short guide for connecting kratos to AzureAD #433

Merged
merged 2 commits into from
May 30, 2020

Conversation

ernax78
Copy link
Contributor

@ernax78 ernax78 commented May 26, 2020

Related issue

Proposed changes

Checklist

  • I have read the contributing guidelines.
  • I have read the security policy.
  • I confirm that this pull request does not address a security
    vulnerability. If this pull request addresses a security. vulnerability, I
    confirm that I got green light (please contact
    security@ory.sh) from the maintainers to push
    the changes.
  • I have added tests that prove my fix is effective or that my feature
    works.
  • I have added or changed the documentation.

Further comments

Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

Looking great! One thing regarding email_verified.

{
identity: {
traits: {
email: claims.email
Copy link
Member

Choose a reason for hiding this comment

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

Does AzureAD include email_verified? If so, we should probably add a warning here as we did for Google/GitHub: https://github.com/ory/kratos/pull/433/files#diff-2a62fee703dc56d70e23792e010a5a01L67-L73

Copy link
Contributor Author

@ernax78 ernax78 May 27, 2020

Choose a reason for hiding this comment

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

I guess a warning should be in place. There is an optional claim, verified_primary_email that can be queried. https://docs.microsoft.com/en-us/azure/active-directory/develop/active-directory-optional-claims. If using a one tenant solution (the setup I have in place) the mails are assigned by the administrator so they are "safe" to use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aeneasr - I read up on the issue on the Microsoft documentation, it seems like the same issue with unverified e-mails applies for Microsoft accounts as for GitHub, however, if you are using a corporate AzureAD (one you have control over), this does not apply and the email is to be considered as "verified". I did try to extract the verified_primary_email token that should theoretically work for this, however I did not have success at the moment (and it is not needed for my use-case). I think the documentation I submitted should be good to go with the warning / links attached.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, sounds good!

@CLAassistant
Copy link

CLAassistant commented May 29, 2020

CLA assistant check
All committers have signed the CLA.

email: claims.email
},
},
}
```

See https://docs.microsoft.com/en-us/azure/active-directory-b2c/user-flow-disable-email-verification for warnings.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
See https://docs.microsoft.com/en-us/azure/active-directory-b2c/user-flow-disable-email-verification for warnings.
Also see ["Disable email verification during customer sign-up in Azure Active Directory B2C"] for warnings.

@aeneasr aeneasr merged commit 7660bcd into ory:master May 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants