Page MenuHomePhabricator

Don't actually construct auth providers when checking for their existence
ClosedPublic

Authored by epriestley on Mar 4 2014, 8:35 PM.
Tags
None
Referenced Files
F14045413: D8395.diff
Wed, Nov 13, 3:50 AM
F14034080: D8395.diff
Sat, Nov 9, 10:02 PM
F13994109: D8395.id19951.diff
Wed, Oct 23, 3:52 AM
F13984982: D8395.diff
Sun, Oct 20, 5:20 PM
F13984157: D8395.id19945.diff
Sun, Oct 20, 11:21 AM
F13975424: D8395.diff
Fri, Oct 18, 10:03 AM
Unknown Object (File)
Oct 13 2024, 5:42 AM
Unknown Object (File)
Oct 1 2024, 3:18 PM
Subscribers

Details

Summary

A user reported this stack trace:

http://pastebin.com/6auGbZsE

...on this GitHub issue:

https://github.com/facebook/phabricator/issues/389#issuecomment-36612511

The problem is similar to the original report, but not identical. In this case, we're following a sequence of steps like:

  • Run setup checks.
    • Check for enabled providers, in order to raise "no providers configured yet" warning.
      • Try to generate login/redirect URIs.
  • Build the request.
  • Set the default base URI.
  • Run normal code.

Since we try to generate URIs before we provide a default, this fatals. Instead, don't try to build objects.

An alternative fix might be to try to set defaults earlier, but we depend on some config and on building the Request in order to be able to figure out if a request is HTTP or HTTPS right now. We could assume one, or guess, or use protocol-relative URIs (///host.com), but I think this fix is a little cleaner overall. If we keep hitting similar stuff, we could look into alternate fixes.

We could also set some kind of "setup mode" flag and make getURI() if it's called during setup mode to detect these during testing. I'd like to hit one more of these before doing that, though.

Test Plan

Reproduced the issue, applied the patch, verified this fixes it.

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

btrahan edited edge metadata.
btrahan added inline comments.
src/applications/config/check/PhabricatorSetupCheckAuth.php
14

s/ever/never

This revision is now accepted and ready to land.Mar 5 2014, 12:05 AM
src/applications/config/check/PhabricatorSetupCheckAuth.php
14

Oh, "ever" is actually intended ("If they've found their way to the app and configured something, we don't need to give them a hint that the app exists, since they already know it exists."). I'll rephrase this to be more clear.

epriestley updated this revision to Unknown Object (????).Mar 5 2014, 12:11 AM
  • Clarify comment.