From 7dfcfe019f5ae62376d9c6ad277311becd24c404 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Mon, 23 Dec 2019 16:51:57 +0100 Subject: [PATCH] Add debug logs when checking account visibility When debugging issues it is often difficult to understand why an account is / is not visible to a user. Having debugs logs for this that show up in traces can make investigations of issues easier. Signed-off-by: Edwin Kempin Change-Id: I41f20023238adc1844333d2ffd6f9c7753093e70 --- .../gerrit/server/account/AccountControl.java | 62 +++++++++++++++++-- 1 file changed, 56 insertions(+), 6 deletions(-) diff --git a/java/com/google/gerrit/server/account/AccountControl.java b/java/com/google/gerrit/server/account/AccountControl.java index f8a5c5c8a0..a6143f4b07 100644 --- a/java/com/google/gerrit/server/account/AccountControl.java +++ b/java/com/google/gerrit/server/account/AccountControl.java @@ -16,6 +16,7 @@ package com.google.gerrit.server.account; import static java.util.stream.Collectors.toSet; +import com.google.common.flogger.FluentLogger; import com.google.gerrit.common.data.PermissionRule; import com.google.gerrit.entities.Account; import com.google.gerrit.entities.AccountGroup; @@ -36,6 +37,8 @@ import java.util.Set; /** Access control management for one account's access to other accounts. */ public class AccountControl { + private static final FluentLogger logger = FluentLogger.forEnclosingClass(); + public static class Factory { private final PermissionBackend permissionBackend; private final ProjectCache projectCache; @@ -145,11 +148,19 @@ public class AccountControl { private boolean canSee(OtherUser otherUser) { if (accountVisibility == AccountVisibility.ALL) { + logger.atFine().log( + "user %s can see account %d (accountVisibility = %s)", + user.getLoggableName(), otherUser.getId().get(), AccountVisibility.ALL); return true; } else if (user.isIdentifiedUser() && user.getAccountId().equals(otherUser.getId())) { // I can always see myself. + logger.atFine().log( + "user %s can see own account %d", user.getLoggableName(), otherUser.getId().get()); return true; } else if (viewAll()) { + logger.atFine().log( + "user %s can see account %d (view all accounts = true)", + user.getLoggableName(), otherUser.getId().get()); return true; } @@ -159,14 +170,32 @@ public class AccountControl { Set usersGroups = groupsOf(otherUser.getUser()); for (PermissionRule rule : accountsSection.getSameGroupVisibility()) { if (rule.isBlock() || rule.isDeny()) { + logger.atFine().log( + "ignoring group %s of user %s for %s account visibility check" + + " because there is a blocked/denied sameGroupVisibility rule: %s", + rule.getGroup().getUUID(), + otherUser.getUser().getLoggableName(), + AccountVisibility.SAME_GROUP, + rule); usersGroups.remove(rule.getGroup().getUUID()); } } if (user.getEffectiveGroups().containsAnyOf(usersGroups)) { + logger.atFine().log( + "user %s can see account %d because they share a group (accountVisibility = %s)", + user.getLoggableName(), otherUser.getId().get(), AccountVisibility.SAME_GROUP); return true; } - break; + + logger.atFine().log( + "user %s cannot see account %d because they don't share a group" + + " (accountVisibility = %s)", + user.getLoggableName(), otherUser.getId().get(), AccountVisibility.SAME_GROUP); + logger.atFine().log("groups of user %s: %s", user.getLoggableName(), groupsOf(user)); + logger.atFine().log( + "groups of other user %s: %s", otherUser.getUser().getLoggableName(), usersGroups); + return false; } case VISIBLE_GROUP: { @@ -174,21 +203,37 @@ public class AccountControl { for (AccountGroup.UUID usersGroup : usersGroups) { try { if (groupControlFactory.controlFor(usersGroup).isVisible()) { + logger.atFine().log( + "user %s can see account %d because it is member of the visible group %s" + + " (accountVisibility = %s)", + user.getLoggableName(), + otherUser.getId().get(), + usersGroup.get(), + AccountVisibility.VISIBLE_GROUP); return true; } } catch (NoSuchGroupException e) { continue; } } - break; + + logger.atFine().log( + "user %s cannot see account %d because none of its groups are visible" + + " (accountVisibility = %s)", + user.getLoggableName(), otherUser.getId().get(), AccountVisibility.VISIBLE_GROUP); + logger.atFine().log( + "groups of other user %s: %s", otherUser.getUser().getLoggableName(), usersGroups); + return false; } case NONE: - break; + logger.atFine().log( + "user %s cannot see account %d (accountVisibility = %s)", + user.getLoggableName(), otherUser.getId().get(), AccountVisibility.NONE); + return false; case ALL: default: throw new IllegalStateException("Bad AccountVisibility " + accountVisibility); } - return false; } private boolean viewAll() { @@ -196,14 +241,19 @@ public class AccountControl { try { perm.check(GlobalPermission.VIEW_ALL_ACCOUNTS); viewAll = true; - } catch (AuthException | PermissionBackendException e) { + } catch (AuthException e) { + viewAll = false; + } catch (PermissionBackendException e) { + logger.atFine().withCause(e).log( + "Failed to check %s global capability for user %s", + GlobalPermission.VIEW_ALL_ACCOUNTS, user.getLoggableName()); viewAll = false; } } return viewAll; } - private Set groupsOf(IdentifiedUser user) { + private Set groupsOf(CurrentUser user) { return user.getEffectiveGroups().getKnownGroups().stream() .filter(a -> !SystemGroupBackend.isSystemGroup(a)) .collect(toSet());