85 cleanup (#86)

* chore(cleanup): Cleanup code, docs and increased code coverage

close #85

CONTRIBUTING.md

@@ -30,7 +30,12 @@ This is a rough outline of what the workflow for code contributions looks like:
 - Check the list of open [issues](https://github.com/zalando-incubator/lib-oauth-tooling/issues). Either assign an existing issue to
 yourself, or create a new one that you would like work on and discuss your ideas and use cases.
 - Fork the repository
-- Create a topic branch from where you want to base your work. This is usually master.
+- Create a feature branch. Best practise for naming:
+
+```
+<branch name> = <Github issue ticket number>-<component-name>-<whatever-describes-the-ticket>
+```
+
 - Make commits of logical units.
 - Write good commit messages ([see below](#commit-messages)).
 - Push your changes to a topic branch in your fork of the repository.

README.md

@@ -254,4 +254,14 @@ The signature of `requireScopesMiddleware` is now incompatible with previous ver
 
 ## License
 
-MIT
+```
+MIT License (MIT)
+
+Copyright (c) 2016 Zalando SE
+
+Permission is hereby granted, free of charge, to any person obtaining a copy of this software and associated documentation files (the "Software"), to deal in the Software without restriction, including without limitation the rights to use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the Software, and to permit persons to whom the Software is furnished to do so, subject to the following conditions:
+
+The above copyright notice and this permission notice shall be included in all copies or substantial portions of the Software.
+
+THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
+```

integration-test/data/credentials/client.json

@@ -1,4 +1,4 @@
 {
-  "client_id":"stups_camp-frontend_45818add-c47d-4731-a40d-cea1ffd0e0c9",
-  "client_secret":"6i5ghB#S2iBK)%btb7%MxgxQX71Qr.)*"
+  "client_id":"nucleus_client",
+  "client_secret":"nucleus_client_secret"
 }

integration-test/data/credentials/user.json

@@ -1,4 +1,4 @@
 {
-  "application_username":"stups_camp-frontend",
-  "application_password":"QOnDASzQcezo.glh(jcVo9ryKxmS[@x3"
+  "application_username":"Nucleus_user",
+  "application_password":"Nucleus_user_pw"
 }

integration-test/mock-tooling/index.ts

@@ -1,5 +1,6 @@
 import * as chai from 'chai';
 import * as chaiAsPromised from 'chai-as-promised';
+import * as HttpStatus from 'http-status';
 
 import {
   getTokenInfo,
@@ -42,7 +43,7 @@ describe('mock tooling', () => {
       const promise = getTokenInfo(tokeninfoEndpoint, 'invalid');
 
       // then
-      return expect(promise).to.rejected;
+      return expect(promise).to.be.rejected;
     });
 
     it('should return the tokeninfo if token is valid', function () {

integration-test/oauth-tooling/getAccessToken.spec.ts

@@ -112,7 +112,7 @@ describe('getAccessToken', () => {
     it('should become the access token', function() {
 
       //given
-      setupTestEnvironment('Basic c3R1cHNfY2FtcC1mcm9udGVuZF80NTgxOGFkZC1jNDdkLTQ3MzEtYTQwZC1jZWExZmZkMGUwYzk6Nmk1Z2hCI1MyaUJLKSVidGI3JU14Z3hRWDcxUXIuKSo=', authServerApp);
+      setupTestEnvironment('Basic bnVjbGV1c19jbGllbnQ6bnVjbGV1c19jbGllbnRfc2VjcmV0', authServerApp);
 
       //when
       const promise = getAccessToken(getAccessTokenOptions);
@@ -168,7 +168,7 @@ describe('getAccessToken', () => {
     it('should become the access token', function() {
 
       //given
-      setupTestEnvironment('Basic c3R1cHNfY2FtcC1mcm9udGVuZF80NTgxOGFkZC1jNDdkLTQ3MzEtYTQwZC1jZWExZmZkMGUwYzk6Nmk1Z2hCI1MyaUJLKSVidGI3JU14Z3hRWDcxUXIuKSo=', authServerApp);
+      setupTestEnvironment('Basic bnVjbGV1c19jbGllbnQ6bnVjbGV1c19jbGllbnRfc2VjcmV0', authServerApp);
 
       //when
       const bearer = getAccessToken(getAccessTokenOptionsAuthorization)
@@ -255,6 +255,26 @@ describe('getAccessToken', () => {
 
   describe('refresh token grant', () => {
 
+    it('should be rejected, if grant type is unknown', function() {
+
+      // given
+      const host = 'http://127.0.0.1:30001/oauth2';
+      const options = {
+        scopes: ['campaign.edit_all', 'campaign.read_all'],
+        accessTokenEndpoint: `${host}/access_token`,
+        credentialsDir: 'integration-test/data/credentials',
+        grantType: 'INVALID_GRANT',
+        code: 'foo-bar',
+        redirectUri: '/redirect/'
+      };
+
+      // when
+      const promise = getAccessToken(options);
+
+      // then
+      return expect(promise).to.be.rejected;
+    });
+
     it('should become the access token', function () {
 
       // given

integration-test/oauth-tooling/middlewares.spec.ts

@@ -106,6 +106,31 @@ describe('middlewares', () => {
     });
   }
 
+  function addAuthenticationEndpointWithBrokenScopes() {
+
+    authServerApp.get('/oauth2/tokeninfo', function(req, res) {
+      const valid = req.query.access_token === '4b70510f-be1d-4f0f-b4cb-edbca2c79d41';
+
+      if (valid) {
+        res
+        .status(200)
+        .send({
+          'expires_in': 3515,
+          'token_type': 'Bearer',
+          'realm': 'employees',
+          'scope': '',
+          'grant_type': PASSWORD_CREDENTIALS_GRANT,
+          'uid': 'services',
+          'access_token': '4b70510f-be1d-4f0f-b4cb-edbca2c79d41'
+        });
+      } else {
+        res
+          .status(401)
+          .send('Unauthorized');
+      }
+    });
+  }
+
   function add500Endpoint() {
 
     authServerApp.get('/oauth2/tokeninfo', function(req, res) {
@@ -197,4 +222,25 @@ describe('middlewares', () => {
       'lastLogin': '2015-12-12'
     });
   });
+
+  it('should return 403 if empty scope is returned', function() {
+
+    // given
+    const authHeader = 'Bearer 4b70510f-be1d-4f0f-b4cb-edbca2c79d41';
+    addAuthenticationEndpointWithBrokenScopes();
+
+    // when
+    const promise = fetch('http://127.0.0.1:30002/resource/user', {
+      method: 'GET',
+      headers: {
+        authorization: authHeader
+      }
+    })
+    .then((res: any) => {
+      return res.status;
+    });
+
+    // then
+    return expect(promise).to.become(HttpStatus.FORBIDDEN);
+  });
 });

integration-test/oauth-tooling/tokenCache.spec.ts

@@ -262,4 +262,32 @@ describe('tokenCache', () => {
         expect(tokens['halo'].access_token).to.equal(otherAccessTokenValue);
       });
   });
+
+  describe('resolveAccessTokenFactory', () => {
+
+      it('should return a promise, evaluating the token', () => {
+
+        // given
+        nock(oauthHost)
+        .post('/access_token')
+        .reply(HttpStatus.OK, {
+          access_token: defaultAccessTokenValue
+        })
+        .get('/tokeninfo')
+        .query({ access_token: defaultAccessTokenValue })
+        .reply(HttpStatus.OK, defaultTokenInfoResponse);
+
+        // when
+        const tokenService = new TokenCache({
+          'nucleus': ['nucleus.write', 'nucleus.read'],
+          'halo': ['all']
+        }, oauthConfig);
+
+        const evalFunction = tokenService.resolveAccessTokenFactory('nucleus');
+        const promise = evalFunction();
+
+        // then
+        return expect(promise).to.become(defaultAccessTokenValue);
+      });
+    });
 });

package.json

@@ -33,7 +33,6 @@
     "http-status": "1.0.1",
     "nock": "9.0.14",
     "node-fetch": "1.7.1",
-    "q": "1.5.0",
     "uuid": "3.1.0"
   },
   "devDependencies": {
@@ -46,7 +45,6 @@
     "@types/mocha": "2.2.41",
     "@types/nock": "8.2.1",
     "@types/node-fetch": "1.6.7",
-    "@types/q": "1.0.2",
     "@types/uuid": "3.4.0",
     "chai": "4.1.0",
     "chai-as-promised": "7.1.1",

src/utils.ts

@@ -1,5 +1,4 @@
 import * as fs from 'fs';
-import * as q from 'q';
 import * as express from 'express';
 import * as HttpStatus from 'http-status';
 import * as btoa from 'btoa';
@@ -10,7 +9,19 @@ import {
 } from './constants';
 import { OAuthConfig, Token } from './types';
 
-const fsReadFile = q.denodeify<any>(fs.readFile);
+const fsReadFile = (fileName: string, encoding = 'utf8'): Promise<string> => {
+  const readPromise: Promise<string> = new Promise((resolve, reject) => {
+    fs.readFile(fileName, encoding, (error, data) => {
+      if (error) {
+        reject(error);
+        return;
+      }
+      resolve(data.toString());
+    });
+  });
+
+  return readPromise;
+};
 
 const AUTHORIZATION_BEARER_PREFIX = 'Bearer';
 const AUTHORIZATION_BASIC_PREFIX = 'Basic';
@@ -22,12 +33,14 @@ const AUTHORIZATION_BASIC_PREFIX = 'Basic';
  * @param fileName
  * @returns {Promise<any>}
  */
-export function getFileData(filePath: string, fileName: string): q.Promise<any> {
+export function getFileData(filePath: string, fileName: string): Promise<string> {
   if (filePath.substr(-1) !== '/') { // substr operates with the length of the string
     filePath += '/';
   }
 
-  return fsReadFile(filePath + fileName, 'utf-8');
+  const promise = fsReadFile(filePath + fileName, 'utf-8');
+
+  return promise;
 }
 
 /**

test/unit/credentials/client.json

@@ -0,0 +1,4 @@
+{
+  "client_id":"nucleus",
+  "client_secret":"nucleus_secret"
+}

test/unit/credentials/user.json

@@ -0,0 +1,4 @@
+{
+  "application_username":"nucleus_user",
+  "application_password":"nucleus_user_password"
+}

test/unit/express-tooling.ts

@@ -10,7 +10,7 @@ import {
 chai.use(chaiAsPromised);
 let expect = chai.expect;
 
-describe('oauth tooling', () => {
+describe('express tooling', () => {
 
   let requestMock: any;
   let responseMock: any;
@@ -236,15 +236,11 @@ describe('oauth tooling', () => {
         },
         precedenceErrorHandler: customErrorhandler,
         logger: {
-          info:  (p: any): void => { return; },
-          debug: (p: any): void => { return; },
+          ...loggerMock,
           error: (p: any): void => {
             expect(p).to.equal('Error while executing precedenceErrorHandler: ');
             done();
-          },
-          fatal: (p: any): void => { return; },
-          trace: (p: any): void => { return; },
-          warn:  (p: any): void => { return; }
+          }
         }
       };
 
@@ -264,7 +260,6 @@ describe('oauth tooling', () => {
 
       // then
       expect(() => { handleOAuthRequestMiddleware(config); }).to.throw(TypeError);
-
     });
 
     it('should call #next on public endpoint', () => {

test/unit/utils.ts

@@ -0,0 +1,27 @@
+import * as chai from 'chai';
+import * as chaiAsPromised from 'chai-as-promised';
+
+import { getFileData } from '../../src/utils';
+
+chai.use(chaiAsPromised);
+const expect = chai.expect;
+
+describe('utils', () => {
+  describe('getFileData', () => {
+    it('should ignore tailing / in filepath', () => {
+      const promise = Promise.all([
+        getFileData('test/unit/credentials', 'user.json'),
+        getFileData('test/unit/credentials/', 'user.json')
+      ])
+      .then((credentials) => {
+        return credentials[0] === credentials[1];
+        });
+      return expect(promise).to.become(true);
+    });
+
+    it('should be rejected, if file does not exist', () => {
+      const promise = getFileData('test/unit/credentials', 'foo.json');
+      return expect(promise).to.be.rejected;
+    });
+  });
+});