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

Attempt to fix the webauthn migration again - part 3 #18770

Merged
merged 19 commits into from
Feb 16, 2022
Merged

Attempt to fix the webauthn migration again - part 3 #18770

merged 19 commits into from
Feb 16, 2022

Conversation

zeripath
Copy link
Contributor

@zeripath zeripath commented Feb 14, 2022 •
edited
Loading

v208.go is seriously broken as it misses an ID() check. We need to no-op and remigrate all of the u2f keys.

See #18756

Signed-off-by: Andrew Thornton [email protected]

@zeripath zeripath added type/bug backport/done All backports for this PR have been created backport/v1.16 labels Feb 14, 2022
@zeripath zeripath added this to the 1.17.0 milestone Feb 14, 2022
@zeripath
Attempt to fix the webauthn migration again - part 3
8c142c3 
MariaDB now asserts that 408 characters is too long for a VARCHAR(410)

I have no words.

See go-gitea#18756

Signed-off-by: Andrew Thornton <[email protected]>
@zeripath zeripath force-pushed the varchar-410-is-not-enough branch from 769f557 to 8c142c3 Compare February 14, 2022 23:28
@zeripath zeripath mentioned this pull request Feb 14, 2022
Merged
@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (main@4482f62). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head 8c142c3 differs from pull request most recent head a6fd120. Consider uploading reports for the commit a6fd120 to get more accurate results

Impacted file tree graph

@@           Coverage Diff           @@
##             main   #18770   +/-   ##
=======================================
  Coverage        ?   46.58%           
=======================================
  Files           ?      852           
  Lines           ?   122456           
  Branches        ?        0           
=======================================
  Hits            ?    57043           
  Misses          ?    58529           
  Partials        ?     6884
Impacted Files Coverage Δ
models/auth/webauthn.go 43.11% <ø> (ø)

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 4482f62...a6fd120. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 14, 2022
fnetX
fnetX reviewed Feb 15, 2022
View reviewed changes
models/auth/webauthn.go Outdated Show resolved Hide resolved
zeripath
zeripath commented Feb 15, 2022
View reviewed changes
models/migrations/v209.go Outdated Show resolved Hide resolved
@zeripath
Copy link
Contributor Author

I think this might actually still be insufficient.

Is it possible that v208 might have updated the IDs to all be the same number too?

If that's the case then this won't remigrate.

Then there's the case of the users who have deleted the old u2f keys already too.

lunny
lunny reviewed Feb 15, 2022
View reviewed changes
models/migrations/v210.go Outdated Show resolved Hide resolved
lunny
lunny reviewed Feb 15, 2022
View reviewed changes
models/migrations/v210.go Outdated Show resolved Hide resolved
zeripath and others added 3 commits February 15, 2022 11:49
@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 Feb 15, 2022
@zeripath
no-op v207
3c168c8 
Signed-off-by: Andrew Thornton <[email protected]>
@zeripath
Copy link
Contributor Author

OK I think this is finally ready

Gusted
Gusted approved these changes Feb 16, 2022
View reviewed changes
Copy link
Contributor

@Gusted Gusted left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's hope it be the last one 😄 Couldn't find any issue with it.

@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 Feb 16, 2022
@zeripath zeripath merged commit 3a29a23 into go-gitea:main Feb 16, 2022
@zeripath zeripath deleted the varchar-410-is-not-enough branch February 16, 2022 21:04
zeripath added a commit that referenced this pull request Feb 16, 2022
@zeripath
Attempt to fix the webauthn migration again - part 3 (#18770) (#18771)
08d5a83 
Backport #18770 

v208.go is seriously broken as it misses an ID() check. We need to no-op and remigrate all of the u2f keys.

See #18756

Signed-off-by: Andrew Thornton <[email protected]>
zjjhot added a commit to zjjhot/gitea that referenced this pull request Feb 17, 2022
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
776fa23 
* giteaofficial/main:
  Move deletebeans into models/db (go-gitea#18781)
  Allow mermaid render error to wrap (go-gitea#18790)
  Attempt to fix the webauthn migration again - part 3 (go-gitea#18770)
  Fix template bug of LFS lock (go-gitea#18784)
@mscherer mscherer mentioned this pull request Feb 24, 2022
Closed
Chianina pushed a commit to Chianina/gitea that referenced this pull request Mar 28, 2022
@zeripath
Attempt to fix the webauthn migration again - part 3 (go-gitea#18770)
4131286 
v208.go is seriously broken as it misses an ID() check. We need to no-op and remigrate all of the u2f keys.

See go-gitea#18756

Signed-off-by: Andrew Thornton <[email protected]>
@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

@Gusted Gusted Gusted approved these changes

@fnetX fnetX fnetX approved these changes

@lunny lunny lunny 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.
6 participants
@zeripath @codecov-commenter @lunny @fnetX @Gusted @GiteaBot