Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
#6155: OAuth 2.0 - Microsoft #6192
#6155: OAuth 2.0 - Microsoft #6192
Changes from all commits
79ccd4c
481dc81
246d2b8
efee662
1bf38c2
ff84855
1f9630a
9ef52c5
982bd88
1638ca5
a4d8114
4828bf7
3f2420d
ed8950f
3ab5e25
97a7549
a44bb2b
31df5a2
4931d6a
44c37b4
38d88e2
ab8714e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still didn't get why this is needed. Could you please elaborate and add a comment next to the code so someone looking at the codebase later has a direct reminder why this is here? Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO this class should be renamed to
MicrosoftAzureOAuth2AP
. One might add other MS based OAuth2 flows later (like MS Live, ...).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ID should correspond to the class name, so if that is going to be changed, this should change as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO it's not a good idea to prefill the users email attribute with the
userPrincipalName
claim.According to https://docs.microsoft.com/en-us/azure/active-directory/hybrid/plan-connect-userprincipalname, it is not guaranteed that you will have an email-adress in there. Your AD admin can decide otherwise.
Shouldn't we be using the
mail
claim instead?Related docs:
As Dataverse is able to deal with an empty mail attribute, it should be ok if we receive an empty value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't click any of the links above, but yes, if an email address is not provided, the user will be asked to fill it in before creating their Dataverse account.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you considered the
mail
claim is the best option for this problem, We can change it.An advantage for Microsoft Login is the possibility of get a user email directly from API compared with others services such as Orcid or GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi!
We test with
mail
claim but is valid only for institutional accounts, unfortunaly thejobTitle
claim is similar.If we considered all Microsoft accounts for login in Dataverse, We need use
userPrincipalName
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Gerafp if you're happy with the code, I'm happy. I'm moving this to QA.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be beyond scope, but it would be totally awesome trying to receive values for
affiliation
fields.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We view the options for receive these values and can implement this in the future. It's really interesting for us. :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could the
position
field be filled withjobTitle
claim already accessible withUser.Read
? Does that value make sense here? 🤔There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that the GitHub implementation is using this, too. I am really keen to see if this actually works, as the email response is retrieved as a string, not being converted to a list anywhere as far as I can see.
I don't know if this has ever been tested with the GitHub provider. It's a good idea to test this anyway, as I don't know what happens when you receive a JSON array of mails in
response.getString()
.Could you please test this for us?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, We test this. :D