[milo] better ACL system for masters.

milo/buildsource/buildbot/master.go

 // Copyright 2016 The LUCI Authors. All rights reserved.
 // Use of this source code is governed under the Apache License, Version 2.0
 // that can be found in the LICENSE file.
 
 package buildbot
 
 import (
         "bytes"
         "compress/gzip"
         "encoding/json"
@@ skipping 18 matching lines @@
         if err != nil {
                 return err
         }
         defer reader.Close()
         if err = json.NewDecoder(reader).Decode(master); err != nil {
                 return err
         }
         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
-                }
-                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 {
+        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
                 }
-                if !allowed {
-                        if cu.Identity == identity.AnonymousIdentity {
-                                return errNotAuth
-                        }
-                        return errMasterNotFound
-                }
         }
 
-        return nil
+        // 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
+        }
+
+        // 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
 }
 
 // getMasterJSON fetches the latest known buildbot master data and returns
 // the buildbotMaster struct (if found), whether or not it is internal,
 // the last modified time, and an error if not found.
 func getMasterJSON(c context.Context, name string) (
         master *buildbotMaster, internal bool, t time.Time, err error) {
         master = &buildbotMaster{}
         entry, err := getMasterEntry(c, name)
@@ skipping 50 matching lines @@
                 for _, bn := range sb {
                         // Go templates escapes this for us, and also
                         // slashes are not allowed in builder names.
                         ml.Builders = append(ml.Builders, *resp.NewLink(
                                 bn, fmt.Sprintf("/buildbot/%s/%s", entry.Name, bn)))
                 }
                 result.BuilderGroups = append(result.BuilderGroups, ml)
         }
         return result, nil
 }