Refactor "change file" API

modules/structs/repo_file.go

@@ -22,6 +22,23 @@ type FileOptions struct {
 	Signoff bool `json:"signoff"`
 }
 
+type FileOptionsWithSHA struct {
+	FileOptions
+	// the blob ID (SHA) for the file that already exists, it is required for changing existing files
+	// required: true
+	SHA string `json:"sha" binding:"Required"`
+}
+
+func (f *FileOptions) GetFileOptions() *FileOptions {
+	return f
+}
+
+type FileOptionsInterface interface {
+	GetFileOptions() *FileOptions
+}
+
+var _ FileOptionsInterface = (*FileOptions)(nil)
+
 // CreateFileOptions options for creating files
 // Note: `author` and `committer` are optional (if only one is given, it will be used for the other, otherwise the authenticated user will be used)
 type CreateFileOptions struct {
@@ -31,55 +48,38 @@ type CreateFileOptions struct {
 	ContentBase64 string `json:"content"`
 }
 
-// Branch returns branch name
-func (o *CreateFileOptions) Branch() string {
-	return o.FileOptions.BranchName
-}
-
 // DeleteFileOptions options for deleting files (used for other File structs below)
 // Note: `author` and `committer` are optional (if only one is given, it will be used for the other, otherwise the authenticated user will be used)
 type DeleteFileOptions struct {
-	FileOptions
-	// sha is the SHA for the file that already exists
-	// required: true
-	SHA string `json:"sha" binding:"Required"`
-}
-
-// Branch returns branch name
-func (o *DeleteFileOptions) Branch() string {
-	return o.FileOptions.BranchName
+	FileOptionsWithSHA
 }
 
 // UpdateFileOptions options for updating files
 // Note: `author` and `committer` are optional (if only one is given, it will be used for the other, otherwise the authenticated user will be used)
 type UpdateFileOptions struct {
-	DeleteFileOptions
+	FileOptionsWithSHA
 	// content must be base64 encoded
 	// required: true
 	ContentBase64 string `json:"content"`
 	// from_path (optional) is the path of the original file which will be moved/renamed to the path in the URL
 	FromPath string `json:"from_path" binding:"MaxSize(500)"`
 }
 
-// Branch returns branch name
-func (o *UpdateFileOptions) Branch() string {
-	return o.FileOptions.BranchName
-}
-
-// FIXME: ChangeFileOperation.SHA is NOT required for update or delete if last commit is provided in the options.
+// FIXME: there is no LastCommitID in FileOptions, actually it should be an alternative to the SHA in ChangeFileOperation
 
 // ChangeFileOperation for creating, updating or deleting a file
 type ChangeFileOperation struct {
-	// indicates what to do with the file
+	// indicates what to do with the file: "create" for creating a new file, "update" for updating an existing file,
+	// "upload" for creating or updating a file, "rename" for renaming a file, and "delete" for deleting an existing file.
 	// required: true
-	// enum: create,update,delete
+	// enum: create,update,upload,rename,delete
 	Operation string `json:"operation" binding:"Required"`
 	// path to the existing or new file
 	// required: true
 	Path string `json:"path" binding:"Required;MaxSize(500)"`
-	// new or updated file content, must be base64 encoded
+	// new or updated file content, it must be base64 encoded
 	ContentBase64 string `json:"content"`
-	// sha is the SHA for the file that already exists, required for update or delete
+	// the blob ID (SHA) for the file that already exists, required for changing existing files
 	SHA string `json:"sha"`
 	// old path of the file to move
 	FromPath string `json:"from_path"`
@@ -94,20 +94,10 @@ type ChangeFilesOptions struct {
 	Files []*ChangeFileOperation `json:"files" binding:"Required"`
 }
 
-// Branch returns branch name
-func (o *ChangeFilesOptions) Branch() string {
-	return o.FileOptions.BranchName
-}
-
-// FileOptionInterface provides a unified interface for the different file options
-type FileOptionInterface interface {
-	Branch() string
-}
-
 // ApplyDiffPatchFileOptions options for applying a diff patch
 // Note: `author` and `committer` are optional (if only one is given, it will be used for the other, otherwise the authenticated user will be used)
 type ApplyDiffPatchFileOptions struct {
-	DeleteFileOptions
+	FileOptions
 	// required: true
 	Content string `json:"content"`
 }

routers/api/v1/api.go

@@ -455,15 +455,6 @@ func reqRepoWriter(unitTypes ...unit.Type) func(ctx *context.APIContext) {
 	}
 }
 
-// reqRepoBranchWriter user should have a permission to write to a branch, or be a site admin
-func reqRepoBranchWriter(ctx *context.APIContext) {
-	options, ok := web.GetForm(ctx).(api.FileOptionInterface)
-	if !ok || (!ctx.Repo.CanWriteToBranch(ctx, ctx.Doer, options.Branch()) && !ctx.IsUserSiteAdmin()) {
-		ctx.APIError(http.StatusForbidden, "user should have a permission to write to this branch")
-		return
-	}
-}
-
 // reqRepoReader user should have specific read permission or be a repo admin or a site admin
 func reqRepoReader(unitType unit.Type) func(ctx *context.APIContext) {
 	return func(ctx *context.APIContext) {
@@ -744,9 +735,17 @@ func mustEnableWiki(ctx *context.APIContext) {
 	}
 }
 
+// FIXME: for consistency, maybe most mustNotBeArchived checks should be replaced with mustEnableEditor
 func mustNotBeArchived(ctx *context.APIContext) {
 	if ctx.Repo.Repository.IsArchived {
-		ctx.APIError(http.StatusLocked, fmt.Errorf("%s is archived", ctx.Repo.Repository.LogString()))
+		ctx.APIError(http.StatusLocked, fmt.Errorf("%s is archived", ctx.Repo.Repository.FullName()))
+		return
+	}
+}
+
+func mustEnableEditor(ctx *context.APIContext) {
+	if !ctx.Repo.Repository.CanEnableEditor() {
+		ctx.APIError(http.StatusLocked, fmt.Errorf("%s is not allowed to edit", ctx.Repo.Repository.FullName()))
 		return
 	}
 }
@@ -1424,24 +1423,27 @@ func Routes() *web.Router {
 					m.Get("/tags/{sha}", repo.GetAnnotatedTag)
 					m.Get("/notes/{sha}", repo.GetNote)
 				}, context.ReferencesGitRepo(true), reqRepoReader(unit.TypeCode))
-				m.Post("/diffpatch", reqRepoWriter(unit.TypeCode), reqToken(), bind(api.ApplyDiffPatchFileOptions{}), mustNotBeArchived, repo.ApplyDiffPatch)
 				m.Group("/contents", func() {
 					m.Get("", repo.GetContentsList)
 					m.Get("/*", repo.GetContents)
-					m.Post("", reqToken(), bind(api.ChangeFilesOptions{}), reqRepoBranchWriter, mustNotBeArchived, repo.ChangeFiles)
-					m.Group("/*", func() {
-						m.Post("", bind(api.CreateFileOptions{}), reqRepoBranchWriter, mustNotBeArchived, repo.CreateFile)
-						m.Put("", bind(api.UpdateFileOptions{}), reqRepoBranchWriter, mustNotBeArchived, repo.UpdateFile)
-						m.Delete("", bind(api.DeleteFileOptions{}), reqRepoBranchWriter, mustNotBeArchived, repo.DeleteFile)
-					}, reqToken())
+					m.Group("", func() {
+						// "change file" operations, need permission to write to the target branch provided by the form
+						m.Post("", bind(api.ChangeFilesOptions{}), repo.ReqChangeRepoFileOptionsAndCheck, repo.ChangeFiles)
+						m.Group("/*", func() {
+							m.Post("", bind(api.CreateFileOptions{}), repo.ReqChangeRepoFileOptionsAndCheck, repo.CreateFile)
+							m.Put("", bind(api.UpdateFileOptions{}), repo.ReqChangeRepoFileOptionsAndCheck, repo.UpdateFile)
+							m.Delete("", bind(api.DeleteFileOptions{}), repo.ReqChangeRepoFileOptionsAndCheck, repo.DeleteFile)
+						})
+						m.Post("/diffpatch", bind(api.ApplyDiffPatchFileOptions{}), repo.ReqChangeRepoFileOptionsAndCheck, repo.ApplyDiffPatch)
+					}, mustEnableEditor, reqToken())
 				}, reqRepoReader(unit.TypeCode), context.ReferencesGitRepo())
 				m.Group("/contents-ext", func() {
 					m.Get("", repo.GetContentsExt)
 					m.Get("/*", repo.GetContentsExt)
 				}, reqRepoReader(unit.TypeCode), context.ReferencesGitRepo())
 				m.Combo("/file-contents", reqRepoReader(unit.TypeCode), context.ReferencesGitRepo()).
 					Get(repo.GetFileContentsGet).
-					Post(bind(api.GetFilesOptions{}), repo.GetFileContentsPost) // POST method requires "write" permission, so we also support "GET" method above
+					Post(bind(api.GetFilesOptions{}), repo.GetFileContentsPost) // the POST method requires "write" permission, so we also support "GET" method above
 				m.Get("/signing-key.gpg", misc.SigningKeyGPG)
 				m.Get("/signing-key.pub", misc.SigningKeySSH)
 				m.Group("/topics", func() {