OAuth1 joining #608
OAuth1 joining #608
Conversation
There was a problem hiding this comment.
Nice! Just some minor comments.
Also not sure if you saw #598, might be good to reference that somewhere in this PR
...y-auth-flickr/src/main/java/org/datatransferproject/auth/flickr/FlickrAuthDataGenerator.java
Outdated
Show resolved
Hide resolved
| logger.debug( | ||
| "Error retrieving Flickr Credentials. Did you set {} and {}?", FLICKR_KEY, FLICKR_SECRET); | ||
| } | ||
| public class FlickrAuthServiceExtension extends OAuth1ServiceExtension { |
There was a problem hiding this comment.
nit: class docs here & elsewhere
...uth-smugmug/src/main/java/org/datatransferproject/auth/smugmug/SmugMugAuthDataGenerator.java
Outdated
Show resolved
Hide resolved
...uth-twitter/src/main/java/org/datatransferproject/auth/twitter/TwitterAuthDataGenerator.java
Outdated
Show resolved
Hide resolved
| /** | ||
| * Returns the {@link OAuthSigner} for the initial token request | ||
| */ | ||
| OAuthSigner getRequestTokenSigner(String clientSecret); |
There was a problem hiding this comment.
all impls look the same, want to provide an interface default for now and we can always split it out later if needed? (I know you mentioned you were not sure if HMAC is always used for oauth1)
|
|
||
| @Override | ||
| public AuthData generateAuthData(String callbackBaseUrl, String authCode, String id, | ||
| AuthData initialAuthData, String extra) { |
There was a problem hiding this comment.
do we still need extra?
There was a problem hiding this comment.
I don't see it being used anywhere, but I'm reluctant to change the interface.
| Preconditions.checkArgument(!Strings.isNullOrEmpty(config.getAccessTokenUrl()), | ||
| "Config is missing access token url"); | ||
| Preconditions | ||
| .checkArgument(config.getExportScopes() != null, "Config is missing export scopes"); |
There was a problem hiding this comment.
I think some APIs may not use OAuth scopes. It's probably bad in that case but technically valid OAuth1 protocol?
There was a problem hiding this comment.
Good point. What do you think about adding a possibility for config.whenAddScopes() to return NEVER?
There was a problem hiding this comment.
Discussed offline, we're going to maintain the scopes requirement until a counterexample presents itself.
There was a problem hiding this comment.
Ack, mind just documenting here that this decision is not OAuth spec but rather us trying to encourage good practices and no accidental scope omission?
libraries/auth/src/main/java/org/datatransferproject/auth/OAuth1ServiceExtension.java
Outdated
Show resolved
Hide resolved
| .checkArgument(!Strings.isNullOrEmpty(config.getAuthUrl()), "Config is missing auth url"); | ||
| Preconditions | ||
| .checkArgument(!Strings.isNullOrEmpty(config.getTokenUrl()), "Config is missing token url"); | ||
| Preconditions |
There was a problem hiding this comment.
same here about scopes for OAuth2, but please double check in case I'm wrong
There was a problem hiding this comment.
Looks like scopes are not required, as evidence by blog posts saying they shouldn't always be used.
There was a problem hiding this comment.
same here re documenting scope decision
…transferproject/auth/flickr/FlickrAuthDataGenerator.java
…atransferproject/auth/smugmug/SmugMugAuthDataGenerator.java
…atransferproject/auth/twitter/TwitterAuthDataGenerator.java
…h1ServiceExtension.java
| import com.google.api.client.auth.oauth.OAuthSigner; | ||
| import java.util.Map; | ||
|
|
||
| public interface OAuth1Config { |
| Preconditions.checkArgument(!Strings.isNullOrEmpty(config.getAccessTokenUrl()), | ||
| "Config is missing access token url"); | ||
| Preconditions | ||
| .checkArgument(config.getExportScopes() != null, "Config is missing export scopes"); |
There was a problem hiding this comment.
Ack, mind just documenting here that this decision is not OAuth spec but rather us trying to encourage good practices and no accidental scope omission?
| .checkArgument(!Strings.isNullOrEmpty(config.getAuthUrl()), "Config is missing auth url"); | ||
| Preconditions | ||
| .checkArgument(!Strings.isNullOrEmpty(config.getTokenUrl()), "Config is missing token url"); | ||
| Preconditions |
There was a problem hiding this comment.
same here re documenting scope decision
Made new OAuth1{Config, DataGenerator, ServiceExtension} classes. So far, Flickr, SmugMug, and Twitter 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 OAuth1 services are combined and verified, I will remove the old auth code; until then, there will be two auth flows coexisting.
The current OAuth{1, 2} configs will eventually be replaced (see #598).