Page MenuHomePhabricator
Create Task
Maniphest T13154

Public tasks not displayed when logged in under a not-yet-approved user account
Closed, WontfixPublic
Actions

Description

Steps to reproduce:

  1. Under /config/all/, set both policy.allow-public and auth.require-approval to true.
  2. Register a new user account.
  3. Get "Your account has been created, but needs to be approved by an administrator. You'll receive an email once your account is approved."
  4. Try to access a public task (visibility: "Public (No Login Required)"), in the default (public) Space.

Expected outcome:
You can access and view the task (though not edit or comment).

Actual outcome:
Task not displayed; instead you see "Wait for Approval - Your account has been created, but needs to be approved by an administrator. You'll receive an email once your account is approved."

Version information:

Further information:

Related Objects

Event Timeline

aklapper created this task.Jun 18 2018, 12:20 PM
Herald added subscribers: eadler, qgil. · View Herald TranscriptJun 18 2018, 12:20 PM
aklapper updated the task description. (Show Details)Jun 18 2018, 12:48 PM
epriestley closed this task as Resolved.Jun 19 2018, 8:33 PM
epriestley claimed this task.
epriestley added a subscriber: epriestley.

It's intentional that unapproved accounts have less access than logged-out accounts in some configurations.

If we give them access like public accounts, we need to special case a number of UIs that currently say "Log in to <take this action>" to possibly say "Wait for approval to <take this action>". Obviously, this isn't impossible, but it adds complexity to a very rare workflow and generally makes changes more difficult to test: a developer would now need to test logged-in, logged-out, and needs-approval states when building a new interface. This is also only relevant if an install allows public access: this roadblock state is already about the most reasonable thing we can do for non-public installs.

In general, there are a large set of account states with potentially less access than logged-out accounts. These states include: disabled accounts; accounts which haven't signed required Legalpad documents; accounts which haven't configured MFA; unapproved accounts; accounts with unverified email addresses on installs where email verification is required; and accounts with more than one of those states. In all cases, users can potentially access more information by logging out (notably, if the install is public). This is somewhat counterintuitive but simplifies handling these states -- and they already don't get enough testing as-is (see T13024 for a series of recent issues where certain unusual account flags could conflict destructively).

If we had infinite resources, sure, we'd probably special case some of these states into read-only workflows with tailored "Log in to <take this action>", "Wait for approval to <take this action>", "Sign required documents to <take this action>", "Configure MFA to <take this action>", "Sign required documents, configure MFA, verify your email address, then wait for approval to <take this action>", etc., states. But we don't have infinite resources and this affects only affects a small portion of users (users on public installs with approval required), usually fairly briefly (only until their account is approved), so it's hard to imagine this change ever making it high enough on the priority list to get implementation in the upstream.

(I believe the minimal version of this change is very complex. As a very rough proxy of complexity, there are 107 isLoggedIn() callsites today; these are more or less the callsites which would need to be reviewed and modified with additional logic to handle logged-in-but-not-activated accounts in a special way.)

epriestley changed the task status from Resolved to Wontfix.Jun 19 2018, 8:34 PM

(I suppose "wontfix" is a more appropriate resolution.)

aklapper added a comment.Jun 19 2018, 8:39 PM

Thanks for the great explanation and the pointers!

epriestley added a comment.Jun 21 2018, 12:05 PM

As a possible workaround, a trivial diff which might "fix" this problem is to change the dialog to tell users that they may get more access while they wait by logging out:

diff --git a/src/applications/auth/controller/PhabricatorAuthNeedsApprovalController.php b/src/applications/auth/controller/PhabricatorAuthNeedsApprovalController.php
index ba2881637..cf92c5b74 100644
--- a/src/applications/auth/controller/PhabricatorAuthNeedsApprovalController.php
+++ b/src/applications/auth/controller/PhabricatorAuthNeedsApprovalController.php
@@ -20,7 +20,9 @@ final class PhabricatorAuthNeedsApprovalController
 
     $wait_for_approval = pht(
       "Your account has been created, but needs to be approved by an ".
-      "administrator. You'll receive an email once your account is approved.");
+      "administrator. You'll receive an email once your account is ".
+      "approved. Until your account is approved, you can continue to browse ".
+      "tasks by logging out (or using an incognito browser window).");
 
     $dialog = id(new AphrontDialogView())
       ->setUser($viewer)

Some similar change can probably be made in the translation.override config setting without actually touching the code.

Phabricator · Built by Phacility