-
-
Notifications
You must be signed in to change notification settings - Fork 5.8k
UX treat register like sign in, even oauth #5033
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
Conversation
8178780
to
2bdc146
Compare
Codecov Report
@@ Coverage Diff @@
## master #5033 +/- ##
==========================================
+ Coverage 41.15% 41.17% +0.01%
==========================================
Files 464 464
Lines 62763 62757 -6
==========================================
+ Hits 25833 25841 +8
+ Misses 33537 33521 -16
- Partials 3393 3395 +2
Continue to review full report at Codecov.
|
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. |
2bdc146
to
c003070
Compare
Rebased on master and ready for re-review! |
bump |
|
||
// This will link the account in such a way that it cannot be removed | ||
// TODO why is this different from normal linking? | ||
if setting.Service.AllowOnlyExternalRegistration { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is necessary. setting.Service.AllowOnlyExternalRegistration
means user must register via external source. Since we have been in LinkAccountPostRegister
. The check is unnecessary.
if err := models.UpdateUserCols(u, "is_admin", "is_active", "last_login_unix"); err != nil { | ||
ctx.ServerError("UpdateUser", err) | ||
// This will link the account in such a way that it can be removed | ||
if !setting.Service.AllowOnlyExternalRegistration { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above
@solderjs could you remove |
@lunny I think they might actually integral to the purpose of this pr. From what I understand the purpose is about being able to remove the link of Gitea user from the external source. |
Whether this is the correct technique to do that is another question. I suspect we should be asking at the time of attempted removal of external links whether it's reasonable to do so. |
The goal is to be able to create a secure account that doesn't have a password that can be used as a backdoor to gain access. If a local account is required, it thwarts the intent. |
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. |
Need to resolve conflicts |
It's stale for long time. What's the status of this PR? |
Re: #4226, #5032,
ALLOW_ONLY_EXTERNAL_REGISTRATION = true
remember me
on sign up pageIf you'd like to test it out
My pull request is against
master
, but I run it as backport to v1.5.1 (includes #5006, #5029, #5033):I would not recommend replacing your existing gitea, but rather creating a symlink so that you can easily switch back if you don't like it. For example, if you keep
gitea
in/opt/gitea/bin
:rsync -av ./gitea /opt/gitea/bin/gitea-v1.5.1-coolaj86 pushd /opt/gitea/bin mv gitea gitea-v1.5.1 ln -s gitea-v1.5.1-coolaj86 gitea
I've run a couple of manual tests so far, so I feel comfortable with someone else trying it out. I won't be pushing any additional changes to that branch (such as the upcoming changes to address the empty checkboxes in the issue) until I've tested them in production for myself.