-
Notifications
You must be signed in to change notification settings - Fork 495
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
Conversation
Can one of the admins verify this patch? |
Thanks @alejandratenorio for the PR. We'll review this and I'll also add some documentation. |
Since the merge of gdcc/dataverse-kubernetes#87, I am working on #5991 and it's recent PR about updating ScribeJava. It might be a good idea to have that merged first, so it doesn't have to be me testing the Microsoft OAuth2... 😉 |
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! The main thing this pull request needs is documentation in doc/sphinx-guides/source/installation/oauth2.rst
@alejandratenorio do you want to work on this?
Also, I left a comment about JSON vs. XML.
@@ -116,6 +116,10 @@ public OAuth2UserRecord getUserRecord(String code, String state, String redirect | |||
final OAuthRequest request = new OAuthRequest(Verb.GET, userEndpoint, service); | |||
request.addHeader("Authorization", "Bearer " + accessToken.getAccessToken()); | |||
request.setCharset("UTF-8"); | |||
|
|||
// Microsoft | |||
request.addHeader("Accept", "application/json"); |
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'm slightly concerned about this because doesn't ORCID use XML instead of JSON?
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.
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 perfect. Thanks! Please let us know when that fix is in.
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.
Thanks @Gerafp - I'll move this PR back to Community Dev.
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 add a instruction for manage the authentication request when use Microsoft.
The branch has updated with our fix.
Hi @Gerafp and @alejandratenorio, can one of you please give me permission to push to this branch? I have some documentation edits ready to push. Thanks! |
@djbrooke, sure, it's done. |
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.
Please be aware that in current releases of ScribeJava an upstream API client is available for this Microsoft endpoint. IMHO it would be a pity to refactor in #5997 and not use the advantages of an updated lib.
Thanks @Gerafp and @alejandratenorio for giving me push access! Please take a look at the docs I added and feel free to expand those sections if necessary. Also note that I did not add any info about the json template at line 55 in oauth.rst, as it seems that still needs to be finalized. Thanks again for this contribution! |
I'm a little confused. Currently Dataverse uses ScribeJava 3.1.0. According to https://github.com/scribejava/scribejava/tree/scribejava-3.1.0#supports-all-major-10a-and-20-oauth-apis-out-of-the-box the following Microsoft API is supported in 3.1.0:
If we upgrade to ScribeJava 6.8.1 (the latest), https://github.com/scribejava/scribejava/tree/scribejava-6.8.1#supports-all-major-10a-and-20-oauth-apis-out-of-the-box indicates that a total of three Microsoft APIs are supported:
Which of these three Microsoft APIs will Dataverse support when this pull request is merged? All of them? One of them? |
@djbrooke, I added info about microsoft.json file. |
…dataverse into 6155-OAuth-2.0-Microsoft Add fix for ORCID XML problem when a user is autenticated with Microsoft
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.
Now that 9ef52c5 is in I believe the code is ready for QA so I'm approving this. The docs seem to be in pretty good shape too.
I have one or two more things to check here. Hoping to move over before standup. :) |
Thanks all, sorry for the delay. I made one last change to generalize an error message in ab8714e. I'm also fine with the buttons as they are. I'm going to move this over to QA now. I'll take a look at the docs in parallel. @kcondon, I tested this successfully with an outlook.com account I created and I also tested this with my Harvard institutional account and the flow worked as I expected, in line with our other Oauth login options. I know that we talked about this a little bit this morning, but if you want to discuss in more detail let me know. |
Hi @Gerafp , I'm testing this pr and running into a bit of an issue. You've tested both public (microsoft live?) and institutional azure ad/oauth accounts, correct? When you did so, how did you get your client id and secret to configure Dataverse? My understanding from the Azure AD link provided in the docs is one needs to register the app (dataverse), with the Azure AD tenant (directory authenticator/IdP), and that is what gives you the id and secret. If that is the case, I understand about the institutional AD registration and am pursuing that with my local university. However, how does that work with a public service like Live? I'm reading about the /tenant versus the /common userEndpoint but not sure how/where to register to get the right credentials. Thanks for any advice. Kevin P.S. Is this implementation multi-tenant by default? I was looking at this: https://docs.microsoft.com/en-us/azure/active-directory/develop/howto-convert-app-to-be-multi-tenant |
Hi @kcondon. You can registry your app in this link Documentation When you registry you app, You obtain the |
Thanks @Gerafp we'll take another look at this on Monday! |
So, I followed the above link and it takes me to the azure portal. My account does not have permission to register an application. This is what I had encountered before. I had looked at the developer quick start guide earlier and that was helpful in better understanding how the accounts worked and how multi tenant (common) and single tenant (tenantid) endpoints worked to support a variety of microsoft identities. What I think I learned is this:
Would you confirm these steps are correct? Also, please send the microsoft.json file(s) you used for institutional and public account testing, client id and secret removed. If the above directions are correct, I think we should make the instructions more like these. |
I've reached out to our contacts at Harvard to try and set up some test institutional creds. |
Hi @kcondon, What kind of account did you use? I used my personal account (live.com.mx) and my cgiar account, in both I have permission to register an application. |
@alejandratenorio When I clicked on the azure portal I was automatically logged in and under switch directory I saw Harvard University as a tenant. Harvard has a subscription, as I understand it, because they provide access to Microsoft cloud applications and we use outlook/web for email. My guess is I have a regular user account as defined by our organization. That is why I was emphasizing the permissions part. I do have a support request in to our IT group for more information. Also, at @pdurbin suggestion, I created a new microsoft account, accessed the portal and had even less permission -no tenants, no ability to click on switch directories. |
Hi! I dont know the reason becausse this happen, Can you give me access to a institutional account for check this situation?. Personally, I have a institutional account with domain |
Thanks @Gerafp. We unfortunately do not have the ability to edit any of this from our side, as the institutional account here is centrally managed. We're waiting to hear back from support and I'll let you know as soon as I have more information. I also reached out to our local ops group to see if they have any ideas. |
@alejandratenorio @Gerafp OK, I was able to get the live account to work, thanks for the link. I was also able to get the account I create from Friday working. Not sure why it did not work when I tried it. I can also authenticate my institutional account when I specify the user endpoint: https://login.microsoftonline.com/common but I am unable to register an app with that account -presumably due to a local IT policy. So, this can be merged. Apologies for the confusion. Kevin |
New Contributors
Welcome! New contributors should at least glance at CONTRIBUTING.md, especially the section on pull requests where we encourage you to reach out to other developers before you start coding. Also, please note that we measure code coverage and prefer you write unit tests. Pull requests can still be reviewed without tests or completion of the checklist outlined below. Thanks!
Related Issues
Pull Request Checklist