Improve slow user authorization if many orgs/repos (#5665)

Co-authored-by: GuillaumeMeurillon <MeurillonGuillaume@users.noreply.github.com>

cmd/server/flags.go

@@ -100,6 +100,11 @@ var flags = append([]cli.Flag{
 		Name:    "custom-js-file",
 		Usage:   "file path for the server to serve a custom .JS file, used for customizing the UI",
 	},
+	&cli.BoolFlag{
+		Sources: cli.EnvVars("WOODPECKER_ASYNC_REPOSITORY_UPDATE"),
+		Name:    "async-repository-update",
+		Usage:   "if true fetch repository permissions asynchronous, impacts performance if there are many repositories with possible tradeoff in consistency",
+	},
 	&cli.StringFlag{
 		Sources: cli.EnvVars("WOODPECKER_GRPC_ADDR"),
 		Name:    "grpc-addr",

cmd/server/setup.go

@@ -181,6 +181,7 @@ func setupEvilGlobals(ctx context.Context, c *cli.Command, s store.Store) (err e
 
 	// authentication
 	server.Config.Pipeline.AuthenticatePublicRepos = c.Bool("authenticate-public-repos")
+	server.Config.Server.AsyncRepositoryUpdate = c.Bool("async-repository-update")
 
 	// Pull requests
 	server.Config.Pipeline.DefaultAllowPullRequests = c.Bool("default-allow-pull-requests")

docs/docs/30-administration/10-configuration/10-server.md

@@ -708,6 +708,19 @@ Always use authentication to clone repositories even if they are public. Needed
 
 ---
 
+### ASYNC_REPOSITORY_UPDATE
+
+- Name: `WOODPECKER_ASYNC_REPOSITORY_UPDATE`
+- Default: `false`
+
+Enable asynchronous fetching user permissions for repositories. Will drastically improve login speed for user login if the organisation has many git repositories.
+
+When disabled (default) users will have to wait for all repository access information before being redirected to the Woodpecker homepage. Choose this for strong consistency.
+
+When enabled users will immediately be redirected to the Woodpecker homepage, but might see outdated information if repository access changed or new repositories were added. Choose this for eventual consistency.
+
+---
+
 ### DEFAULT_ALLOW_PULL_REQUESTS
 
 - Name: `WOODPECKER_DEFAULT_ALLOW_PULL_REQUESTS`

server/api/login.go

@@ -293,19 +293,37 @@ func HandleAuth(c *gin.Context) {
 		return
 	}
 
-	err = updateRepoPermissions(c, user, _store, _forge, forgeID)
-	if err != nil {
-		log.Error().Err(err).Msgf("cannot update repo permissions for user %s", user.Login)
+	var noStoredRepositories bool
+	if repos, err := _store.RepoList(user, false, false, &model.RepoFilter{}); err != nil {
+		log.Error().Err(err).Msgf("Could not list stored repositories for user %s", user.Login)
 		c.Redirect(http.StatusSeeOther, server.Config.Server.RootPath+"/login?error=internal_error")
 		return
+	} else {
+		noStoredRepositories = len(repos) == 0
+	}
+
+	if !server.Config.Server.AsyncRepositoryUpdate || noStoredRepositories {
+		if err := updateRepoPermissions(c, user, _store, _forge, forgeID); err != nil {
+			if err != nil {
+				log.Error().Err(err).Msgf("cannot update repo permissions for user %s", user.Login)
+			}
+			c.Redirect(http.StatusSeeOther, server.Config.Server.RootPath+"/login?error=internal_error")
+			return
+		}
+	} else {
+		go func() {
+			if err := updateRepoPermissions(c, user, _store, _forge, forgeID); err != nil {
+				log.Error().Err(err).Msgf("could not update repo permissions for user %s in background", user.Login)
+			}
+		}()
 	}
 
 	httputil.SetCookie(c.Writer, c.Request, "user_sess", tokenString)
-
 	c.Redirect(http.StatusSeeOther, server.Config.Server.RootPath+"/")
 }
 
 func updateRepoPermissions(c *gin.Context, user *model.User, _store store.Store, _forge forge.Forge, forgeID int64) error {
+	start := time.Now()
 	repos, err := utils.Paginate(func(page int) ([]*model.Repo, error) {
 		return _forge.Repos(c, user, &model.ListOptions{
 			Page:    page,
@@ -350,6 +368,7 @@ func updateRepoPermissions(c *gin.Context, user *model.User, _store store.Store,
 		return err
 	}
 
+	log.Debug().Msgf("update repo permissions for user %s in %dms", user.Login, time.Since(start).Milliseconds())
 	return nil
 }
 

server/api/login_test.go

@@ -179,6 +179,7 @@ func TestHandleAuth(t *testing.T) {
 		_store.On("OrgCreate", mock.Anything).Return(nil)
 		_store.On("UpdateUser", mock.Anything).Return(nil)
 		_store.On("PermPrune", mock.Anything, []int64(nil)).Return(nil)
+		_store.On("RepoList", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil, nil)
 		_forge.On("Repos", mock.Anything, mock.Anything, mock.Anything).Return(nil, nil)
 
 		api.HandleAuth(c)
@@ -212,6 +213,7 @@ func TestHandleAuth(t *testing.T) {
 		_store.On("OrgGet", org.ID).Return(org, nil)
 		_store.On("UpdateUser", mock.Anything).Return(nil)
 		_store.On("PermPrune", mock.Anything, []int64(nil)).Return(nil)
+		_store.On("RepoList", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil, nil)
 		_forge.On("Repos", mock.Anything, mock.Anything, mock.Anything).Return(nil, nil)
 
 		api.HandleAuth(c)
@@ -308,6 +310,7 @@ func TestHandleAuth(t *testing.T) {
 		_store.On("OrgCreate", mock.Anything).Return(nil)
 		_store.On("UpdateUser", mock.Anything).Return(nil)
 		_store.On("PermPrune", mock.Anything, []int64(nil)).Return(nil)
+		_store.On("RepoList", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil, nil)
 		_forge.On("Repos", mock.Anything, mock.Anything, mock.Anything).Return(nil, nil)
 
 		api.HandleAuth(c)
@@ -343,6 +346,7 @@ func TestHandleAuth(t *testing.T) {
 		_store.On("OrgUpdate", mock.Anything).Return(nil)
 		_store.On("UpdateUser", mock.Anything).Return(nil)
 		_store.On("PermPrune", mock.Anything, []int64(nil)).Return(nil)
+		_store.On("RepoList", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil, nil)
 		_forge.On("Repos", mock.Anything, mock.Anything, mock.Anything).Return(nil, nil)
 
 		api.HandleAuth(c)
@@ -378,6 +382,7 @@ func TestHandleAuth(t *testing.T) {
 		_store.On("OrgUpdate", mock.Anything).Return(nil)
 		_store.On("UpdateUser", mock.Anything).Return(nil)
 		_store.On("PermPrune", mock.Anything, []int64(nil)).Return(nil)
+		_store.On("RepoList", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil, nil)
 		_forge.On("Repos", mock.Anything, mock.Anything, mock.Anything).Return(nil, nil)
 
 		api.HandleAuth(c)

server/api/user.go

@@ -213,6 +213,25 @@ func GetRepos(c *gin.Context) {
 	c.JSON(http.StatusOK, repos)
 }
 
+func RefreshRepos(c *gin.Context) {
+	_store := store.FromContext(c)
+	user := session.User(c)
+
+	_forge, err := server.Config.Services.Manager.ForgeFromUser(user)
+	if err != nil {
+		log.Error().Err(err).Msg("Cannot get forge from user")
+		c.AbortWithStatus(http.StatusInternalServerError)
+		return
+	}
+
+	if err := updateRepoPermissions(c, user, _store, _forge, user.ForgeID); err != nil {
+		log.Error().Err(err).Msgf("Can't update repo permissions for user %s in forge %s", user.Login, _forge.Name())
+		c.AbortWithStatus(http.StatusInternalServerError)
+		return
+	}
+	c.JSON(http.StatusOK, "Ok")
+}
+
 // PostToken
 //
 //	@Summary	Return the token of the current user as string

server/config.go

@@ -36,21 +36,22 @@ var Config = struct {
 		LogStore   log.Service
 	}
 	Server struct {
-		JWTSecret           string
-		Key                 string
-		Cert                string
-		OAuthHost           string
-		Host                string
-		WebhookHost         string
-		Port                string
-		PortTLS             string
-		AgentToken          string
-		StatusContext       string
-		StatusContextFormat string
-		SessionExpires      time.Duration
-		RootPath            string
-		CustomCSSFile       string
-		CustomJsFile        string
+		JWTSecret             string
+		Key                   string
+		Cert                  string
+		OAuthHost             string
+		Host                  string
+		WebhookHost           string
+		Port                  string
+		PortTLS               string
+		AgentToken            string
+		StatusContext         string
+		StatusContextFormat   string
+		SessionExpires        time.Duration
+		RootPath              string
+		CustomCSSFile         string
+		CustomJsFile          string
+		AsyncRepositoryUpdate bool
 	}
 	Agent struct {
 		DisableUserRegisteredAgentRegistration bool

server/router/api.go

@@ -32,9 +32,13 @@ func apiRoutes(e *gin.RouterGroup) {
 			user.Use(session.MustUser())
 			user.GET("", api.GetSelf)
 			user.GET("/feed", api.GetFeed)
-			user.GET("/repos", api.GetRepos)
 			user.POST("/token", api.PostToken)
 			user.DELETE("/token", api.DeleteToken)
+			repoUserBase := user.Group("/repos")
+			{
+				repoUserBase.GET("", api.GetRepos)
+				repoUserBase.POST("/refresh", api.RefreshRepos)
+			}
 		}
 
 		users := apiBase.Group("/users")

web/src/assets/locales/en.json

@@ -68,6 +68,7 @@
     "branches": "Branches",
     "pull_requests": "Pull requests",
     "add": "Add repository",
+    "refresh": "Refresh repository list",
     "user_none": "This organization/user has no projects yet",
     "not_allowed": "You are not allowed to access this repository",
     "enable": {

web/src/assets/locales/fr.json

@@ -191,6 +191,7 @@
     "repo": {
         "activity": "Activité",
         "add": "Ajouter un dépôt",
+        "refresh": "Actualiser la liste dépots",
         "branches": "Branches",
         "deploy_pipeline": {
             "enter_target": "Environnement de 'déploiement' ciblé",

web/src/assets/locales/nl.json

@@ -18,6 +18,7 @@
     "repo": {
         "activity": "Activiteit",
         "add": "Repository toevoegen",
+        "refresh": "Vernieuw repository lijst",
         "branches": "Branches",
         "deploy_pipeline": {
             "enter_target": "Doelomgeving deployment",

web/src/components/atomic/Icon.vue

@@ -58,6 +58,7 @@
   <SvgIcon v-else-if="name === 'alert'" :bg-circle="bgCircle" :path="mdiAlertCircle" size="1.3rem" />
   <SvgIcon v-else-if="name === 'question'" :bg-circle="bgCircle" :path="mdiHelpCircle" size="1.3rem" />
   <SvgIcon v-else-if="name === 'plus'" :bg-circle="bgCircle" :path="mdiPlus" size="1.3rem" />
+  <SvgIcon v-else-if="name === 'refresh'" :bg-circle="bgCircle" :path="mdiRefresh" size="1.3rem" />
   <SvgIcon v-else-if="name === 'list'" :bg-circle="bgCircle" :path="mdiFormatListBulleted" size="1.3rem" />
   <SvgIcon v-else-if="name === 'heal'" :bg-circle="bgCircle" :path="mdiWrenchCogOutline" size="1.3rem" />
   <SvgIcon v-else-if="name === 'turn-off'" :bg-circle="bgCircle" :path="mdiPower" size="1.3rem" />
@@ -178,6 +179,7 @@ import {
   mdiPuzzleOutline,
   mdiRadioboxBlank,
   mdiRadioboxIndeterminateVariant,
+  mdiRefresh,
   mdiShieldKeyOutline,
   mdiSourceBranch,
   mdiSourceCommit,
@@ -234,6 +236,7 @@ export type IconNames =
   | 'question'
   | 'list'
   | 'plus'
+  | 'refresh'
   | 'blank'
   | 'heal'
   | 'chevron-right'

web/src/lib/api/index.ts

@@ -50,6 +50,10 @@ export default class WoodpeckerClient extends ApiClient {
     return this._get(`/api/user/repos?${query}`) as Promise<Repo[]>;
   }
 
+  async refreshRepoList(): Promise<unknown> {
+    return this._post(`/api/user/repos/refresh`);
+  }
+
   async lookupRepo(owner: string, name: string): Promise<Repo | undefined> {
     return this._get(`/api/repos/lookup/${owner}/${name}`) as Promise<Repo | undefined>;
   }

web/src/store/repos.ts

@@ -67,6 +67,10 @@ export const useRepoStore = defineStore('repos', () => {
     }
   }
 
+  async function refreshRepos() {
+    await apiClient.refreshRepoList();
+  }
+
   return {
     repos,
     ownedRepos,
@@ -75,5 +79,6 @@ export const useRepoStore = defineStore('repos', () => {
     setRepo,
     loadRepo,
     loadRepos,
+    refreshRepos,
   };
 });

web/src/views/Repos.vue

@@ -6,6 +6,7 @@
 
     <template #headerActions>
       <Button :to="{ name: 'repo-add' }" start-icon="plus" :text="$t('repo.add')" />
+      <Button start-icon="refresh" :is-loading="isRefreshing" :text="$t('repo.refresh')" @click="refreshRepositories" />
     </template>
 
     <Transition name="fade" mode="out-in">
@@ -47,6 +48,7 @@ import { useI18n } from 'vue-i18n';
 import Button from '~/components/atomic/Button.vue';
 import Scaffold from '~/components/layout/scaffold/Scaffold.vue';
 import RepoItem from '~/components/repo/RepoItem.vue';
+import { useAsyncAction } from '~/compositions/useAsyncAction';
 import useRepos from '~/compositions/useRepos';
 import { useRepoSearch } from '~/compositions/useRepoSearch';
 import { useWPTitle } from '~/compositions/useWPTitle';
@@ -63,6 +65,11 @@ const search = ref('');
 const { searchedRepos } = useRepoSearch(repos, search);
 const reposLastActivity = computed(() => sortReposByLastActivity(searchedRepos.value || []));
 
+const { doSubmit: refreshRepositories, isLoading: isRefreshing } = useAsyncAction(async () => {
+  await repoStore.refreshRepos();
+  await repoStore.loadRepos();
+});
+
 onMounted(async () => {
   await repoStore.loadRepos();
 });