[API] Delete Token accept names too

[6543]

if token identifer is no integer or id not exist interpret it as name

[CirnoT]

This smells like bad design, there is no way to get token ID unless it was stored manually when calling API to create it (using UI its impossible).

It would most likely be better to introduce breaking change where it expects actual token in order to delete it, otherwise this API call is nearly useless.

[6543]

@CirnoT you can get the ID from the GetTokenList - but this is not the best way to do it, I agree

[zeripath]

Could just extend it to do SHAs too?

diff --git a/routers/api/v1/user/app.go b/routers/api/v1/user/app.go
index 624beff5b..16d7fc5a1 100644
--- a/routers/api/v1/user/app.go
+++ b/routers/api/v1/user/app.go
@@ -128,15 +128,26 @@ func DeleteAccessToken(ctx *context.APIContext) {
 	//   required: true
 	// - name: token
 	//   in: path
-	//   description: token to be deleted
-	//   type: integer
-	//   format: int64
+	//   description: token to be deleted, can be token ID or the token itself
+	//   type: string
 	//   required: true
 	// responses:
 	//   "204":
 	//     "$ref": "#/responses/empty"
 
 	tokenID := ctx.ParamsInt64(":id")
+	if tokenID == 0 {
+		token, err := models.GetAccessTokenBySHA(ctx.Params(":id"))
+		if err != nil {
+			if models.IsErrAccessTokenNotExist(err) {
+				ctx.NotFound()
+			} else {
+				ctx.Error(http.StatusInternalServerError, "DeleteAccessTokenByID", err)
+			}
+			return
+		}
+		tokenID = token.ID
+	}
 	if err := models.DeleteAccessTokenByID(tokenID, ctx.User.ID); err != nil {
 		if models.IsErrAccessTokenNotExist(err) {
 			ctx.NotFound()
@@ -145,7 +156,6 @@ func DeleteAccessToken(ctx *context.APIContext) {
 		}
 		return
 	}
-
 	ctx.Status(http.StatusNoContent)
 }

[zeripath]

Actually that might not be best - we probably want to try the SHA first then if that fails drop back to ID

[6543]

since gitea >= 1.12.0 make sure token names are uniqe we could use names

[lafriks]

This would be breaking change if changed to string

[6543]

I can change the api to allow names too
(If :id match /d+ its an id -> check if exist, else check if name exist)

[6543]

@lafriks it wont

[zeripath]

This will try to use the id as a sha before it's used as number

diff --git a/routers/api/v1/user/app.go b/routers/api/v1/user/app.go
index 624beff5b..ad747c8bd 100644
--- a/routers/api/v1/user/app.go
+++ b/routers/api/v1/user/app.go
@@ -128,15 +128,22 @@ func DeleteAccessToken(ctx *context.APIContext) {
 	//   required: true
 	// - name: token
 	//   in: path
-	//   description: token to be deleted
-	//   type: integer
-	//   format: int64
+	//   description: token to be deleted, can be token ID or the token itself
+	//   type: string
 	//   required: true
 	// responses:
 	//   "204":
 	//     "$ref": "#/responses/empty"
-
 	tokenID := ctx.ParamsInt64(":id")
+	token, err := models.GetAccessTokenBySHA(ctx.Params(":id"))
+	if err != nil {
+		if !models.IsErrAccessTokenNotExist(err) {
+			ctx.Error(http.StatusInternalServerError, "DeleteAccessTokenByID", err)
+		}
+		return
+	} else token.UID ==  ctx.User.ID {
+		tokenID = token.ID
+	}
 	if err := models.DeleteAccessTokenByID(tokenID, ctx.User.ID); err != nil {
 		if models.IsErrAccessTokenNotExist(err) {
 			ctx.NotFound()
@@ -145,7 +152,6 @@ func DeleteAccessToken(ctx *context.APIContext) {
 		}
 		return
 	}
-
 	ctx.Status(http.StatusNoContent)
 }
 

[zeripath]

It won't be a breaking change @lafriks - those who want to pass integers will still work

[6543]

@zeripath i rather wiuld prever names over sha since you mostly have the name but knowing the sha if you like to delete it is not that common

@zeripath or do you have a reason to prevere using token secyit?

[zeripath]

Feel free to use name instead. I was just showing how sha could be used

[6543]

@zeripath changed a lot, can you review again?

[6543]

@CirnoT do you like it now a bit more?

[CirnoT]

What is current behavior given token with ID 1 and name "test" + token with ID 24 and name "1". If i request token "1" to be removed, which one will it be and why is it so? If there is specific priority to such cases it should be clearly documented.

[codecov-commenter]

Codecov Report

Merging #12366 into master will increase coverage by 0.01%.
The diff coverage is 51.51%.

@@            Coverage Diff             @@
##           master   #12366      +/-   ##
==========================================
+ Coverage   43.41%   43.43%   +0.01%     
==========================================
  Files         645      645              
  Lines       71306    71328      +22     
==========================================
+ Hits        30958    30978      +20     
- Misses      35334    35336       +2     
  Partials     5014     5014              

Impacted Files	Coverage Δ
routers/api/v1/user/app.go	49.59% <43.47%> (-1.39%)	⬇️
models/token.go	76.36% <62.50%> (+0.43%)	⬆️
routers/user/setting/applications.go	41.50% <100.00%> (ø)
routers/user/setting/security.go	31.14% <100.00%> (ø)
modules/log/event.go	56.60% <0.00%> (-1.89%)	⬇️
services/pull/pull.go	41.57% <0.00%> (ø)
routers/repo/view.go	37.98% <0.00%> (+0.64%)	⬆️
modules/queue/unique_queue_disk_channel.go	55.38% <0.00%> (+1.53%)	⬆️
modules/process/manager.go	75.00% <0.00%> (+2.50%)	⬆️
models/unit.go	49.31% <0.00%> (+2.73%)	⬆️
... and 2 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eb1bf23...35489e8. Read the comment docs.

[6543]

@CirnoT done

[CirnoT]

Looks OK, @lafriks how about you?

[lafriks]

🚀

[6543]

where is the automerge ;)

[lafriks]

here it is :D