Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update the webauthn_credential_id_sequence in Postgres #19048

Merged
merged 4 commits into from
Mar 10, 2022
Merged

Update the webauthn_credential_id_sequence in Postgres #19048

merged 4 commits into from
Mar 10, 2022

Conversation

zeripath
Copy link
Contributor

@zeripath zeripath commented Mar 10, 2022 •
edited
Loading

There is (yet) another problem with v210 in that Postgres will silently allow preset
ID insertions ... but it will not update the sequence value.

This PR simply adds a little step to the end of the v210 migration to update the
sequence number.

Users who have already migrated who find that they cannot insert new
webauthn_credentials into the DB can either run:

gitea doctor recreate-table webauthn_credential

or in postgres run:

SELECT setval('webauthn_credential_id_seq', COALESCE((SELECT MAX(id)+1 FROM `webauthn_credential`), 1), false)

which will fix the bad sequence.

Fix #19012

Signed-off-by: Andrew Thornton art27@cantab.net

@zeripath
Update the webauthn_credential_id_sequence in Postgres
ff48df3 
There is (yet) another problem with v210 in that Postgres will silently allow preset
ID insertions ... but it will not update the sequence value.

This PR simply adds a little step to the end of the v210 migration to update the
sequence number.

Users who have already migrated who find that they cannot insert new
webauthn_credentials into the DB can either run:

```bash
gitea doctor recreate-table webauthn_credential
```

or

```bash
./gitea doctor --run=check-db-consistency --fix
```

which will fix the bad sequence.

Fix go-gitea#19012

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath zeripath added this to the 1.17.0 milestone Mar 10, 2022
@zeripath
Copy link
Contributor Author

zeripath commented Mar 10, 2022 •
edited
Loading

An alternative option is to add yet another migration v211 which will simply run the alter sequence command then we can drop the rest of the warning and make it automatic.

Actually let me just do that. On second thoughts maybe not - I'm gonna let reviewers decide that. The problem with adding this as a new migration is:

  • Imagine a server which was not affected primarily because no insertions took place in v210.
  • Users have since been able to insert and then delete webauthn_credentials. Imagine the last few credentials inserted have been deleted.
  • The max ID in the webauthn_credential would be say n, whereas the sequence would actually be n+k.
  • An additional migration would set the sequence back to n+1.

This is a low risk problem and should cause no issues but it is one worth considering.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 10, 2022
@zeripath
spelling mistakes
ad642e2 
Signed-off-by: Andrew Thornton <art27@cantab.net>
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Mar 10, 2022
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Mar 10, 2022
@6543
Copy link
Member

6543 commented Mar 10, 2022

🚀

@6543 6543 merged commit 886b1de into go-gitea:main Mar 10, 2022
@zeripath zeripath deleted the fix-v210-again-postgres-sequence branch March 10, 2022 22:26
zjjhot added a commit to zjjhot/gitea that referenced this pull request Mar 11, 2022
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
151563f 
* giteaofficial/main:
  Prevent 500 when there is an error during new auth source post (go-gitea#19041)
  Update the webauthn_credential_id_sequence in Postgres (go-gitea#19048)
  If rendering has failed due to a net.OpError stop rendering (attempt 2) (go-gitea#19049)
  use xorm builder for models.getReviewers() (go-gitea#19033)
  RSS/Atom support for Orgs (go-gitea#17714)
  Fix flag validation (go-gitea#19046)
  Improve SyncMirrors logging (go-gitea#19045)
zeripath added a commit to zeripath/gitea that referenced this pull request Mar 11, 2022
@zeripath
Update the webauthn_credential_id_sequence in Postgres (go-gitea#19048)
2bbd830 
Backport go-gitea#19048

There is (yet) another problem with v210 in that Postgres will silently allow preset
ID insertions ... but it will not update the sequence value.

This PR simply adds a little step to the end of the v210 migration to update the
sequence number.

Users who have already migrated who find that they cannot insert new
webauthn_credentials into the DB can either run:

```bash
gitea doctor recreate-table webauthn_credential
```

or

```bash
SELECT setval('webauthn_credential_id_seq', COALESCE((SELECT MAX(id)+1 FROM `webauthn_credential`), 1), false)
```

which will fix the bad sequence.

Fix go-gitea#19012

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath zeripath mentioned this pull request Mar 11, 2022
Merged
@zeripath zeripath added the backport/done All backports for this PR have been created label Mar 11, 2022
lunny pushed a commit that referenced this pull request Mar 13, 2022
@zeripath @6543
Update the webauthn_credential_id_sequence in Postgres (#19048) (#19060)
d285905 
Backport #19048

There is (yet) another problem with v210 in that Postgres will silently allow preset
ID insertions ... but it will not update the sequence value.

This PR simply adds a little step to the end of the v210 migration to update the
sequence number.

Users who have already migrated who find that they cannot insert new
webauthn_credentials into the DB can either run:

```bash
gitea doctor recreate-table webauthn_credential
```

or

```bash
SELECT setval('webauthn_credential_id_seq', COALESCE((SELECT MAX(id)+1 FROM `webauthn_credential`), 1), false)
```

which will fix the bad sequence.

Fix #19012

Signed-off-by: Andrew Thornton <art27@cantab.net>

Co-authored-by: 6543 <6543@obermui.de>
Chianina pushed a commit to Chianina/gitea that referenced this pull request Mar 28, 2022
@zeripath
Update the webauthn_credential_id_sequence in Postgres (go-gitea#19048)
57812d9 
* Update the webauthn_credential_id_sequence in Postgres

There is (yet) another problem with v210 in that Postgres will silently allow preset
ID insertions ... but it will not update the sequence value.

This PR simply adds a little step to the end of the v210 migration to update the
sequence number.

Users who have already migrated who find that they cannot insert new
webauthn_credentials into the DB can either run:

```bash
gitea doctor recreate-table webauthn_credential
```

or

```bash
./gitea doctor --run=check-db-consistency --fix
```

which will fix the bad sequence.

Fix go-gitea#19012

Signed-off-by: Andrew Thornton <art27@cantab.net>
@go-gitea go-gitea locked and limited conversation to collaborators Apr 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@techknowlogick techknowlogick techknowlogick left review comments

@6543 6543 6543 approved these changes

@Gusted Gusted Gusted approved these changes

Assignees
No one assigned
Labels
backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug
Projects
None yet
Milestone
1.17.0
Development

Successfully merging this pull request may close these issues.

Unable to re-register a security key (webauthn_credential_pkey constraint)
5 participants
@zeripath @6543 @techknowlogick @Gusted @GiteaBot