Skip to content
New issue

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

#

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

Already on GitHub? # to your account

Jump to bottom

Use user.FullName in Oauth2 id_token response #32542

Merged
merged 1 commit into from
Nov 18, 2024
Merged

Use user.FullName in Oauth2 id_token response #32542

merged 1 commit into from
Nov 18, 2024

Conversation

baltitenger
Copy link
Contributor

This makes /#/oauth/authorize behave the same way as the /#/oauth/userinfo endpoint.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 17, 2024
@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Nov 17, 2024
@github-actions github-actions bot added the modifies/go Pull requests that update Go code label Nov 17, 2024
@lunny
Copy link
Member

lunny commented Nov 17, 2024

But FullName maybe empty

@baltitenger
Copy link
Contributor Author

Then I guess the userinfo handler should be changed to work the same way, or both changed to use FullName with a fallback to Name

@baltitenger
Copy link
Contributor Author

Oh wtf, there's DisplayName and GetDisplayName... Would using DisplayName in both places be fine?

@pull-request-size pull-request-size bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 17, 2024
@lunny
Copy link
Member

lunny commented Nov 17, 2024

I think both needs to be changed to use DisplayName, because it implemented a fallback if Fullname is empty.

@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 Nov 18, 2024
@lunny lunny added the type/enhancement An improvement of existing functionality label Nov 18, 2024
@lunny lunny added this to the 1.23.0 milestone Nov 18, 2024
@wxiaoguang
Copy link
Contributor

Thank you for the PR!

It seems that DisplayName/GetDisplayName/FullName these names are too ambiguous .... I will try to make some improvements in this PR together to make them easier for contributors to understand

@baltitenger
Use user.FullName in Oauth2 id_token response
bc6cce9 
This makes `/#/oauth/authorize` behave the same way as the
`/#/oauth/userinfo` endpoint.
@wxiaoguang wxiaoguang self-assigned this Nov 18, 2024
wxiaoguang
wxiaoguang approved these changes Nov 18, 2024
View reviewed changes
Copy link
Contributor

@wxiaoguang wxiaoguang left a comment

Choose a reason for hiding this comment

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

Maybe these function names could be refactored later, too complex ......

@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 Nov 18, 2024
@wxiaoguang wxiaoguang removed their assignment Nov 18, 2024
@wxiaoguang wxiaoguang merged commit 5eb0ee4 into go-gitea:main Nov 18, 2024
26 checks passed
zjjhot added a commit to zjjhot/gitea that referenced this pull request Nov 20, 2024
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
a8b8d32 
* giteaofficial/main:
  Remove duplicate empty repo check in delete branch API (go-gitea#32569)
  Optimize installation-page experience (go-gitea#32558)
  Remove unnecessary code (go-gitea#32560)
  Fix a compilation error in the Gitpod environment (go-gitea#32559)
  Use user.FullName in Oauth2 id_token response (go-gitea#32542)
  Fix some places which doesn't repsect org full name setting (go-gitea#32243)
  Refactor push mirror find and add check for updating push mirror (go-gitea#32539)
  Refactor markup render system (go-gitea#32533)
  Refactor find forks and fix possible bugs that weak permissions check (go-gitea#32528)
  Use better name for userinfo structure (go-gitea#32544)
DennisRasey pushed a commit to DennisRasey/forgejo that referenced this pull request Nov 27, 2024
@baltitenger
Use user.FullName in Oauth2 id_token response (#6071)
0734596 
Cherry-pick of [gitea#32542](go-gitea/gitea#32542).

This makes /#/oauth/authorize behave the same way as the /#/oauth/userinfo endpoint. Previously, `name` property of the returned OIDCToken used to depend on the UI.DefaultShowFullName setting (I don't think this is desired behavior). Even worse, the `userinfo` endpoint can return basically the same data, but the `name` value there always returned `FullName`, even if it's empty (no fallback to `Name`).

A few notes:

I'm not sure what branch to target with this PR, please correct me if I'm chose the wrong one.

The deleted lines in the tests are duplicates, there's a copy of the whole thing just below, the only difference being the `Name` field (used to test the dependency on the UI.DefaultShowFullName setting)

## Checklist

The [contributor guide](https://forgejo.org/docs/next/contributor/) contains information that will be helpful to first time contributors. There also are a few [conditions for merging Pull Requests in Forgejo repositories](https://codeberg.org/forgejo/governance/src/branch/main/PullRequestsAgreement.md). You are also welcome to join the [Forgejo development chatroom](https://matrix.to/#/#forgejo-development:matrix.org).

### Tests

- I added test coverage for Go changes...
  - [ ] in their respective `*_test.go` for unit tests.
  - [ ] in the `tests/integration` directory if it involves interactions with a live Forgejo server.
- I added test coverage for JavaScript changes...
  - [ ] in `web_src/js/*.test.js` if it can be unit tested.
  - [ ] in `tests/e2e/*.test.e2e.js` if it requires interactions with a live Forgejo server (see also the [developer guide for JavaScript testing](https://codeberg.org/forgejo/forgejo/src/branch/forgejo/tests/e2e/README.md#end-to-end-tests)).

### Documentation

- [ ] I created a pull request [to the documentation](https://codeberg.org/forgejo/docs) to explain to Forgejo users how to use this change.
- [ ] I did not document these changes and I do not expect someone else to do it.

### Release notes

- [ ] I do not want this change to show in the release notes.
- [ ] I want the title to show in the release notes with a link to this pull request.
- [ ] I want the content of the `release-notes/<pull request number>.md` to be be used for the release notes instead of the title.

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/6071
Reviewed-by: Earl Warren <earl-warren@noreply.codeberg.org>
Reviewed-by: Gusted <gusted@noreply.codeberg.org>
Co-authored-by: Baltazár Radics <baltazar.radics@gmail.com>
Co-committed-by: Baltazár Radics <baltazar.radics@gmail.com>
@baltitenger baltitenger deleted the oauth-name-fullname branch December 7, 2024 16:34
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Feb 16, 2025
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Reviewers

@lunny lunny lunny approved these changes

@wxiaoguang wxiaoguang wxiaoguang approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/go Pull requests that update Go code size/S Denotes a PR that changes 10-29 lines, ignoring generated files. type/enhancement An improvement of existing functionality
Projects
None yet
Milestone
1.23.0
Development

Successfully merging this pull request may close these issues.
4 participants
@baltitenger @lunny @wxiaoguang @GiteaBot