cgivre commented on code in PR #3094:
URL: https://github.com/apache/drill/pull/3094#discussion_r4371907317
##########
contrib/storage-googlesheets/src/main/java/org/apache/drill/exec/store/googlesheets/GoogleSheetsStoragePlugin.java:
##########
@@ -201,26 +254,17 @@ public boolean supportsInsert() {
* @return An authenticated {@link Sheets} Google Sheets service.
*/
public Sheets getSheetsService(String queryUser) {
- if (service != null && dataStore != null) {
- return service;
- } else {
- // Check if datastore is null and initialize if so.
- if (dataStore == null) {
- this.dataStore = getDataStore(queryUser);
- }
-
- try {
- if (config.getAuthMode() == AuthMode.USER_TRANSLATION) {
- service = GoogleSheetsUtils.getSheetsService(config, dataStore,
queryUser);
- } else {
- service = GoogleSheetsUtils.getSheetsService(config, dataStore,
SHARED_USERNAME);
- }
- return service;
- } catch (IOException | GeneralSecurityException e) {
- throw UserException.connectionError(e)
- .message("Error connecting to Googlesheets Service: " +
e.getMessage())
- .build(logger);
- }
+ return sheetsServices.computeIfAbsent(cacheKey(queryUser), k ->
buildSheetsService(queryUser));
Review Comment:
Good catch, fixed in e7ff67cf3. The per-identity data store and the
Sheets/Drive services are now tagged with the access token they were built
from: nothing is cached while that identity has no access token, and an entry
is dropped as soon as the stored token differs from its tag. So "query, auth
error, authorize, query again" works without a restart, and so does
re-authorizing after a revoke. A token refresh rewrites the stored token too,
which costs one extra rebuild.
I went with "do not cache without tokens" rather than having OAuthRequests
evict, since that would mean a callback from java-exec into the contrib
plugins. Written up in docs/dev/OAuthTokenStorage.md, since you are right that
nothing in-tree would catch it.
##########
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/OAuthRequests.java:
##########
@@ -189,18 +201,63 @@ private static JsonResult message(String message,
Object... args) {
}
/**
- * This function checks to see if a given storage plugin is using
USER_TRANSLATION mode and if user
- * authentication is enabled. If so, it will return the active user name.
If not it will return null.
+ * Resolves which token table the caller may write to, and refuses the write
if
+ * they may not.
+ *
+ * <p>A {@code USER_TRANSLATION} plugin gives each user their own token
table,
+ * so the caller gets their own name and can only ever touch their own
tokens.
+ * Every other auth mode shares one token table across the whole cluster, and
+ * those tokens are admin-owned configuration: overwriting them changes the
+ * identity that <em>all</em> users' queries run under at the data source.
The
+ * credential endpoints that reach this code are open to any authenticated
+ * user, so the shared table has to be gated on the admin role here rather
than
+ * left to the resource class.
+ *
+ * <p>{@link DrillUserPrincipal} is used rather than the {@code
SecurityContext}
+ * because it is bound to the anonymous admin principal when authentication
is
+ * disabled, where the security context carries no principal at all.
+ *
* @param config {@link StoragePluginConfig} The current plugin configuration
- * @return If USER_TRANSLATION is enabled, returns the active user. If not,
returns null.
+ * @param principal the caller, never null in practice
+ * @return the caller's user name for a user translation plugin, or null to
+ * select the shared token table
+ * @throws TokenAccessDeniedException if the caller may not write these
tokens
*/
- private static String getQueryUser(StoragePluginConfig config,
- UserAuthEnabled authEnabled,
- SecurityContext sc) {
- if (config.getAuthMode() == AuthMode.USER_TRANSLATION &&
authEnabled.get()) {
- return sc.getUserPrincipal().getName();
- } else {
- return null;
+ private static String getQueryUser(StoragePluginConfig config,
DrillUserPrincipal principal) {
+ if (principal == null) {
+ // Only reachable with authentication on but no authenticated principal;
+ // with it off the anonymous principal is always supplied.
+ throw new TokenAccessDeniedException("Could not determine the current
user.");
+ }
+
+ if (config.getAuthMode() == AuthMode.USER_TRANSLATION) {
+ // The name must be the one queries run under, or tokens would be written
+ // to a table the query path never reads. With authentication disabled
that
+ // is the anonymous user, which is also what the query session is given.
+ return principal.getName();
Review Comment:
Documented rather than changed, because USER_TRANSLATION with authentication
off is not a supported combination in the first place: translating users means
giving each one their own credentials, which needs user names that are unique
and trusted, and with auth off the name is anonymous on web/REST and whatever
the client sends everywhere else, with every caller an admin. Keeping the
shared registry in that mode would also mean teaching the query path in all
three plugins to read the auth flag, to prop up a configuration that cannot
work.
So docs/dev/OAuthTokenStorage.md now has a "User translation needs
authentication" section saying exactly that, and pointing at SHARED_USER or
turning authentication on.
##########
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/StorageResources.java:
##########
@@ -422,9 +431,15 @@ public Response deletePluginViaGet(@PathParam("name")
String name) {
* @param config {@link StoragePluginConfig} The current plugin configuration
* @return If USER_TRANSLATION is enabled, returns the query user. If not,
returns null.
*/
+ /**
+ * The owner of the token table belonging to this plugin: the caller for a
user
Review Comment:
Removed, thanks.
##########
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/CredentialResources.java:
##########
@@ -146,7 +183,14 @@ public List<PluginConfigWrapper>
getConfigsFor(@PathParam("group") String plugin
public Response createOrUpdateCredentials(@FormParam("plugin") String
pluginName,
@FormParam("username") String username,
@FormParam("password") String password)
{
- String queryUser = sc.getUserPrincipal().getName();
+ String queryUser = queryUser();
Review Comment:
Fixed both, thanks.
##########
exec/java-exec/src/main/java/org/apache/drill/exec/oauth/OAuthTokenProvider.java:
##########
@@ -21,44 +21,70 @@
import org.apache.drill.common.AutoCloseables;
import org.apache.drill.exec.server.DrillbitContext;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+
/**
* Class for managing oauth tokens. Storage plugins will have to manage
obtaining the tokens, but
* these classes handle the storage of access and refresh tokens.
+ *
+ * <p>Tokens are held in one registry per identity: a registry for each user
of a
+ * {@code USER_TRANSLATION} plugin, plus one shared registry for plugins that
+ * authenticate as a single service account. Each maps to its own path in the
+ * persistent store, so a registry must be looked up by the identity whose
tokens
+ * are wanted and never cached across identities.
*/
public class OAuthTokenProvider implements AutoCloseable {
private static final String STORAGE_REGISTRY_PATH = "oauth_tokens";
+ /**
+ * Map key standing in for the shared registry, which has no user. The empty
+ * string cannot collide with a user name because a blank name is treated as
+ * absent.
+ */
+ private static final String SHARED_REGISTRY_KEY = "";
+
private final DrillbitContext context;
- private PersistentTokenRegistry oauthTokenRegistry;
+ private final Map<String, PersistentTokenRegistry> oauthTokenRegistries =
new ConcurrentHashMap<>();
Review Comment:
Added a "Known limitation: per-identity caches are unbounded" section in
docs/dev/OAuthTokenStorage.md, next to the orphaned-token-table one. It names
both caches, notes the entries are small and that a deployment with very many
distinct users wants a bounded or expiring cache, and that nothing in the
design depends on an entry living forever since both are rebuilt on demand.
##########
exec/java-exec/src/main/java/org/apache/drill/exec/oauth/OAuthTokenProvider.java:
##########
@@ -21,44 +21,70 @@
import org.apache.drill.common.AutoCloseables;
import org.apache.drill.exec.server.DrillbitContext;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+
/**
* Class for managing oauth tokens. Storage plugins will have to manage
obtaining the tokens, but
* these classes handle the storage of access and refresh tokens.
+ *
+ * <p>Tokens are held in one registry per identity: a registry for each user
of a
+ * {@code USER_TRANSLATION} plugin, plus one shared registry for plugins that
+ * authenticate as a single service account. Each maps to its own path in the
+ * persistent store, so a registry must be looked up by the identity whose
tokens
+ * are wanted and never cached across identities.
*/
public class OAuthTokenProvider implements AutoCloseable {
private static final String STORAGE_REGISTRY_PATH = "oauth_tokens";
+ /**
+ * Map key standing in for the shared registry, which has no user. The empty
+ * string cannot collide with a user name because a blank name is treated as
+ * absent.
+ */
+ private static final String SHARED_REGISTRY_KEY = "";
+
private final DrillbitContext context;
- private PersistentTokenRegistry oauthTokenRegistry;
+ private final Map<String, PersistentTokenRegistry> oauthTokenRegistries =
new ConcurrentHashMap<>();
public OAuthTokenProvider(DrillbitContext context) {
this.context = context;
}
+ /**
+ * Returns the token registry holding {@code username}'s tokens, creating it
on
+ * first use. A null or empty user name selects the shared registry used by
+ * plugins that do not translate users.
+ *
+ * <p>The registry is resolved per call: returning a registry belonging to
+ * another user would let one user's queries run with another user's tokens,
+ * which is precisely what user translation exists to prevent.
+ */
public TokenRegistry getOauthTokenRegistry(String username) {
- if (oauthTokenRegistry == null) {
- initRemoteRegistries(username);
- }
- return oauthTokenRegistry;
- }
+ String key = StringUtils.isNotEmpty(username) ? username :
SHARED_REGISTRY_KEY;
- private synchronized void initRemoteRegistries(String username) {
- // Add the username to the path if present
- String finalpath;
- if (StringUtils.isNotEmpty(username)) {
- finalpath = STORAGE_REGISTRY_PATH + "/" + username;
- } else {
- finalpath = STORAGE_REGISTRY_PATH;
- }
+ // computeIfAbsent so that concurrent callers for the same identity share
one
+ // registry, and so a failure to build one is not cached.
+ return oauthTokenRegistries.computeIfAbsent(
+ key,
+ k -> new PersistentTokenRegistry(context, registryPath(username))
+ );
+ }
- if (oauthTokenRegistry == null) {
- oauthTokenRegistry = new PersistentTokenRegistry(context, finalpath);
- }
+ /**
+ * The persistent store path for a user's tokens, or the root path for the
+ * shared registry.
+ */
+ private static String registryPath(String username) {
+ return StringUtils.isNotEmpty(username)
+ ? STORAGE_REGISTRY_PATH + "/" + username
Review Comment:
The user name is now percent-encoded before it becomes a path segment
(URLEncoder plus "." to %2E, which URLEncoder leaves alone), so a full Kerberos
principal or a name containing ".." stays inside its own segment. Encoding
rather than rejecting, so a Kerberos deployment is not locked out of a plugin
by its own principal names.
Existing per-user paths move as a result, which the upgrade note already
covers: those users re-authorize once.
OAuthTokenProviderTest#testUserNamesCannotEscapeTheirOwnPath guards it.
Being straight about it: it is a regression guard, not a reproduction, as I
could not get the unencoded path to actually collide under the test persistent
store.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
