LogDog: Project READ access for user endpoints.

appengine/logdog/coordinator/project.go

Index: appengine/logdog/coordinator/project.go
diff --git a/appengine/logdog/coordinator/project.go b/appengine/logdog/coordinator/project.go
index 0553620a5bfe27caa9a059bcde0c247b9fd94e46..78b358eddc00ba6149cd35015a048337b501adb7 100644
--- a/appengine/logdog/coordinator/project.go
+++ b/appengine/logdog/coordinator/project.go
@@ -7,20 +7,27 @@ package coordinator
 import (
 	"strings"
 
-	"github.com/luci/gae/service/datastore/meta"
 	"github.com/luci/gae/service/info"
-	"github.com/luci/luci-go/common/config"
+	"github.com/luci/luci-go/appengine/logdog/coordinator/config"
+	luciConfig "github.com/luci/luci-go/common/config"
+	log "github.com/luci/luci-go/common/logging"
 	"github.com/luci/luci-go/common/proto/logdog/svcconfig"
 	"golang.org/x/net/context"
 )
 
-// projectNamespacePrefix is the datastore namespace prefix for project
-// namespaces.
-const projectNamespacePrefix = "luci."
+const (
+	// projectNamespacePrefix is the datastore namespace prefix for project
+	// namespaces.
+	projectNamespacePrefix = "luci."
+
+	// projectConfigWorkers is the number of workers that will pull project
+	// configs from the config service.
+	projectConfigWorkers = 16
+)
 
 // ProjectNamespace returns the AppEngine namespace for a given luci-config
 // project name.
-func ProjectNamespace(project config.ProjectName) string {
+func ProjectNamespace(project luciConfig.ProjectName) string {
 	return projectNamespacePrefix + string(project)
 }
 
@@ -29,11 +36,11 @@ func ProjectNamespace(project config.ProjectName) string {
 //
 // If the namespace does not have a project namespace prefix, this function
 // will return an empty string.
-func ProjectFromNamespace(ns string) config.ProjectName {
+func ProjectFromNamespace(ns string) luciConfig.ProjectName {
 	if !strings.HasPrefix(ns, projectNamespacePrefix) {
 		return ""
 	}
-	return config.ProjectName(ns[len(projectNamespacePrefix):])
+	return luciConfig.ProjectName(ns[len(projectNamespacePrefix):])
 }
 
 // CurrentProject returns the current project based on the currently-loaded
@@ -41,7 +48,7 @@ func ProjectFromNamespace(ns string) config.ProjectName {
 //
 // If there is no current namespace, or if the current namespace is not a valid
 // project namespace, an empty string will be returned.
-func CurrentProject(c context.Context) config.ProjectName {
+func CurrentProject(c context.Context) luciConfig.ProjectName {
 	if ns, ok := info.Get(c).GetNamespace(); ok {
 		return ProjectFromNamespace(ns)
 	}
@@ -57,18 +64,31 @@ func CurrentProjectConfig(c context.Context) (*svcconfig.ProjectConfig, error) {
 	return GetServices(c).ProjectConfig(c, CurrentProject(c))
 }
 
-// AllProjectsWithNamespaces scans current namespaces and returns those that
-// belong to LUCI projects.
-func AllProjectsWithNamespaces(c context.Context) ([]config.ProjectName, error) {
-	var projects []config.ProjectName
-	err := meta.NamespacesWithPrefix(c, projectNamespacePrefix, func(ns string) error {
-		if proj := ProjectFromNamespace(ns); proj != "" {
-			projects = append(projects, proj)
-		}
-		return nil
-	})
+// ActiveUserProjects returns a full list of all config service projects with
+// a LogDog project configurations that the current user has READ access to.

[nodir, 2016/05/19 17:17:21]

s/a //

[dnj (Google), 2016/05/19 20:10:46]

Done.

+func ActiveUserProjects(c context.Context) (map[luciConfig.ProjectName]*svcconfig.ProjectConfig, error) {
+	allPcfgs, err := config.AllProjectConfigs(c)

[nodir, 2016/05/19 17:17:21]

I'd strongly recommend calling this function in a cron job and persisting
results to datastore; and then read from datastore here.

Then you can update your other functions for config loading to load from
datastore.
I am sorry I writing this only now. We should probably write a package that does
this sort of replication because all apps need it. If you want to do that, I can
review the code

[dnj (Google), 2016/05/19 20:10:46]

Agreed, and I'll put a TODO in, but this is not a performance bottleneck ATM.
This is an infrequently-called function at best, and memcached either way.

 	if err != nil {
 		return nil, err
 	}
-	return projects, nil
+
+	for project, pcfg := range allPcfgs {

[nodir, 2016/05/19 17:17:21]

add a TODO to make acl check concurrent. Auth groups can be large, so checks may
take time and we may have a lot of projects

[dnj (Google), 2016/05/19 20:10:46]

If this is actually a problem, I think we need to optimize this lookup. It is
the sort of thing that needs to happen fast.

+		// Verify user access. This includes loading the project's config.

[nodir, 2016/05/19 17:17:21]

it does not include that

[dnj (Google), 2016/05/19 20:10:46]

Done.

+		if err := IsProjectReader(c, pcfg); err != nil {
+			delete(allPcfgs, project)

[nodir, 2016/05/19 17:17:21]

this makes me nervous. I'd rather create a separate map with restricted set and
add projects there explicitly. Anyway you will probably do the explicit addition
if you make this code parallel because you will send accessible projects to a
channel

[dnj (Google), 2016/05/19 20:10:46]

Agree about parallel construction.

I thought about this in response to your comment, but I think this is fine.
Delete is a reliable Go operation, and this logic is really simple: if there is
an error, remove the thing from the map. I am comfortable relying on this. Any
reconstruction around building another map will use the same checks and have
equivalent code paths.

+
+			// If it is a membership error, prune this project and continue.
+			// Otherwise, forward the error.
+			if !IsMembershipError(err) {
+				// No configuration for this project, the configuration is invalid, or
+				// the user didn't have access. Remove it from the list.
+				log.Fields{
+					log.ErrorKey: err,
+					"project":    project,
+				}.Errorf(c, "Failed to check project.")
+				return nil, err
+			}
+		}
+	}
+	return allPcfgs, nil
 }