From 331f437be5cb4c0af614e2ac631e2365f43e9d10 Mon Sep 17 00:00:00 2001
From: Nicholas Tsim <nicholastsim@gmail.com>
Date: Wed, 12 Sep 2018 21:14:38 +0100
Subject: [PATCH 1/2] Add `provider` query parameter for `/auth` endpoint

This commit adds the `provider` query parameter to allow users to select
the specific authentication provider they wish to use when authenticating
through Staticman.

This is useful in situations where the site repository is hosted on a
different provider to the authentication provider. For example, if the
site is hosted on GitHub, but the commenter wishes to sign in through
GitLab. Consequently, this is a prerequisite to adding additional
authentication providers in the future.
---
 controllers/auth.js                |  6 +++---
 coverage/cobertura-coverage.xml    |  2 +-
 test/unit/controllers/auth.test.js | 18 ++++++++++++------
 3 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/controllers/auth.js b/controllers/auth.js
index ab02c80b..27e11f43 100644
--- a/controllers/auth.js
+++ b/controllers/auth.js
@@ -11,7 +11,7 @@ module.exports = (req, res) => {
 
   let requestAccessToken
 
-  switch (req.params.service) {
+  switch (req.query.provider) {
     case 'gitlab':
       requestAccessToken = siteConfig =>
         oauth.requestGitLabAccessToken(
@@ -34,12 +34,12 @@ module.exports = (req, res) => {
   return staticman.getSiteConfig()
     .then(requestAccessToken)
     .then((accessToken) => {
-      const git = gitFactory.create(req.params.service, {
+      const git = gitFactory.create(req.query.provider, {
         oauthToken: accessToken
       })
 
       // TODO: Simplify this when v2 support is dropped.
-      const getUser = req.params.version === '2' && req.params.service === 'github'
+      const getUser = req.params.version === '2' && req.query.provider === 'github'
         ? git.api.users.get({}).then(({data}) => data)
         : git.getCurrentUser()
 
diff --git a/coverage/cobertura-coverage.xml b/coverage/cobertura-coverage.xml
index 9fb167bb..5a2f94c3 100644
--- a/coverage/cobertura-coverage.xml
+++ b/coverage/cobertura-coverage.xml
@@ -1,6 +1,6 @@
 <?xml version="1.0" ?>
 <!DOCTYPE coverage SYSTEM "http://cobertura.sourceforge.net/xml/coverage-04.dtd">
-<coverage lines-valid="786" lines-covered="686" line-rate="0.8728" branches-valid="360" branches-covered="278" branch-rate="0.7722" timestamp="1534078804178" complexity="0" version="0.1">
+<coverage lines-valid="786" lines-covered="686" line-rate="0.8728" branches-valid="360" branches-covered="278" branch-rate="0.7722" timestamp="1536783029286" complexity="0" version="0.1">
   <sources>
     <source>/home/nick/Development/Javascript/staticman</source>
   </sources>
diff --git a/test/unit/controllers/auth.test.js b/test/unit/controllers/auth.test.js
index 61e2fdfe..8a90e127 100644
--- a/test/unit/controllers/auth.test.js
+++ b/test/unit/controllers/auth.test.js
@@ -50,7 +50,8 @@ describe('Auth controller', () => {
 
       const reqWithQuery = Object.assign({}, req, {
         query: {
-          code: mockCode
+          code: mockCode,
+          provider: 'github'
         }
       })
 
@@ -96,7 +97,8 @@ describe('Auth controller', () => {
           version: '2'
         },
         query: {
-          code: mockCode
+          code: mockCode,
+          provider: 'github'
         }
       })
 
@@ -167,7 +169,8 @@ describe('Auth controller', () => {
 
       const reqWithQuery = Object.assign({}, req, {
         query: {
-          code: mockCode
+          code: mockCode,
+          provider: 'github'
         }
       })
 
@@ -217,7 +220,8 @@ describe('Auth controller', () => {
           service: 'gitlab'
         },
         query: {
-          code: mockCode
+          code: mockCode,
+          provider: 'gitlab'
         }
       })
 
@@ -251,7 +255,8 @@ describe('Auth controller', () => {
           service: 'gitlab'
         },
         query: {
-          code: mockCode
+          code: mockCode,
+          provider: 'gitlab'
         }
       })
 
@@ -296,7 +301,8 @@ describe('Auth controller', () => {
           service: 'gitlab'
         },
         query: {
-          code: mockCode
+          code: mockCode,
+          provider: 'gitlab'
         }
       })
 

From 8ba2ea3136b067de08695e2bedc91dc5a7572aaf Mon Sep 17 00:00:00 2001
From: Nicholas Tsim <nicholastsim@gmail.com>
Date: Wed, 12 Sep 2018 21:29:06 +0100
Subject: [PATCH 2/2] Cleanup `auth` tests removing any excessive copy/paste

---
 coverage/cobertura-coverage.xml    |  32 ++++-----
 test/unit/controllers/auth.test.js | 109 ++++++++---------------------
 2 files changed, 46 insertions(+), 95 deletions(-)

diff --git a/coverage/cobertura-coverage.xml b/coverage/cobertura-coverage.xml
index 5a2f94c3..09ec016a 100644
--- a/coverage/cobertura-coverage.xml
+++ b/coverage/cobertura-coverage.xml
@@ -1,6 +1,6 @@
 <?xml version="1.0" ?>
 <!DOCTYPE coverage SYSTEM "http://cobertura.sourceforge.net/xml/coverage-04.dtd">
-<coverage lines-valid="786" lines-covered="686" line-rate="0.8728" branches-valid="360" branches-covered="278" branch-rate="0.7722" timestamp="1536783029286" complexity="0" version="0.1">
+<coverage lines-valid="786" lines-covered="686" line-rate="0.8728" branches-valid="360" branches-covered="278" branch-rate="0.7722" timestamp="1536784015619" complexity="0" version="0.1">
   <sources>
     <source>/home/nick/Development/Javascript/staticman</source>
   </sources>
@@ -669,9 +669,9 @@
                 <line number="10" hits="20"/>
               </lines>
             </method>
-            <method name="(anonymous_1)" hits="61" signature="()V">
+            <method name="(anonymous_1)" hits="64" signature="()V">
               <lines>
-                <line number="13" hits="61"/>
+                <line number="13" hits="64"/>
               </lines>
             </method>
             <method name="(anonymous_2)" hits="10" signature="()V">
@@ -778,12 +778,12 @@
             <line number="7" hits="53" branch="false"/>
             <line number="8" hits="53" branch="false"/>
             <line number="10" hits="53" branch="false"/>
-            <line number="14" hits="61" branch="false"/>
-            <line number="16" hits="61" branch="false"/>
-            <line number="25" hits="61" branch="true" condition-coverage="100% (2/2)"/>
+            <line number="14" hits="64" branch="false"/>
+            <line number="16" hits="64" branch="false"/>
+            <line number="25" hits="64" branch="true" condition-coverage="100% (2/2)"/>
             <line number="26" hits="4" branch="false"/>
-            <line number="30" hits="57" branch="true" condition-coverage="100% (2/2)"/>
-            <line number="31" hits="56" branch="false"/>
+            <line number="30" hits="60" branch="true" condition-coverage="100% (2/2)"/>
+            <line number="31" hits="59" branch="false"/>
             <line number="36" hits="1" branch="false"/>
             <line number="41" hits="10" branch="false"/>
             <line number="48" hits="1" branch="false"/>
@@ -815,9 +815,9 @@
         </class>
         <class name="GitLab.js" filename="lib/GitLab.js" line-rate="0.8649" branch-rate="0.6923">
           <methods>
-            <method name="(anonymous_0)" hits="24" signature="()V">
+            <method name="(anonymous_0)" hits="21" signature="()V">
               <lines>
-                <line number="11" hits="24"/>
+                <line number="11" hits="21"/>
               </lines>
             </method>
             <method name="(anonymous_1)" hits="15" signature="()V">
@@ -928,11 +928,11 @@
             <line number="6" hits="47" branch="false"/>
             <line number="7" hits="47" branch="false"/>
             <line number="8" hits="47" branch="false"/>
-            <line number="12" hits="24" branch="false"/>
-            <line number="14" hits="24" branch="true" condition-coverage="100% (2/2)"/>
+            <line number="12" hits="21" branch="false"/>
+            <line number="14" hits="21" branch="true" condition-coverage="100% (2/2)"/>
             <line number="15" hits="3" branch="false"/>
-            <line number="19" hits="21" branch="true" condition-coverage="100% (2/2)"/>
-            <line number="20" hits="20" branch="false"/>
+            <line number="19" hits="18" branch="true" condition-coverage="100% (2/2)"/>
+            <line number="20" hits="17" branch="false"/>
             <line number="25" hits="1" branch="false"/>
             <line number="30" hits="15" branch="true" condition-coverage="75% (3/4)"/>
             <line number="31" hits="15" branch="false"/>
@@ -1093,8 +1093,8 @@
             <line number="4" hits="77" branch="false"/>
             <line number="6" hits="77" branch="false"/>
             <line number="7" hits="99" branch="true" condition-coverage="100% (2/2)"/>
-            <line number="9" hits="8" branch="false"/>
-            <line number="11" hits="91" branch="false"/>
+            <line number="9" hits="5" branch="false"/>
+            <line number="11" hits="94" branch="false"/>
           </lines>
         </class>
         <class name="Logger.js" filename="lib/Logger.js" line-rate="0" branch-rate="0">
diff --git a/test/unit/controllers/auth.test.js b/test/unit/controllers/auth.test.js
index 8a90e127..18f25743 100644
--- a/test/unit/controllers/auth.test.js
+++ b/test/unit/controllers/auth.test.js
@@ -18,17 +18,10 @@ beforeEach(() => {
 
 describe('Auth controller', () => {
   describe('GitHub', () => {
-    test('authenticates to GitHub with the given code and returns the authenticated user', () => {
-      const mockAccessToken = 'qwertyuiop'
-      const mockCode = '1q2w3e4r'
-      const mockUser = {
-        login: 'johndoe',
-        name: 'John Doe',
-        email: 'johndoe@test.com'
-      }
-
-      const siteConfig = helpers.getConfig()
+    const siteConfig = helpers.getConfig()
+    const mockCode = '1q2w3e4r'
 
+    const oauthRequest =
       nock(/github\.com/)
         .post('/login/oauth/access_token')
         .query({
@@ -37,6 +30,16 @@ describe('Auth controller', () => {
           code: mockCode,
           redirect_uri: siteConfig.get('githubAuth.redirectUri')
         })
+
+    test('authenticates to GitHub with the given code and returns the authenticated user', () => {
+      const mockAccessToken = 'qwertyuiop'
+      const mockUser = {
+        login: 'johndoe',
+        name: 'John Doe',
+        email: 'johndoe@test.com'
+      }
+
+      oauthRequest
         .reply(200, {
           access_token: mockAccessToken
         })
@@ -65,21 +68,11 @@ describe('Auth controller', () => {
 
     test('authenticates to GitHub with the given code and returns the original GitHub user when using v2 API', () => {
       const mockAccessToken = 'qwertyuiop'
-      const mockCode = '1q2w3e4r'
       const mockUser = {
         login: 'johndoe'
       }
 
-      const siteConfig = helpers.getConfig()
-
-      nock(/github\.com/)
-        .post('/login/oauth/access_token')
-        .query({
-          client_id: siteConfig.get('githubAuth.clientId'),
-          client_secret: siteConfig.get('githubAuth.clientSecret'),
-          code: mockCode,
-          redirect_uri: siteConfig.get('githubAuth.redirectUri')
-        })
+      oauthRequest
         .reply(200, {
           access_token: mockAccessToken
         })
@@ -93,7 +86,6 @@ describe('Auth controller', () => {
 
       const reqWithQuery = Object.assign({}, req, {
         params: {
-          service: 'github',
           version: '2'
         },
         query: {
@@ -110,25 +102,12 @@ describe('Auth controller', () => {
     })
 
     test('returns a 401 response when unable to get an access token from GitHub', () => {
-      const mockCode = '1q2w3e4r'
-      const siteConfig = helpers.getConfig()
-
-      nock(/github\.com/)
-        .post('/login/oauth/access_token')
-        .query({
-          client_id: siteConfig.get('githubAuth.clientId'),
-          client_secret: siteConfig.get('githubAuth.clientSecret'),
-          code: mockCode,
-          redirect_uri: siteConfig.get('githubAuth.redirectUri')
-        })
+      oauthRequest
         .reply(401, {
           error: 'invalid_code'
         })
 
       const reqWithQuery = Object.assign({}, req, {
-        params: {
-          service: 'github'
-        },
         query: {
           code: mockCode
         }
@@ -183,17 +162,10 @@ describe('Auth controller', () => {
   })
 
   describe('GitLab', () => {
-    test('authenticates to GitLab with the given code and returns the authenticated user', () => {
-      const mockAccessToken = 'qwertyuiop'
-      const mockCode = '1q2w3e4r'
-      const mockUser = {
-        username: 'johndoe',
-        name: 'John Doe',
-        email: 'johndoe@test.com'
-      }
-
-      const siteConfig = helpers.getConfig()
+    const siteConfig = helpers.getConfig()
+    const mockCode = '1q2w3e4r'
 
+    const oauthRequest =
       nock(/gitlab\.com/)
         .post('/oauth/token')
         .query({
@@ -203,6 +175,16 @@ describe('Auth controller', () => {
           grant_type: 'authorization_code',
           redirect_uri: siteConfig.get('gitlabAuth.redirectUri')
         })
+
+    test('authenticates to GitLab with the given code and returns the authenticated user', () => {
+      const mockAccessToken = 'qwertyuiop'
+      const mockUser = {
+        username: 'johndoe',
+        name: 'John Doe',
+        email: 'johndoe@test.com'
+      }
+
+      oauthRequest
         .reply(200, {
           access_token: mockAccessToken
         })
@@ -216,9 +198,6 @@ describe('Auth controller', () => {
         .reply(200, mockUser)
 
       const reqWithQuery = Object.assign({}, req, {
-        params: {
-          service: 'gitlab'
-        },
         query: {
           code: mockCode,
           provider: 'gitlab'
@@ -234,26 +213,12 @@ describe('Auth controller', () => {
     })
 
     test('returns a 401 response when unable to get an access token from GitLab', () => {
-      const mockCode = '1q2w3e4r'
-      const siteConfig = helpers.getConfig()
-
-      nock(/gitlab\.com/)
-        .post('/oauth/token')
-        .query({
-          client_id: siteConfig.get('gitlabAuth.clientId'),
-          client_secret: siteConfig.get('gitlabAuth.clientSecret'),
-          code: mockCode,
-          grant_type: 'authorization_code',
-          redirect_uri: siteConfig.get('gitlabAuth.redirectUri')
-        })
+      oauthRequest
         .reply(401, {
           error: 'invalid_code'
         })
 
       const reqWithQuery = Object.assign({}, req, {
-        params: {
-          service: 'gitlab'
-        },
         query: {
           code: mockCode,
           provider: 'gitlab'
@@ -269,19 +234,8 @@ describe('Auth controller', () => {
 
     test('returns a 401 response when an incorrect access token is used for the GitLab API', () => {
       const mockAccessToken = 'qwertyuiop'
-      const mockCode = '1q2w3e4r'
-
-      const siteConfig = helpers.getConfig()
 
-      nock(/gitlab\.com/)
-        .post('/oauth/token')
-        .query({
-          client_id: siteConfig.get('gitlabAuth.clientId'),
-          client_secret: siteConfig.get('gitlabAuth.clientSecret'),
-          code: mockCode,
-          grant_type: 'authorization_code',
-          redirect_uri: siteConfig.get('gitlabAuth.redirectUri')
-        })
+      oauthRequest
         .reply(200, {
           access_token: mockAccessToken
         })
@@ -297,9 +251,6 @@ describe('Auth controller', () => {
         })
 
       const reqWithQuery = Object.assign({}, req, {
-        params: {
-          service: 'gitlab'
-        },
         query: {
           code: mockCode,
           provider: 'gitlab'