-
Notifications
You must be signed in to change notification settings - Fork 500
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
Changes from 1 commit
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
Diff view
Diff view
There are no files selected for viewing
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{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMHO this class should be renamed to |
||
|
||
private static final Logger logger = Logger.getLogger(MicrosoftOAuth2AP.class.getCanonicalName()); | ||
|
||
public MicrosoftOAuth2AP(String aClientId, String aClientSecret){ | ||
this.id = "microsoft"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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", ""), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Shouldn't we be using the 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. If you considered the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi! We test with There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Could the |
||
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() ) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Could you please test this for us? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, We test this. :D |
||
); | ||
} | ||
} | ||
|
||
public boolean isDisplayIdentifier() | ||
{ | ||
return false; | ||
} | ||
} |
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.
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.
We work in a fix for the ORCID XML problem.
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.