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

Auth joining #595

Merged
merged 10 commits into from Oct 16, 2018

Conversation

Projects
None yet
3 participants
@olsona
Contributor

olsona commented Oct 12, 2018

Made new OAuth2{Config, DataGenerator, ServiceExtension} classes. So far, Google and Instagram work.

The plan is to combine all the service-level auth flows where possible to allow for more uniform and easy-to-understand auth flows based on underlying protocol, and to reduce reliance on third-party SDKs for future secure deployment environments (see #553). Once all OAuth2 services are combined and verified, I will remove the old auth code; until then, there will be two auth flows coexisting. When OAuth2 is done, I will do the same with OAuth1 services.

@olsona olsona requested a review from holachuy Oct 12, 2018

@rtannenbaum

Nice!

Couple high level comments in addition to the one I left in code:

  1. I think OAuth2-level stuff should be moved to core auth library, from extensions. For example, I think OAuth2Config should be core library, whereas FacebookOAuthConfig should be in the facebook extension.

  2. Can you include some context about the migration you plan to do? I think it's confusing to have 2 versions of auth components without an explanation why.

HttpTransport httpTransport,
String dataType, AuthMode authMode) {
this.config = config;
this.clientId = appCredentials.getKey();

This comment has been minimized.

@rtannenbaum

rtannenbaum Oct 14, 2018

Contributor

Depending on how this auth data generator is used (whether as a singleton or we get new ones each time), it could be a problem to cache the client ID and secret here.

Could you file an issue & TODO to handle dynamic updates of client IDs/secrets? I think we should eventually handle this by keeping an AppCredentialStore here instead of the retrieved app credentials (ID and secret). Then the different AppCredentialStore's may be responsible for receiving dynamic updates (there's an example of this in GoogleAppCredentialStore already).

I would just leave an issue/Todo though, and not implement that now.

@holachuy

Very nice, mostly feedback on cleanups and clarifications

import java.util.Map;
import org.datatransferproject.auth.oauth2.OAuth2Config;
public class FacebookOAuthConfig implements OAuth2Config {

This comment has been minimized.

@holachuy

holachuy Oct 15, 2018

Collaborator

Let's add a comment with link to the facebook api documentation site.

@Override
public String getServiceName() {
return "Facebook";

This comment has been minimized.

@holachuy

holachuy Oct 15, 2018

Collaborator

Is this for display or a canonical value that should be all in the same case?

This comment has been minimized.

@olsona

olsona Oct 15, 2018

Contributor

I believe it's functionally only for display, aside from the client ids and secrets (which are the service name in all-caps)

import java.util.Map;
import org.datatransferproject.auth.oauth2.OAuth2Config;
public class GoogleOAuthConfig implements OAuth2Config {

This comment has been minimized.

@holachuy

holachuy Oct 15, 2018

Collaborator

Same comments here on documentation and values below

This comment has been minimized.

@olsona

olsona Oct 15, 2018

Contributor

Done.

This comment has been minimized.

@rtannenbaum

rtannenbaum Oct 16, 2018

Contributor

I don't see class docs here, do you?

This comment has been minimized.

@olsona

olsona Oct 16, 2018

Contributor

Sorry, I only added documentation to the methods, not the class.

import java.util.Map;
import org.datatransferproject.auth.oauth2.OAuth2Config;
public class InstagramOAuthConfig implements OAuth2Config {

This comment has been minimized.

@holachuy

holachuy Oct 15, 2018

Collaborator

Same comment here

This comment has been minimized.

@olsona

olsona Oct 15, 2018

Contributor

Done.

This comment has been minimized.

@rtannenbaum

rtannenbaum Oct 16, 2018

Contributor

same, I don't see this one either? is my diff wrong?

@Override
public Map<String, List<String>> getImportScopes() {
return ImmutableMap.of();

This comment has been minimized.

@holachuy

holachuy Oct 15, 2018

Collaborator

Comment here

This comment has been minimized.

@olsona

olsona Oct 15, 2018

Contributor

Done.

import java.util.Map;
import org.datatransferproject.auth.oauth2.OAuth2Config;
public class MicrosoftOAuthConfig implements OAuth2Config {

This comment has been minimized.

@holachuy

holachuy Oct 15, 2018

Collaborator

Same comment

import java.util.List;
import java.util.Map;
public interface OAuth2Config {

This comment has been minimized.

@holachuy

holachuy Oct 15, 2018

Collaborator

Document this interface

String getTokenUrl();
Map<String, List<String>> getExportScopes();

This comment has been minimized.

@holachuy

holachuy Oct 15, 2018

Collaborator

Does ordering or duplicates matter? Should it be a Set or OrderedSet instead?

This comment has been minimized.

@holachuy

holachuy Oct 15, 2018

Collaborator

Add comment including the first value is 'transferType' as defined in the auth data generators and elsewhere.

This comment has been minimized.

@olsona

olsona Oct 15, 2018

Contributor

Order does not seem to matter, but I can't imagine duplicates would be a good thing.

import java.util.Map;
import org.datatransferproject.auth.oauth2.OAuth2Config;
public class TwitterOAuthConfig implements OAuth2Config {

This comment has been minimized.

@holachuy

holachuy Oct 15, 2018

Collaborator

Same

olsona added some commits Oct 15, 2018

@rtannenbaum

Nice!

Few more nits in the comments, also could you document the current oauth2 classes with a note that we are in the process of deprecating for the new ones? that way it is clearer when looking in the code without the context of this PR.

Could you also link to a GH issue for more context? I think the notes you have in the PR description are good, only thing I would add is another goal of the refactor is to maintain vetted versions of auth handlers with minimal/no use of third party SDKs, for use in secure deployment environments.

Thanks!

import java.util.Map;
import org.datatransferproject.auth.oauth2.OAuth2Config;
public class GoogleOAuthConfig implements OAuth2Config {

This comment has been minimized.

@rtannenbaum

rtannenbaum Oct 16, 2018

Contributor

I don't see class docs here, do you?

}
@Override
// See https://developers.google.com/identity/protocols/OAuth2WebServer#creatingclient

This comment has been minimized.

@rtannenbaum

rtannenbaum Oct 16, 2018

Contributor

nit: sorry this is annoying but I think the comments should go above all annotations like @override

This comment has been minimized.

@olsona

olsona Oct 16, 2018

Contributor

Done.

import java.util.Map;
import org.datatransferproject.auth.oauth2.OAuth2Config;
public class InstagramOAuthConfig implements OAuth2Config {

This comment has been minimized.

@rtannenbaum

rtannenbaum Oct 16, 2018

Contributor

same, I don't see this one either? is my diff wrong?

import org.datatransferproject.types.transfer.auth.AuthData;
import org.datatransferproject.types.transfer.auth.TokensAndUrlAuthData;
public class OAuth2DataGenerator implements AuthDataGenerator {

This comment has been minimized.

@rtannenbaum

rtannenbaum Oct 16, 2018

Contributor

missing class doc?

This comment has been minimized.

@olsona

olsona Oct 16, 2018

Contributor

Done.

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
public class OAuth2ServiceExtension implements AuthServiceExtension {

This comment has been minimized.

@rtannenbaum

rtannenbaum Oct 16, 2018

Contributor

nit: missing class doc

This comment has been minimized.

@olsona

olsona Oct 16, 2018

Contributor

Done.

@rtannenbaum

LGTM!

Actually, are the old auth data generators even used any more? If not, want to just get rid of them (fine in a separate PR too)?

@olsona olsona merged commit caaaad0 into master Oct 16, 2018

3 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@olsona olsona deleted the auth-joining branch Oct 16, 2018

private boolean initialized = false;
public OAuth2ServiceExtension(OAuth2Config oAuth2Config) {
this.oAuth2Config = oAuth2Config;

This comment has been minimized.

@holachuy

holachuy Oct 16, 2018

Collaborator

We should probably validate that all the data is present in the config before continuing construction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment