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 API to return 'source_id' for users #29718

Merged
merged 35 commits into from Apr 16, 2024
Merged

Update API to return 'source_id' for users #29718

merged 35 commits into from Apr 16, 2024
+9 −0

Conversation

tobiasbp
Copy link
Contributor

Using the API, a user's source_id can be set in the CreateUserOption model, but the field is not returned in the User model.

This PR updates the User model to include the field source_id (The ID of the Authentication Source).

The name source_id is a bit confusing since it does not hint at any relation with the login_name field. Perhaps login_source_id or login_id would be a better name? If a different name than source_id was to be used, the name should also be updated in the CreateUserOption and EditUserOptions models.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 11, 2024
@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Mar 11, 2024
@lunny
Copy link
Member

lunny commented Mar 12, 2024

You need to generate swagger docs.

@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 Mar 12, 2024
@github-actions github-actions bot added the modifies/api This PR adds API routes or modifies them label Mar 12, 2024
@pull-request-size pull-request-size bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 12, 2024
@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 14, 2024
@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 14, 2024
@lunny lunny added type/enhancement An improvement of existing functionality reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. labels Mar 14, 2024
@lunny lunny added this to the 1.22.0 milestone Mar 14, 2024
@github-actions github-actions bot added the modifies/go Pull requests that update Go code label Mar 14, 2024
@techknowlogick
Copy link
Member

I agree re The name source_id is a bit confusing since it does not hint at any relation with the login_name field, but I think, at least for now, keeping it the same is fine, especially since you have it documented in the swagger docs to denote the relationship.

@tobiasbp
Copy link
Contributor Author

tobiasbp commented Apr 2, 2024

Is there no way to get this into 1.22.0 @lunny? I don't know what I need to change to make @techknowlogick and @delvh approve of it?

@lunny
Copy link
Member

lunny commented Apr 2, 2024

This looks not too big and is not a feature and got 2 approvals before the v1.22 branch was created. I think it's possible to backport to v1.22 if @techknowlogick and @delvh have no objections.

@delvh
Copy link
Member

delvh commented Apr 2, 2024

So, did you come to a conclusion which name to use?

@tobiasbp
Copy link
Contributor Author

tobiasbp commented Apr 2, 2024

So, did you come to a conclusion which name to use?

I proposed this earlier: #29718 (comment)

@tobiasbp
Copy link
Contributor Author

tobiasbp commented Apr 3, 2024

So, what do you think of my suggestion @delvh @techknowlogick ?
#29718 (comment)

@lunny
Copy link
Member

lunny commented Apr 3, 2024

Suddenly, I have another idea. Since we have the opportunity to refactor the user system. Maybe we should remove login_source_id in the user table but put it into external_account because one user could bind multiple auth systems. So if we merge this PR, we need to deprecate it later.

@tobiasbp
Copy link
Contributor Author

tobiasbp commented Apr 3, 2024

Suddenly, I have another idea. Since we have the opportunity to refactor the user system. Maybe we should remove login_source_id in the user table but put it into external_account because one user could bind multiple auth systems. So if we merge this PR, we need to deprecate it later.

I'm not sure we are talking about the same thing here. A user can only have a single authentication source .

image

@lunny
Copy link
Member

lunny commented Apr 3, 2024

This is the current design but I think the user can also login with some OAuth2.

@tobiasbp
Copy link
Contributor Author

tobiasbp commented Apr 3, 2024

This is the current design but I think the user can also login with some OAuth2.

I have this set up with two openID authentication sources for two different types of users. Since I can not get the auth source ID from the API, I can not (easily) tell the users apart.

Is this system going to change in the next version of Gitea?

@lunny
Copy link
Member

lunny commented Apr 3, 2024

This is the current design but I think the user can also login with some OAuth2.

I have this set up with two openID authentication sources for two different types of users. Since I can not get the auth source ID from the API, I can not (easily) tell the users apart.

Is this system going to change in the next version of Gitea?

I think it will not be in next version of Gitea but we know it has the problem and would like to do a new design like that.

Maybe we can have a new API to get a user's all auth sources so it will look compatible with backend.
Sorry for the confusions.

@tobiasbp
Copy link
Contributor Author

tobiasbp commented Apr 4, 2024

I think it will not be in next version of Gitea but we know it has the problem and would like to do a new design like that.

Hmmm.... I'm now really confused @lunny . I think you are saying one of these things:

  1. The next release of Gitea (1.22.0) will have the use of authentication sources redesigned in a way that will allow users to have more than one authentication source. This feature changes the API in way that will make this PR obsolete. We should reject this PR.

  2. Let's update the API for Gitea 1.22.0 in this request to add an endpoint that will return a users authentication source in a format that will be compatible with the plans to introduce the feature of users having several authentication sources in a version of Gitea beyond 1.22.0.

Thanks

@tobiasbp
Copy link
Contributor Author

tobiasbp commented Apr 9, 2024

Sorry to be pushy, but what are we going to do about this PR? I think it's quite the oversight, that you can currently use the API to set the source_id for a user, but not retrieve it. @lunny @delvh @techknowlogick

@techknowlogick
Copy link
Member

Yes, I agree. I think if we have a set, we need to have a get. And if/when a refactor happens then it'll also affect the set, so it'd already be breaking, so adding the get wouldn't affect things.
And thinking back about it, while a rename on the key would be nice, since it aligns with an already existing key for the set, then it would be probably best to keep the key name the same.

@tobiasbp
Copy link
Contributor Author

Yes, I agree. I think if we have a set, we need to have a get. And if/when a refactor happens then it'll also affect the set, so it'd already be breaking, so adding the get wouldn't affect things. And thinking back about it, while a rename on the key would be nice, since it aligns with an already existing key for the set, then it would be probably best to keep the key name the same.

Sounds good. Can you approve the PR please?

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged labels Apr 15, 2024
@techknowlogick techknowlogick added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Apr 15, 2024
@lunny lunny enabled auto-merge (squash) April 16, 2024 05:14
@lunny lunny merged commit 58b204b into go-gitea:main Apr 16, 2024
26 checks passed
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Apr 16, 2024
zjjhot added a commit to zjjhot/gitea that referenced this pull request Apr 17, 2024
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
357aec6 
* giteaofficial/main:
  Reduce unnecessary database queries on actions table (go-gitea#30509)
  [skip ci] Updated translations via Crowdin
  Tweak and fix toggle checkboxes (go-gitea#30527)
  Tweak repo buttons on mobile and labeled button border-radius (go-gitea#30503)
  Fix long branch name overflows (go-gitea#30345)
  Update API to return 'source_id' for users (go-gitea#29718)
  Allow `preferred_username` as username source for OIDC (go-gitea#30454)
  Fix empty field `login_name` in API response JSON when creating user (go-gitea#30511)
  feat(api): implement branch/commit comparison API (go-gitea#30349)
@wxiaoguang wxiaoguang modified the milestones: 1.23.0, 1.22.0 Apr 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lunny lunny lunny approved these changes

@techknowlogick techknowlogick techknowlogick 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/api This PR adds API routes or modifies them modifies/go Pull requests that update Go code size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. type/enhancement An improvement of existing functionality
Projects
None yet
Milestone
1.22.0
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@tobiasbp @lunny @techknowlogick @delvh @wxiaoguang @GiteaBot