[milo] better ACL system for masters.

milo/buildsource/buildbot/master.go

Index: milo/buildsource/buildbot/master.go
diff --git a/milo/buildsource/buildbot/master.go b/milo/buildsource/buildbot/master.go
index d79b4ae376068addcb94db305269e20586da5d35..67f876a14f91a0d6f1ba4036e7fdd09163e2b9d3 100644
--- a/milo/buildsource/buildbot/master.go
+++ b/milo/buildsource/buildbot/master.go
@@ -36,49 +36,40 @@ func decodeMasterEntry(
 	return nil
 }
 
-// User not logged in, master found, master public: nil
-// User not logged in, master not found: 401
-// User not logged in, master internal: 401
-// User logged in, master found, master internal: nil
-// User logged in, master not found: 404
-// User logged in, master found, master internal: 404
-// Other error: 500
-func checkAccess(c context.Context, err error, internal bool) error {
+func canAccessMaster(c context.Context, name string) error {
 	cu := auth.CurrentUser(c)
-	switch {
-	case err == ds.ErrNoSuchEntity:
-		if cu.Identity == identity.AnonymousIdentity {
-			return errNotAuth
+	if cu.Identity != identity.AnonymousIdentity {

[Ryan Tseng, 2017/07/11 18:37:16]

This should go after L51, otherwise a logged in user (eg
happyperson@chromium.org) would lose access to public masters after logging in.

[iannucci, 2017/07/11 21:01:39]

chatted, we think this logic is actually correct (non-internal logged in users
should fall through to the anonymous access check below)

+		// if we're logged in, and we can internal stuff, return nil. getMasterEntry

[dnj, 2017/07/11 18:46:33]

nit: "we can see internal stuff"

[iannucci, 2017/07/11 21:01:39]

Done.

+		// will maybe return 404 later.
+		if allowed, err := common.IsAllowedInternal(c); err != nil || allowed {
+			return err
 		}
-		return errMasterNotFound
-	case err != nil:
-		return err
 	}
 
-	// Do the ACL check if the entry is internal.
-	if internal {
-		allowed, err := common.IsAllowedInternal(c)
-		if err != nil {
-			return err
-		}
-		if !allowed {
-			if cu.Identity == identity.AnonymousIdentity {
-				return errNotAuth
-			}
-			return errMasterNotFound
-		}
+	// We're not logged in, or we can only see public stuff, so see if the master
+	// is public.
+	if err := ds.Get(c, &buildbotMasterPublic{name}); err == nil {
+		// it exists and is public
+		return nil
 	}
 
-	return nil
+	// They need to log in before we can tell them more stuff.
+	return errNotAuth
 }
 
 // getMasterEntry feches the named master and does an ACL check on the
 // current user.
 // It returns:
 func getMasterEntry(c context.Context, name string) (*buildbotMasterEntry, error) {
+	if err := canAccessMaster(c, name); err != nil {
+		return nil, err
+	}
+
 	entry := buildbotMasterEntry{Name: name}
 	err := ds.Get(c, &entry)
-	err = checkAccess(c, err, entry.Internal)
+	if err == ds.ErrNoSuchEntity {
+		err = errMasterNotFound
+	}
 	return &entry, err
 }