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

check if signups are allowed during SignUpPost #5437

Closed
wants to merge 1 commit into from

Conversation

r-52
Copy link

@r-52 r-52 commented Nov 30, 2018

Previously you could hide the sign up page with the following
configs:

[service]
SHOW_REGISTRATION_BUTTON          = false

This would remove the sign up button from the navbar, but the user could still access the page with a direct link. During the http post action, the go action checked if the key SHOW_REGISTRATION_BUTTON was set to true. If not, the user received a 403.

You can use this scenario for a semi-hidden sign up page that is only accessible through a direct link.

I've changed the check in this PR to check if the sign up is allowed or not. The user can access the sign up page (if it's enabled) and sign up even if the value of SHOW_REGISTRATION_BUTTON is false, the user can sign up.

Another solution could be:

  • unify SHOW_REGISTRATION_BUTTON with DISABLE_REGISTRATION to
    only show the registration page if it's enabled, but that would dis-
    allow the mentioned scenario

fixes: #5183

Previously you could hide the sign up page with the following
configs:

```
[service]
SHOW_REGISTRATION_BUTTON          = false
```

This would remove the sign up button from the navbar, but the user
could still access the page with a direct link. During the http post
action, the go action checked if the key `SHOW_REGISTRATION_BUTTON` was
set to true. If not, the user received a 403.

You can use this scenario for a semi-hidden sign up page that is only
accessible through a direct link.

I've changed the check in this PR to check if the sign up is allowed
or not. The user can access the sign up page (if it's enabled) and
sign up even if the value of `SHOW_REGISTRATION_BUTTON` is `false`, the
user can sign up.

Another solution could be:
- unify `SHOW_REGISTRATION_BUTTON ` with `DISABLE_REGISTRATION ` to
only show the registration page if it's enabled, but that would dis-
allow the mentioned scenario

fixes: go-gitea#5183
Signed-off-by: Roman <romaaan.git@gmail.com>
@codecov-io
Copy link

Codecov Report

Merging #5437 into master will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #5437   +/-   ##
=======================================
  Coverage   37.56%   37.56%           
=======================================
  Files         317      317           
  Lines       46821    46821           
=======================================
  Hits        17590    17590           
  Misses      26732    26732           
  Partials     2499     2499
Impacted Files Coverage Δ
routers/user/auth.go 13.28% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 06ef5b6...b33f71b. Read the comment docs.

@bkcsoft bkcsoft added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 30, 2018
@lunny lunny added the type/bug label Dec 1, 2018
@lunny lunny added this to the 1.7.0 milestone Dec 1, 2018
@kolaente
Copy link
Member

I see no point in why we have two settings which try to achive the same goal. From a security perspective, only hiding the button and still enabling sign ups is useless if the path is kept the same, an attacker could just try to visit gitea-instance.tld/register and only hiding a button to that page wont prevent him from doing so.

So, I'd prefer to uinon these two settings.

@lafriks lafriks modified the milestones: 1.7.0, 1.8.0 Dec 27, 2018
@stale
Copy link

stale bot commented Feb 25, 2019

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 months. Thank you for your contributions.

@stale stale bot added the issue/stale label Feb 25, 2019
@stale stale bot removed the issue/stale label Feb 25, 2019
@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 Feb 25, 2019
@lunny
Copy link
Member

lunny commented Feb 25, 2019

I think we can drop SHOW_REGISTRATION_BUTTON and always use DISABLE_REGISTRATION.

@zeripath
Copy link
Contributor

@lunny this PR doesn't actually drop SHOW_REGISTRATION_BUTTON - it's just that this PR disconnects SHOW_REGISTRATION_BUTTON and DISABLE_REGISTRATION - so if you set SHOW_REGISTRATION_BUTTON to false but leave DISABLE_REGISTRATION unset you can still register.

@romankl in order prevent a broken UI you need to change https://github.com/go-gitea/gitea/blob/master/modules/setting/service.go#L57 :

Service.ShowRegistrationButton = sec.Key("SHOW_REGISTRATION_BUTTON").MustBool(!(Service.DisableRegistration || Service.AllowOnlyExternalRegistration))

to

Service.ShowRegistrationButton = sec.Key("SHOW_REGISTRATION_BUTTON").MustBool(true) && !(Service.DisableRegistration || Service.AllowOnlyExternalRegistration)

So that DISABLE_REGISTRATION and ALLOW_ONLY_EXTERNAL_REGISTRATION properly override SHOW_REGISTRATION_BUTTON

Copy link
Contributor

@zeripath zeripath left a comment

Choose a reason for hiding this comment

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

See my comment above.

@techknowlogick techknowlogick modified the milestones: 1.8.0, 1.9.0 Mar 8, 2019
@stale
Copy link

stale bot commented May 7, 2019

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 months. Thank you for your contributions.

@stale stale bot added the issue/stale label May 7, 2019
@techknowlogick techknowlogick modified the milestones: 1.9.0, 1.10.0 Jun 17, 2019
@stale stale bot removed issue/stale labels Jun 17, 2019
@r-52 r-52 closed this Jul 19, 2019
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 1 This PR needs approval from one additional maintainer to be merged. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Registration fails when SHOW_REGISTRATION_BUTTON = false
9 participants