I think we should not remove the indexes. They can still be valuable.

I thought you would simply use `models/DeleteUser#72-91`.

I thought you would simply use `models.DeleteRepository#130-152` for it…

I think I've also mentioned it above, but here it is again: I think it would be a good idea to strip leading and trailing whitespace from secrets. There is only a marginal benefit in not doing it (who has a secret that starts or ends with whitespace?). However, there is a huge benefit for usability, since many copying programs add for example whitespace at the end, meaning you would get an invalid secret that you first have to debug why it isn't working.

Do we copyright the year the code was written, or the year it is merged? I would have thought it is the year it is merged…

There is a name mismatch here: `InsertOrgSecret` and `FindUserSecrets`. I would rename both to `Owner`. Maybe change the structs member too.

Should update to `.locale.Tr`.

I think that can be deleted as it doesn't really look good.

Something doesn't add up with the `for`-parts of the labels here: Since when do they accept `name`? Don't they only accept `id`?

That looks hacky. Is there no better approach for this? Especially: Why are there two different types of hyphens?

Should we also store `UpdatedUnix`? Or should we perhaps **only** store `UpdatedUnix`?

tooltip? And why italic? Wouldn't the svg suffice?

Please write a comment on what these methods are supposed to do… I don't really know what I have to think of a sealed or unsealed provider. Is it unable to generate further secrets, or what is its purpose?

Didn't we remove that?

Copied from above: ```suggestion ctx.Flash.Error(ctx.Tr("repo.settings.secret_deletion_error", error)) ```

```suggestion if form.MasterKeyProvider != secrets.MasterKeyProviderTypeNone { ``` ?

Those two config options are currently missing from `config-cheat-sheet`. Or are they supposed to be undocumented?

`OwnerID`?

and given my suggestion above, the case `case s.UserID == 0` should also return an error.

I just noticed: If we change our preconditions slightly, we can also do something more advanced: If we **always** set the user ID even on org secrets, then an org secret is simply a secret with `repoID == 0`. This has the benefit that we can then enforce a combined uniqueness constraint over the userID, repoID, and the name. This would have the advantage that you cannot have more than one secret for the same user-repo combi, so no global or repo specific secret could have a duplicate name. I think that's a check we cannot omit, as the current implementation allows for inconsistent states when multiple secrets have the same name. --- Let's test that there are no drawbacks on both possible values: Assume we have a secret `name` in org `1`. - the org secret would be `1, 0, name` - the repo secret for a specific repo, i.e. `2`, would be `1, 2, name`, so it doesn't violate the constraint. the only thing to keep in mind when collecting the secrets is that we need to query the org secrets first so that the repo secrets will override the org secrets. --- One other thing I noticed: We should also delete repo secrets when a repo is deleted and org secrets when an org is deleted. Should hopefully be a two-liner.

Actually, that's only half of the answer. The imported org secrets are currently not shown, which is not a good idea for consistency. GitHub does this nicely, they show what org secrets are available as well (read-only) and simply redirect to the org secrets settings page where you can do whatever you want with them once you click on an org secret. That also has the benefit that we don't care about permissions or similar, since a user that has owner access in a repo must also have owner access in the org to access that page.

copy-paste? ```suggestion ctx.ServerError("FindUserSecrets", err) ```

Why plain?

Should also be updated when you update the data model as requested above.

What about an extra function that simply takes all parameters and returns the potential error? Also, as it seems right now, we don't remove the surrounding whitespace. I don't think that's a good idea. It's too easy to have a newline at the end or spaces in the beginning, and you have no way to check then if your secret was copied correctly other than to manually run something that uses the secret and then fails. If requested, we can still add a checkbox later on, to disable removing the whitespace, but I don't think that'll be needed for now.

Isn't that placeholder redundant? It doesn't serve any purpose. It would only have a purpose if it clarified for example how surounding whitespace is handled. Speaking of which: Do we strip surrounding whitespace already? I hope so as that is in general much more user-friendly…

These lines do not sound like English: <details><summary>UI Screenshot</summary>  </details> I have absolutely no idea what they are even supposed to mean, so I can't suggest alternatives.

```suggestion in_error = ` can only contain specific values: %s.` ```

Erm… You are displaying an error as a log message in the front end? Why not a ```suggestion ctx.Flash.Error(ctx.Tr("repo.settings.secret_deletion_error", error)) ``` for a new translation string?

Why 50? Why not 255?

I have no idea what this is supposed to mean. Is that some sort of access control system? If yes, we should refine it before merging it like this.

I'm almost in favor of setting `context.OrgAssignment(true, true)` here as a duplicate middleware simply to ensure that there is no chance ever that someone changes the permissions below and doesn't notice that the secrets are now publicly available.

All new files don't conform with the new headers anymore: Sometimes, the year is still `2021`, and the other two lines should be replaced with the new header.

Admit it: You wanted to name it that way :)

Why so short when the `SecretKey` already has 64 runes?

Should we perhaps allow `@` and `#` as well, and disallow a leading `-` at the beginning? This will probably cause far fewer problems long term…

I am not quite sure what this attribute is supposed to be: Why would we need that? What would a PR-specific secret be? Or how is that meant? And, if we do need it: I don't think it is a good idea to limit it to 0 and 1 only, it's probably a better idea to define a separate `type SecretType int { GlobalSecret SecretType = iota; PullSecret = 1; …}`, or to use a separate permissions table for that.

Why this check? I do not think mixing handlers is correct.

Will there ever be providers which return multiple secrets? If yes, why does the `Unseal` method only take one secret?