Skip to content

Commit

Permalink
Don't save token from authorization header (#2228)
Browse files Browse the repository at this point in the history
When Dockstore API is invoked with a Google token,
we look up the user based on the email associated with that
token. If we don't find a user, we create one. Then we would
save the token along with the user; this was
done to use that token to authenticate subsequent API calls to SAM.

Instead, don't persist the token to the database.

Added a transient field to User to hold the token and when
calls are made to SAM, check that field first.

Not wild about changing the User class to not directly map
to the database -- we don't use @transient anywhere else. But this
seems to be the lowest impact way of implementing this.
  • Loading branch information
coverbeck committed Mar 15, 2019
1 parent 40ee223 commit 3d3ed0e
Show file tree
Hide file tree
Showing 4 changed files with 42 additions and 29 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,10 @@

package io.dockstore.webservice;

import java.util.List;
import java.util.Optional;

import com.google.api.services.oauth2.model.Userinfoplus;
import io.dockstore.webservice.core.Token;
import io.dockstore.webservice.core.TokenType;
import io.dockstore.webservice.core.User;
import io.dockstore.webservice.helpers.GoogleHelper;
import io.dockstore.webservice.jdbi.TokenDAO;
Expand Down Expand Up @@ -69,11 +67,10 @@ public Optional<User> authenticate(String credentials) {
.map(userinfoPlus -> {
final String email = userinfoPlus.getEmail();
User user = userDAO.findByGoogleEmail(email);
if (user != null) {
updateGoogleToken(credentials, user);
} else {
user = createUser(credentials, userinfoPlus);
if (user == null) {
user = createUser(userinfoPlus);
}
user.setTemporaryCredential(credentials);
initializeUserProfiles(user);
return Optional.of(user);
})
Expand All @@ -90,23 +87,10 @@ Optional<Userinfoplus> userinfoPlusFromToken(String credentials) {
return GoogleHelper.userinfoplusFromToken(credentials);
}

void updateGoogleToken(String credentials, User user) {
List<Token> tokens = dao.findByUserId(user.getId());
final Token googleToken = Token.extractToken(tokens, TokenType.GOOGLE_COM);
if (googleToken == null) {
dao.create(new Token(credentials, null, user.getId(), user.getUsername(), TokenType.GOOGLE_COM));
} else {
googleToken.setContent(credentials);
dao.update(googleToken);
}
}

User createUser(String credentials, Userinfoplus userinfoPlus) {
User createUser(Userinfoplus userinfoPlus) {
User user = new User();
GoogleHelper.updateUserFromGoogleUserinfoplus(userinfoPlus, user);
user.setUsername(userinfoPlus.getEmail());
final long userID = userDAO.create(user);
dao.create(new Token(credentials, null, userID, user.getUsername(), TokenType.GOOGLE_COM));
return user;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
import javax.persistence.OneToMany;
import javax.persistence.OrderBy;
import javax.persistence.Table;
import javax.persistence.Transient;
import javax.persistence.UniqueConstraint;

import com.fasterxml.jackson.annotation.JsonIgnore;
Expand Down Expand Up @@ -160,6 +161,16 @@ public class User implements Principal, Comparable<User>, Serializable {
@JsonIgnore
private Integer hostedEntryVersionsLimit;

/**
* A temporary credential to hold the access token of a request made to Dockstore with
* a token not minted by/through Dockstore. For example, if a user got an access token
* by logging into Google outside of Dockstore, then made an API call to Dockstore using that token,
* this field is used to hold the token.
*/
@Transient
@JsonIgnore
private String temporaryCredential;


public User() {
entries = new TreeSet<>();
Expand Down Expand Up @@ -386,6 +397,16 @@ public void setHostedEntryVersionsLimit(Integer hostedEntryVersionsLimit) {
this.hostedEntryVersionsLimit = hostedEntryVersionsLimit;
}

public String getTemporaryCredential() {
return temporaryCredential;
}

public void setTemporaryCredential(String temporaryCredential) {
this.temporaryCredential = temporaryCredential;
}



/**
* The profile of a user using a token (Google profile, GitHub profile, etc)
* The order of the properties are important, the UI lists these properties in this order.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ private List<AccessPolicyResponseEntry> ensureResourceExists(Workflow workflow,

@Override
public Map<Role, List<String>> workflowsSharedWithUser(User user) {
if (googleToken(user) == null) {
if (!hasGoogleToken(user)) {
return Collections.emptyMap();
}
ResourcesApi resourcesApi = getResourcesApi(user);
Expand Down Expand Up @@ -333,7 +333,7 @@ public boolean canDoAction(User user, Workflow workflow, Role.Action action) {

@Override
public void selfDestruct(User user) {
if (googleToken(user) != null) {
if (hasGoogleToken(user)) {
final ResourcesApi resourcesApi = getResourcesApi(user);
try {
final List<String> resourceIds = ownedResourceIds(resourcesApi);
Expand All @@ -352,7 +352,7 @@ public void selfDestruct(User user) {

@Override
public boolean isSharing(User user) {
if (googleToken(user) == null) {
if (!hasGoogleToken(user)) {
return false;
}
final ResourcesApi resourcesApi = getResourcesApi(user);
Expand Down Expand Up @@ -437,6 +437,9 @@ private String encodedWorkflowResource(Workflow workflow, ApiClient apiClient) {
* @return
*/
Optional<String> googleAccessToken(User user) {
if (user.getTemporaryCredential() != null) {
return Optional.of(user.getTemporaryCredential());
}
Token token = googleToken(user);
if (token != null) {
return GoogleHelper.getValidAccessToken(token).map(accessToken -> {
Expand All @@ -455,6 +458,10 @@ Token googleToken(User user) {
return Token.extractToken(tokens, TokenType.GOOGLE_COM);
}

boolean hasGoogleToken(User user) {
return user.getTemporaryCredential() != null || googleToken(user) != null;
}

/**
* Converts the response from SAM into a list of Dockstore {@link Permission}
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ public class SimpleAuthenticatorTest {
private final String credentials = "asdfafds";
private static final Long USER_ID = new Long(1);
private final Token token = Mockito.mock(Token.class);
private final User user = Mockito.mock(User.class);
private final User user = new User();
private final Userinfoplus userinfoplus = Mockito.mock(Userinfoplus.class);
private final static String USER_EMAIL = "jdoe@example.com";

Expand All @@ -35,15 +35,15 @@ public void setUp() {
userDAO = Mockito.mock(UserDAO.class);
simpleAuthenticator = spy(new SimpleAuthenticator(tokenDAO, userDAO));
doNothing().when(simpleAuthenticator).initializeUserProfiles(user);
doNothing().when(simpleAuthenticator).updateGoogleToken(credentials, user);
}

@Test
public void authenticateDockstoreToken() {
when(token.getUserId()).thenReturn(USER_ID);
when(tokenDAO.findByContent(credentials)).thenReturn(token);
when(userDAO.findById(USER_ID)).thenReturn(user);
Assert.assertEquals(user, simpleAuthenticator.authenticate(credentials).get());
final User authenticatedUser = simpleAuthenticator.authenticate(credentials).get();
Assert.assertNull(authenticatedUser.getTemporaryCredential());
}

@Test
Expand All @@ -52,7 +52,8 @@ public void authenticateGoogleTokenExistingUser() {
doReturn(Optional.of(userinfoplus)).when(simpleAuthenticator).userinfoPlusFromToken(credentials);
when(userinfoplus.getEmail()).thenReturn(USER_EMAIL);
when(userDAO.findByGoogleEmail(USER_EMAIL)).thenReturn(user);
Assert.assertEquals(user, simpleAuthenticator.authenticate(credentials).get());
final User authenticatedUser = simpleAuthenticator.authenticate(credentials).get();
Assert.assertEquals(credentials, authenticatedUser.getTemporaryCredential());
}

@Test
Expand All @@ -61,8 +62,8 @@ public void authenticateGoogleTokenNewUser() {
doReturn(Optional.of(userinfoplus)).when(simpleAuthenticator).userinfoPlusFromToken(credentials);
when(userinfoplus.getEmail()).thenReturn(USER_EMAIL);
when(userDAO.findByUsername(USER_EMAIL)).thenReturn(null);
doReturn(user).when(simpleAuthenticator).createUser(credentials, userinfoplus);
Assert.assertEquals(user, simpleAuthenticator.authenticate(credentials).get());
final User authenticatedUser = simpleAuthenticator.authenticate(credentials).get();
Assert.assertEquals(credentials, authenticatedUser.getTemporaryCredential());
}

@Test
Expand Down

0 comments on commit 3d3ed0e

Please sign in to comment.