Better error handling, refactoring, documentation

README.md

@@ -203,10 +203,19 @@ modules: {
 
 > The login page is powered by Passport's `local` strategy, which is added to Apostrophe by the standard `apostrophe-login` module. That's why we disable it there and not in `apostrophe-passport`'s options.
 
+## Overriding the error page
+
+If login fails, for instance because you are matching on `email` but the `username` duplicates another account, or because a user is valid in Google but `emailDomain` does not match, the `error.html` template of the `apostrophe-passport` module is rendered. By default, it works, but it's pretty ugly! You'll want to customize it to your project's needs.
+
+Like other templates in Apostrophe, you can override this template by copying it to `lib/modules/apostrophe-passport/views/error.html` *in your project* (**do not modify the npm module itself**). You can then extend your own layout template and so on, just as you have most likely already done for the 404 Not Found page or the `login.html` page.
+
+## Frequently Asked Questions
+
 "What about redirecting `/login` to one of these fancy new strategies?"
 
 You can do that. Once the login page is gone, it's possible for you to decide what happens at that URL. Use the [apostrophe-redirects](https://npmjs.org/package/apostrophe-redirects) module to set it up through a nice UI, or add an Express route and a redirect in your own code.
 
 "I tried this with [passport strategy module X] and it didn't work."
 
-Feel free to open an issue but be sure to provide full specifics and a test project. Note that some strategies may not follow the standard practices this module is built upon. Those written by Jared Hanson or following his best practices should work well.
+Feel free to open an issue but be sure to provide full specifics and a test project. Note that some strategies may not follow the standard practices this module is built upon. Those written by Jared Hanson, the author of Passport, or following his best practices should work well. You might want to test directly with the sample code provided with that strategy module first, to rule out problems with the module or with your configuration of it.
+

index.js

@@ -47,18 +47,20 @@ module.exports = {
           ), self.findOrCreateUser(spec));
           spec.name = dummy.name;
         }
-        spec.options.callbackURL = self.getCallbackUrl(spec);
+        spec.options.callbackURL = self.getCallbackUrl(spec, true);
         self.apos.login.passport.use(new Strategy(spec.options, self.findOrCreateUser(spec)));
         self.addLoginRoute(spec);
         self.addCallbackRoute(spec);
+        self.addFailureRoute(spec);
       });
     };
 
     // Returns the oauth2 callback URL, which must match the route
-    // established by `addCallbackRoute`.
+    // established by `addCallbackRoute`. If `absolute` is true
+    // then `baseUrl` is prepended.
 
-    self.getCallbackUrl = function(spec) {
-      return self.apos.baseUrl + '/auth/' + spec.name + '/callback';        
+    self.getCallbackUrl = function(spec, absolute) {
+      return (absolute ? self.apos.baseUrl : '') + '/auth/' + spec.name + '/callback';        
     };
 
     self.getLoginUrl = function(spec) {
@@ -75,7 +77,7 @@ module.exports = {
     // Adds the oauth2 callback route, which is invoked 
 
     self.addCallbackRoute = function(spec) {
-      self.apos.app.get('/auth/' + spec.name + '/callback',
+      self.apos.app.get(self.getCallbackUrl(spec, false),
         // middleware
         self.apos.login.passport.authenticate(
           spec.name,
@@ -88,8 +90,17 @@ module.exports = {
       );
     };
 
+    self.addFailureRoute = function(spec) {
+      self.apos.app.get(self.getFailureUrl(spec), function(req, res) {
+        // Gets i18n'd in the template, also bc with what templates that tried to work
+        // before certain fixes would expect (this is why we still pass a string and not
+        // a flag, and why we call it `message`)
+        return self.sendPage(req, 'error', { spec: spec, message: 'Your credentials were not accepted, your account is not affiliated with this site, or an existing account has the same username or email address.' });
+      });
+    }
+
     self.getFailureUrl = function(spec) {
-      return '/';
+      return '/auth/' + spec.name + '/error';
     };
 
     // Given a strategy spec from the configuration, return
@@ -162,7 +173,10 @@ module.exports = {
           }
           return self.createUser(spec, profile, function(err, user) {
             if (err) {
-              return callback(err);
+              // Typically a duplicate key, not surprising with username and
+              // email address duplication possibilities when we're matching
+              // on the other field, treat it as a login error
+              return callback(null, false);
             }
             return callback(null, user);
           });
@@ -256,7 +270,7 @@ module.exports = {
       });
       console.log('\nThese are the callback URLs you may need to configure on sites:\n');
       _.each(self.options.strategies, function(spec) {
-        console.log(self.getCallbackUrl(spec));
+        console.log(self.getCallbackUrl(spec, true));
       });
       return callback(null);
     };

views/error.html

@@ -0,0 +1,20 @@
+{# You will almost certainly want to override this template to #}
+{# extend your own project's layout like your pages do, etc. #}
+
+{% extends data.outerLayout %}
+
+{% block main %}
+  <h2>A login error occurred</h2>
+  <p>
+    An error occurred while logging in via {{ __(data.spec.label or data.spec.name) }}:
+  </p>
+  <p>
+    {{ __(data.message) }}
+  </p>
+  <p>
+    If you believe you are seeing this message in error please contact the administrator.
+  </p>
+  <p>
+    <a href="/">Go Home</a>
+  </p>
+{% endblock %}