forgejo
/
forgejo
51
858
Fork 98
Code Issues 267 Pull Requests 14 Actions Packages Releases 28 Wiki Activity

FEAT: oauth2: use link_account page when email/username is missing #1757

Merged
earl-warren merged 5 commits from wetneb/forgejo:1755-oauth2-missing-email into forgejo 2023-11-19 11:50:10 +00:00
Conversation 6 Commits 5 Files Changed 2
wetneb commented 2023-11-12 13:33:34 +00:00
Collaborator

This avoids simply failing with an HTTP 500 error instead.
Closes #1755.

This is a work in progress, I am looking for feedback on this approach :)

This avoids simply failing with an HTTP 500 error instead. Closes #1755. <s>This is a work in progress,</s> I am looking for feedback on this approach :) <!-- Before submitting a PR, please read the contributing guidelines: https://codeberg.org/forgejo/forgejo/src/branch/main/CONTRIBUTING.md -->
👍 1
wetneb added 1 commit 2023-11-12 13:33:46 +00:00
testing / lint-backend (pull_request) Failing after 58s Details
testing / checks-backend (pull_request) Failing after 1m8s Details
testing / test-unit (pull_request) Has been skipped Details
testing / test-mysql (pull_request) Has been skipped Details
testing / test-pgsql (pull_request) Has been skipped Details
testing / test-sqlite (pull_request) Has been skipped Details
a61fc88726 oauth2: use link_account page when email/username is missing 
This avoids simply failing with an HTTP 500 error instead.
Closes #1755.
earl-warren added the
enhancement/feature
User Research - feature
 labels 2023-11-12 22:46:45 +00:00
earl-warren commented 2023-11-12 22:47:33 +00:00
Owner

I like the idea.

I like the idea.
🎉 1
earl-warren commented 2023-11-13 12:53:41 +00:00
Owner

Heads up: Forgejo is being rebased on Gitea today (that's a weekly occurrence). It means you will need to cherry-pick your commits on top of the newer base branch for this PR and maybe resolve conflicts. You can do that on your own time, you do not need to do that today. It can be in a week or a month and you can keep working in the meantime.

Ideally Gitea would be a package that can be upgraded as any other Go package. But it is not designed in this way and the weekly rebase is the equivalent of such an upgrade, only it is more work.

If you need help doing that, feel free to reach out in the development chatroom.

Heads up: Forgejo is being [rebased on Gitea](https://codeberg.org/forgejo/forgejo/issues?q=Furnace+cleanup) today (that's a weekly occurrence). It means you will need to cherry-pick your commits on top of the newer base branch for this PR and maybe resolve conflicts. You can do that on your own time, you do not need to do that today. It can be in a week or a month and you can keep working in the meantime. Ideally Gitea would be a package that can be upgraded as any other Go package. But it is not designed in this way and the weekly rebase is the equivalent of such an upgrade, only it is more work. If you need help doing that, feel free to reach out in the [development chatroom](https://matrix.to/#/#forgejo-development:matrix.org).
wetneb force-pushed 1755-oauth2-missing-email from a61fc88726 to 54fa881cba 2023-11-14 19:05:43 +00:00 Compare
wetneb force-pushed 1755-oauth2-missing-email from 54fa881cba to 687ad13956 2023-11-14 19:32:17 +00:00 Compare
wetneb force-pushed 1755-oauth2-missing-email from 687ad13956 to c7206910b7 2023-11-14 19:39:41 +00:00 Compare
wetneb added 1 commit 2023-11-14 20:29:37 +00:00
testing / lint-backend (pull_request) Failing after 58s Details
testing / checks-backend (pull_request) Failing after 1m9s Details
testing / test-unit (pull_request) Has been skipped Details
testing / test-mysql (pull_request) Has been skipped Details
testing / test-pgsql (pull_request) Has been skipped Details
testing / test-sqlite (pull_request) Has been skipped Details
215a20078a Add failing test
wetneb force-pushed 1755-oauth2-missing-email from 215a20078a to ee6885faf0 2023-11-14 20:30:02 +00:00 Compare
wetneb force-pushed 1755-oauth2-missing-email from ee6885faf0 to 50e6c28c6b 2023-11-14 20:36:52 +00:00 Compare
wetneb force-pushed 1755-oauth2-missing-email from 50e6c28c6b to 5f1f6a1ce4 2023-11-14 20:55:01 +00:00 Compare
wetneb changed title from WIP: oauth2: use link_account page when email/username is missing to FEAT: oauth2: use link_account page when email/username is missing 2023-11-14 21:12:51 +00:00
wetneb commented 2023-11-14 21:13:34 +00:00
Poster
Collaborator

@earl-warren thanks for the encouragement! I would declare it ready for review then :)

@earl-warren thanks for the encouragement! I would declare it ready for review then :)
wetneb added 1 commit 2023-11-14 21:19:00 +00:00
testing / lint-backend (pull_request) Successful in 2m57s Details
testing / checks-backend (pull_request) Successful in 3m3s Details
testing / test-unit (pull_request) Successful in 6m52s Details
testing / test-mysql (pull_request) Successful in 12m8s Details
testing / test-sqlite (pull_request) Successful in 11m57s Details
testing / test-pgsql (pull_request) Successful in 13m5s Details
8dcdcd5ceb Remove spurious newline
Gusted reviewed 2023-11-16 21:07:18 +00:00
routers/web/auth/oauth.go Outdated
@ -970,1 +971,3 @@
ctx.ServerError("CreateUser", err)
// we don't have enough information to create an account automatically,
// so we prompt the user for the remaining bits
showLinkingLogin(ctx, gothUser)
Gusted commented 2023-11-16 21:06:20 +00:00
Owner

I think it's good to add a log.Trace to show which fields were missing.

I think it's good to add a `log.Trace` to show which fields were missing.
wetneb commented 2023-11-16 22:51:18 +00:00
Poster
Collaborator

Fair enough, I have added one.

Fair enough, I have added one.
Gusted marked this conversation as resolved
wetneb added 1 commit 2023-11-16 22:52:22 +00:00
testing / lint-backend (pull_request) Successful in 3m0s Details
testing / checks-backend (pull_request) Successful in 2m47s Details
testing / test-unit (pull_request) Successful in 6m14s Details
testing / test-mysql (pull_request) Successful in 11m32s Details
testing / test-sqlite (pull_request) Successful in 11m36s Details
testing / test-pgsql (pull_request) Successful in 12m44s Details
9fa7601a0c Add trace logging suggested by Gusted
Gusted reviewed 2023-11-18 22:01:15 +00:00
tests/integration/oauth_test.go Outdated
@ -472,0 +473,4 @@
func TestSignUpViaOAuthWithMissingFields(t *testing.T) {
defer tests.PrepareTestEnv(t)()
// enable auto-creation of accounts via OAuth2
setting.OAuth2Client.EnableAutoRegistration = true
Gusted commented 2023-11-18 22:01:10 +00:00
Owner

This has to be disabled after this.

This has to be disabled after this.
wetneb commented 2023-11-18 22:27:10 +00:00
Poster
Collaborator

That's a most excellent point. Did I do it right?

That's a most excellent point. Did I do it right?
Gusted commented 2023-11-19 10:14:12 +00:00
Owner

Yes, perfect!

Yes, perfect!
Gusted marked this conversation as resolved
wetneb added 1 commit 2023-11-18 22:23:36 +00:00
testing / lint-backend (pull_request) Successful in 2m56s Details
testing / checks-backend (pull_request) Successful in 2m53s Details
testing / test-unit (pull_request) Successful in 7m7s Details
testing / test-mysql (pull_request) Successful in 15m42s Details
testing / test-sqlite (pull_request) Successful in 16m31s Details
testing / test-pgsql (pull_request) Successful in 17m53s Details
f6ca2f749c Reset the setting after end of test
wetneb force-pushed 1755-oauth2-missing-email from f6ca2f749c to 2d343db363 2023-11-18 22:27:03 +00:00 Compare
wetneb force-pushed 1755-oauth2-missing-email from 2d343db363 to 127a86d452 2023-11-18 22:28:19 +00:00 Compare
Gusted approved these changes 2023-11-19 10:17:44 +00:00
earl-warren merged commit 564e094256 into forgejo 2023-11-19 11:50:10 +00:00
earl-warren commented 2023-11-19 11:52:49 +00:00
Owner

For the record, moved the commit to the forgejo-dependency branch where it belongs.

For the record, [moved the commit](https://codeberg.org/forgejo/forgejo/commit/0f6e0f90359b4b669d297a533de18b41e3293df2) to the `forgejo-dependency` branch where it belongs.
wetneb commented 2023-11-19 11:53:36 +00:00
Poster
Collaborator

Thanks, I'll make sure I'll use that branch for my upcoming PRs.

Thanks, I'll make sure I'll use that branch for my upcoming PRs.
wetneb deleted branch 1755-oauth2-missing-email 2023-11-19 11:53:48 +00:00
earl-warren commented 2023-11-19 12:05:07 +00:00
Owner

Thanks! It's perfectly ok to target forgejo if unsure: moving commits around is quite common in the Forgejo workflow.

Thanks! It's perfectly ok to target forgejo if unsure: moving commits around is quite common in [the Forgejo workflow](https://forgejo.org/docs/next/developer/workflow/).
earl-warren added this to the Forgejo v1.22 milestone 2023-11-30 14:17:37 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Gusted
Labels
Clear labels   
backport/v1.19

Scheduled for backport to Forgejo v1.19   
backport/v1.20

Scheduled for backport to Forgejo v1.20   
backport/v1.21

Scheduled for backport to Forgejo v1.21   
bug

Something is not working
  
dependency
Chi
https://github.com/go-chi/chi/   
dependency
Chroma
https://github.com/alecthomas/chroma/   
dependency
citation.js
https://github.com/larsgw/citation.js/   
dependency
devcontainers
https://github.com/devcontainers/cli   
dependency
dropzone
https://github.com/dropzone/dropzone   
dependency
F3
https://f3.forgefriends.org/   
dependency
ForgeFed
https://forgefed.org   
dependency
garage
https://git.deuxfleurs.fr/Deuxfleurs/garage   
dependency
Gitea
https://github.com/go-gitea/gitea   
dependency
go-enry
https://github.com/go-enry/go-enry   
dependency
Go-org
https://github.com/niklasfasching/go-org   
dependency
go-sql-driver mysql
https://github.com/go-sql-driver/mysql/   
dependency
Golang
https://go.dev/   
dependency
goldmark
https://github.com/yuin/goldmark   
dependency
Goth
https://github.com/markbates/goth/   
dependency
Helm
https://helm.sh/   
dependency
MariaDB
https://mariadb.com/   
dependency
Mermaid
https://github.com/mermaid-js/mermaid/   
dependency
minio-go
https://github.com/minio/minio-go/   
dependency
Monaco
https://github.com/microsoft/monaco-editor   
dependency
PDFobject
https://pdfobject.com/   
dependency
ssh
https://github.com/gliderlabs/ssh   
dependency
urfave
https://github.com/urfave/cli   
dependency
Woodpecker CI
https://woodpecker-ci.org/   
dependency
XORM
https://gitea.com/xorm/xorm
  
Discussion
   
duplicate

This issue or pull request already exists   
enhancement/feature

New feature   
forgejo/accessibility

Accessibility   
forgejo/actions

Actions   
forgejo/branding

Branding (logo, name, tagline etc.)   
forgejo/ci

Forgejo Actions CI configuration   
forgejo/documentation

Documentation is now at https://codeberg.org/forgejo/website/src/branch/main/src/pages/docs   
forgejo/federation

Federation   
forgejo/furnace cleanup

Keeping Forgejo in sync with its dependencies and contributing back to them   
forgejo/internationalization

Internationalization (i18n) and Localization (l10n)   
forgejo/moderation

Moderation   
forgejo/privacy

Privacy first   
forgejo/release

Release management   
forgejo/scaling

Performance and scaling   
forgejo/security

Security (please disclose responsibly)   
forgejo/ui

User interface
  
issue
closed
The issue was resolved in the repository of the dependency   
issue
do-not-exist-yet
An issue should be created in the respository of the dependency   
issue
open
An open issue exists in the upstream repository of the dependency
  
OS
FreeBSD
Specific to the FreeBSD Operating System   
OS
Linux
Specific to (GNU/)Linux Operating Systems   
OS
MacOS
Specific to the MacOS Operating System   
OS
Windows
Specific to the Windows Operating System
  
ready-to-merge

Trigger additional tests on the PR when it is ready to be merged   
untested

Pull requests that have been merged with no test and submitted as is to the dependency where they belong
  
User Research - bug
   
User Research - communication
   
User Research - documentation
   
User Research - feature
   
User Research - federation
   
User Research - governance
   
User Research - release
   
User Research - security
   
User Research - testing
   
User Research - UI
   
User Research - UX
No Label
backport/v1.19
backport/v1.20
backport/v1.21
bug
dependency
Chi
dependency
Chroma
dependency
citation.js
dependency
devcontainers
dependency
dropzone
dependency
F3
dependency
ForgeFed
dependency
garage
dependency
Gitea
dependency
go-enry
dependency
Go-org
dependency
go-sql-driver mysql
dependency
Golang
dependency
goldmark
dependency
Goth
dependency
Helm
dependency
MariaDB
dependency
Mermaid
dependency
minio-go
dependency
Monaco
dependency
PDFobject
dependency
ssh
dependency
urfave
dependency
Woodpecker CI
dependency
XORM
Discussion
duplicate
enhancement/feature
forgejo/accessibility
forgejo/actions
forgejo/branding
forgejo/ci
forgejo/documentation
forgejo/federation
forgejo/furnace cleanup
forgejo/internationalization
forgejo/moderation
forgejo/privacy
forgejo/release
forgejo/scaling
forgejo/security
forgejo/ui
issue
closed
issue
do-not-exist-yet
issue
open
OS
FreeBSD
OS
Linux
OS
MacOS
OS
Windows
ready-to-merge
untested
User Research - bug
User Research - communication
User Research - documentation
User Research - feature
User Research - federation
User Research - governance
User Research - release
User Research - security
User Research - testing
User Research - UI
User Research - UX
Milestone
Clear milestone
No items
No Milestone
Forgejo v1.22
Assignees
Clear assignees
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: forgejo/forgejo#1757
There is no content yet.
Delete Branch "wetneb/forgejo:1755-oauth2-missing-email"

Deleting a branch is permanent. Although the deleted branch may exist for a short time before cleaning up, in most cases it CANNOT be undone. Continue?