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.

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

Allow disabling authentication related user features #31535

Conversation

bohde
Copy link
Contributor

@bohde bohde commented Jul 1, 2024

We have some instances that only allow using an external authentication source for authentication. In this case, users changing their email, password, or linked OpenID connections will not have any effect, and we'd like to prevent showing that to them to prevent confusion.

Included in this are several changes to support this:

  • A new setting to disable user managed authentication credentials (email, password & OpenID connections)
  • A new setting to disable user managed MFA (2FA codes & WebAuthn)
  • Fix an issue where some templates had separate logic for determining if a feature was disabled since it didn't check the globally disabled features
  • Hide more user setting pages in the navbar when their settings aren't enabled

We have some instances that only allow using an external
authentication source for authentication. In this case, users changing
their email, password, or linked OpenID connections will not have any
effect, and we'd like to prevent showing that to them to prevent confusion.

Included in this are several changes to support this:
* A new setting to disable user managed authentication credentials (email, password & OpenID connections)
* A new setting to disable user managed MFA (2FA codes & WebAuthn)
* Fix an issue where some templates had separate logic for determining if a feature was disabled since it didn't check the globally disabled features
* Hide more user setting pages in the navbar when their settings aren't enabled
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 1, 2024
@github-actions github-actions bot added modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files modifies/docs labels Jul 1, 2024
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 1, 2024
@lunny lunny added this to the 1.23.0 milestone Jul 2, 2024
Copy link
Contributor

@kdumontnu kdumontnu left a comment

Choose a reason for hiding this comment

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

@bohde do you think it's worth updating any of the integration tests here? For example, it might be worth validating we see the "Not Found" response when updating user password.

{{template "user/settings/security/twofa" .}}
{{template "user/settings/security/webauthn" .}}
{{end}}
{{if not ($.UserDisabledFeatures.Contains "manage_credentials")}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we disable only when both 'manage_credentials' and 'deleted' are true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is the reasoning behind that? Is it to prevent them from unlinking their primary account?

Copy link
Contributor

Choose a reason for hiding this comment

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

What is the reasoning behind that? Is it to prevent them from unlinking their primary account?

I think I referred to the wrong file. This comment is for navbar.tmpl, and you have already added it in commit 3df7358.

And there is another bug: when I click Blocked users, the Account option will appear again, even though manage_credentials and deletion are true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching that! It should be fixed in c9487b4.

add integration tests for navbar and all disabled user settings
@pull-request-size pull-request-size bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 8, 2024
@bohde
Copy link
Contributor Author

bohde commented Jul 8, 2024

@bohde do you think it's worth updating any of the integration tests here? For example, it might be worth validating we see the "Not Found" response when updating user password.

Added integration tests in 3df7358

Copy link
Contributor

@kdumontnu kdumontnu left a comment

Choose a reason for hiding this comment

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

🥇

@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 Jul 8, 2024
Copy link
Member

@techknowlogick techknowlogick left a comment

Choose a reason for hiding this comment

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

🎉

@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 Jul 9, 2024
@techknowlogick techknowlogick enabled auto-merge (squash) July 9, 2024 17:02
@techknowlogick
Copy link
Member

Thanks! This will be useful for additional use cases too, as I was starting work on disabling changing user visibility and can leverage your work (this will aid in spam prevention so accounts potentially flagged as spam can be hidden until they can be reviewed by a human)

@techknowlogick techknowlogick merged commit 1ee59f0 into go-gitea:main Jul 9, 2024
26 checks passed
@lunny
Copy link
Member

lunny commented Jul 10, 2024

missed doc has been pushed to https://gitea.com/gitea/docs/pulls/25

zjjhot added a commit to zjjhot/gitea that referenced this pull request Jul 10, 2024
* giteaofficial/main:
  Fix: Allow org team names of length 255 in create team form (go-gitea#31564)
  Remove docs sub folder since docs has been moved to https://gitea.com/gitea/docs (go-gitea#31536)
  Add bohde as maintainer (go-gitea#31601)
  Add `YEAR`, `MONTH`, `MONTH_ENGLISH`, `DAY` variables for template repos (go-gitea#31584)
  Allow disabling authentication related user features (go-gitea#31535)
  Add back esbuild-loader for .js files (go-gitea#31585)
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Oct 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/docs modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants