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

Add setting to disable user features when user login type is not plain #29615

Merged
merged 15 commits into from Mar 29, 2024
Merged

Add setting to disable user features when user login type is not plain #29615

merged 15 commits into from Mar 29, 2024

Conversation

jackHay22
Copy link
Contributor

@jackHay22 jackHay22 commented Mar 5, 2024 •
edited

Changes

  • Adds setting EXTERNAL_USER_DISABLE_FEATURES to disable any supported user features when login type is not plain
    • In general, this is necessary for SSO implementations to avoid inconsistencies between the external account management and the linked account
  • Adds helper functions to encourage correct use

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 5, 2024
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 5, 2024
@github-actions github-actions bot added the modifies/api This PR adds API routes or modifies them label Mar 5, 2024
This was referenced Mar 5, 2024
Draft
Merged
@lunny
Copy link
Member

lunny commented Mar 7, 2024

But I don't think password is meaningful for external users.

@jackHay22
Copy link
Contributor Author

But I don't think password is meaningful for external users.

Do you mean that changing a password should be disabled? I agree but I'm not sure if that's directly related to the changes here.

@lunny
Copy link
Member

lunny commented Mar 7, 2024

What if the user system needs to be refactored in future that a user will always be a plain one and can linked to external account?

@jackHay22
Copy link
Contributor Author

What if the user system needs to be refactored in future that a user will always be a plain one and can linked to external account?

So checking LoginType wouldn't be the best way to check if the account is linked to an external provider? Should we instead check for an ExternalLoginUser?

@lunny
Copy link
Member

lunny commented Mar 7, 2024

What if the user system needs to be refactored in future that a user will always be a plain one and can linked to external account?

So checking LoginType wouldn't be the best way to check if the account is linked to an external provider? Should we instead check for an ExternalLoginUser?

Currently I think check LoginType is OK because not every external accounts have been saved into ExternalLoginUser. But we need to have a comment here, once all external login users have been stored, then we can check the table.

lunny
lunny reviewed Mar 7, 2024
View reviewed changes
custom/conf/app.example.ini Outdated Show resolved Hide resolved
@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 7, 2024
@lunny lunny added the type/enhancement An improvement of existing functionality label Mar 7, 2024
@lunny
Copy link
Member

lunny commented Mar 7, 2024

And please add documents and tests.

@jackHay22 jackHay22 marked this pull request as draft March 13, 2024 19:48
@jackHay22
Copy link
Contributor Author

I think it would be better to change EXTERNAL_USER_DISABLE_ALL_FEATURES to EXTERNAL_USER_DISABLE_FEATURES and allow selective inclusion.

@pull-request-size pull-request-size bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 26, 2024
@jackHay22 jackHay22 marked this pull request as ready for review March 26, 2024 13:34
@github-actions github-actions bot added the modifies/go Pull requests that update Go code label Mar 26, 2024
@GiteaBot GiteaBot added lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Mar 26, 2024
silverwind
silverwind reviewed Mar 28, 2024
View reviewed changes
models/user/user.go Outdated Show resolved Hide resolved
@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 28, 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 28, 2024
@silverwind silverwind merged commit 59d4aad into go-gitea:main Mar 29, 2024
26 checks passed
@GiteaBot GiteaBot added this to the 1.23.0 milestone Mar 29, 2024
zjjhot added a commit to zjjhot/gitea that referenced this pull request Mar 30, 2024
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
7a8a44a 
* giteaofficial/main:
  Include encoding in signature payload (go-gitea#30174)
  Add `stylelint-value-no-unknown-custom-properties` and convert stylelint config to js (go-gitea#30117)
  Remove jQuery class from the commit button (go-gitea#30178)
  Remove jQuery class from the diff view (go-gitea#30176)
  Remove jQuery class from the notification count (go-gitea#30172)
  Remove jQuery class from the code range selection (go-gitea#30173)
  Fix:the rounded corners of the folded file are not displayed correctly (go-gitea#29953)
  Add setting to disable user features when user login type is not plain (go-gitea#29615)

# Conflicts:
#	models/user/user.go
@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

@silverwind silverwind silverwind approved these changes

@techknowlogick techknowlogick techknowlogick approved these changes

@lunny lunny Awaiting requested review from lunny

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/docs modifies/go Pull requests that update Go code size/L Denotes a PR that changes 100-499 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
@jackHay22 @lunny @silverwind @techknowlogick @wxiaoguang @GiteaBot