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

#6155: OAuth 2.0 - Microsoft #6192

Merged
merged 22 commits into from
Oct 29, 2019
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
79ccd4c
6155 - OAuth 2.0 - Microsoft
Sep 19, 2019
481dc81
adding documentation
djbrooke Sep 19, 2019
246d2b8
updating documentation
djbrooke Sep 19, 2019
efee662
Merge branch '6155-OAuth-2.0-Microsoft' of https://github.com/CIMMYT/…
djbrooke Sep 19, 2019
1bf38c2
adding microsoft to page title
djbrooke Sep 19, 2019
ff84855
adding documentation
alejandratenorio Sep 20, 2019
1f9630a
adding documentation
alejandratenorio Sep 20, 2019
9ef52c5
Add fix for ORCID XML problem when an user is authenticated with Micr…
Sep 20, 2019
982bd88
Merge branch '6155-OAuth-2.0-Microsoft' of https://github.com/CIMMYT/…
Sep 20, 2019
1638ca5
Merge branch 'develop' into 6155-OAuth-2.0-Microsoft
Oct 9, 2019
a4d8114
Merge branch 'develop' into 6155-OAuth-2.0-Microsoft
Oct 21, 2019
4828bf7
Update MicrosoftOauth2AP for ScribeJava 6.6.3
Oct 22, 2019
3f2420d
Update MicrosoftOauth2AP for ScribeJava 6.6.3
Oct 22, 2019
ed8950f
Remove comments from AbstractOAuth2AuthenticationProvider.java
Oct 23, 2019
3ab5e25
switching back to ol' Google
djbrooke Oct 24, 2019
97a7549
changing from ms to ms azure ad
djbrooke Oct 24, 2019
a44bb2b
one more instance of ms to ms azure ad
djbrooke Oct 24, 2019
31df5a2
a few more cases where we want to be specific about azure ad
djbrooke Oct 24, 2019
4931d6a
correct number of =, scalable title
djbrooke Oct 24, 2019
44c37b4
Update clases and .gitignore
Oct 25, 2019
38d88e2
Merge branch '6155-OAuth-2.0-Microsoft' of https://github.com/CIMMYT/…
Oct 25, 2019
ab8714e
make error message non-ORCID specific
djbrooke Oct 25, 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
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import edu.harvard.iq.dataverse.authorization.providers.oauth2.impl.GitHubOAuth2AP;
import edu.harvard.iq.dataverse.authorization.providers.oauth2.impl.GoogleOAuth2AP;
import edu.harvard.iq.dataverse.authorization.providers.oauth2.impl.OrcidOAuth2AP;
import edu.harvard.iq.dataverse.authorization.providers.oauth2.impl.MicrosoftOAuth2AP;
import edu.harvard.iq.dataverse.authorization.providers.shib.ShibAuthenticationProvider;
import edu.harvard.iq.dataverse.authorization.providers.shib.ShibAuthenticationProviderFactory;
import edu.harvard.iq.dataverse.authorization.users.ApiToken;
Expand Down Expand Up @@ -880,13 +881,15 @@ public AuthenticatedUser canLogInAsBuiltinUser(String username, String password)
public List<String> getAuthenticationProviderIdsSorted() {
GitHubOAuth2AP github = new GitHubOAuth2AP(null, null);
GoogleOAuth2AP google = new GoogleOAuth2AP(null, null);
MicrosoftOAuth2AP microsoft = new MicrosoftOAuth2AP(null, null);
return Arrays.asList(
BuiltinAuthenticationProvider.PROVIDER_ID,
ShibAuthenticationProvider.PROVIDER_ID,
OrcidOAuth2AP.PROVIDER_ID_PRODUCTION,
OrcidOAuth2AP.PROVIDER_ID_SANDBOX,
github.getId(),
google.getId()
google.getId(),
microsoft.getId()
);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Copy link
Member

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

13:48
Hi!

We use this line because Microsoft identity platform needs this header when send
an authentication request. If this header is out, we receive an error.

image

We work in a fix for the ORCID XML problem.

Copy link
Member

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.

Copy link
Contributor

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.

Copy link
Contributor

@Gerafp Gerafp Sep 20, 2019

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.



final Response response = request.send();
int responseCode = response.getCode();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import edu.harvard.iq.dataverse.authorization.providers.oauth2.impl.GitHubOAuth2AP;
import edu.harvard.iq.dataverse.authorization.providers.oauth2.impl.GoogleOAuth2AP;
import edu.harvard.iq.dataverse.authorization.providers.oauth2.impl.OrcidOAuth2AP;
import edu.harvard.iq.dataverse.authorization.providers.oauth2.impl.MicrosoftOAuth2AP;
import java.util.Arrays;
import java.util.HashMap;
import java.util.Map;
Expand All @@ -31,6 +32,7 @@ public OAuth2AuthenticationProviderFactory() {
builders.put("github", (row, data) -> readRow(row, new GitHubOAuth2AP(data.get("clientId"), data.get("clientSecret"))));
builders.put("google", (row, data) -> readRow(row, new GoogleOAuth2AP(data.get("clientId"), data.get("clientSecret"))));
builders.put("orcid", (row, data) -> readRow(row, new OrcidOAuth2AP(data.get("clientId"), data.get("clientSecret"), data.get("userEndpoint"))));
builders.put("microsoft", (row, data) -> readRow(row, new MicrosoftOAuth2AP(data.get("clientId"), data.get("clientSecret"))));
}

@Override
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
package edu.harvard.iq.dataverse.authorization.providers.oauth2.impl;

import com.github.scribejava.core.builder.api.DefaultApi20;
import com.github.scribejava.core.model.OAuthConfig;
import com.github.scribejava.core.model.ParameterList;
import com.github.scribejava.core.model.Verb;
import java.util.Map;

/**
*
* @CIMMYT
*
*/

public class MicrosoftAzureApi extends DefaultApi20{

private static class InstanceHolder {
private static final MicrosoftAzureApi INSTANCE = new MicrosoftAzureApi();
}

public static MicrosoftAzureApi instance()
{
return InstanceHolder.INSTANCE;
}

@Override
public Verb getAccessTokenVerb()
{
return Verb.POST;
}

@Override
public String getAccessTokenEndpoint()
{
return "https://login.microsoftonline.com/common/oauth2/v2.0/token";
}

@Override
protected String getAuthorizationBaseUrl()
{
return "https://login.microsoftonline.com/common/oauth2/v2.0/authorize";
}

@Override
public String getAuthorizationUrl(OAuthConfig config, Map<String, String> additionalParams)
{
ParameterList parameters = new ParameterList(additionalParams);

parameters.add("response_type", config.getResponseType());
parameters.add("client_id", config.getApiKey());
String callback = config.getCallback();
if (callback != null) {
parameters.add("redirect_uri", callback);
}
parameters.add("scope", "user.read");
//parameters.add("domain_hint", "cgiar.org");

String state = config.getState();
if (state != null) {
parameters.add("state", state);
}
return parameters.appendTo(getAuthorizationBaseUrl());
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
package edu.harvard.iq.dataverse.authorization.providers.oauth2.impl;

import com.github.scribejava.core.builder.api.BaseApi;
import com.github.scribejava.core.oauth.OAuth20Service;
import edu.harvard.iq.dataverse.authorization.providers.oauth2.AbstractOAuth2AuthenticationProvider;

import java.util.Collections;
import java.util.logging.Logger;
import java.io.StringReader;
import javax.json.Json;
import javax.json.JsonObject;
import javax.json.JsonReader;
import edu.harvard.iq.dataverse.authorization.AuthenticatedUserDisplayInfo;

/**
*
* @author
*/
public class MicrosoftOAuth2AP extends AbstractOAuth2AuthenticationProvider{
Copy link
Contributor

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, ...).


private static final Logger logger = Logger.getLogger(MicrosoftOAuth2AP.class.getCanonicalName());

public MicrosoftOAuth2AP(String aClientId, String aClientSecret){
this.id = "microsoft";
Copy link
Contributor

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.

this.title = "Microsoft";
this.clientId = aClientId;
this.clientSecret = aClientSecret;
this.scope = "user.read";
this.baseUserEndpoint = "https://graph.microsoft.com/v1.0/me";
}

@Override
public BaseApi<OAuth20Service> getApiInstance(){
return MicrosoftAzureApi.instance();
}

@Override
protected ParsedUserResponse parseUserResponse(final String responseBody) {
try ( StringReader rdr = new StringReader(responseBody);
JsonReader jrdr = Json.createReader(rdr) ) {
JsonObject response = jrdr.readObject();
AuthenticatedUserDisplayInfo displayInfo = new AuthenticatedUserDisplayInfo(
response.getString("givenName", ""),
response.getString("surname", ""),
response.getString("userPrincipalName", ""),
Copy link
Contributor

@poikilotherm poikilotherm Oct 23, 2019

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.

Copy link
Member

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.

Copy link
Contributor

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.

Copy link
Contributor

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 the jobTitle claim is similar.
If we considered all Microsoft accounts for login in Dataverse, We need use userPrincipalName.

Copy link
Member

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.

"", "");
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

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

Copy link
Contributor

@poikilotherm poikilotherm Oct 23, 2019

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 with jobTitle claim already accessible with User.Read? Does that value make sense here? 🤔

String persistentUserId = response.getString("id");
String username = response.getString("userPrincipalName");
return new ParsedUserResponse(displayInfo, persistentUserId, username,
(displayInfo.getEmailAddress().length() > 0 ? Collections.singletonList(displayInfo.getEmailAddress()) : Collections.emptyList() )
Copy link
Contributor

@poikilotherm poikilotherm Oct 23, 2019

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?

Copy link
Contributor

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

);
}
}

public boolean isDisplayIdentifier()
{
return false;
}
}