Changes on ACCESS_TOKEN and implements REFRESH_TOKEN #7

Craaftx · 2018-02-28T16:25:52Z

Need to manage the expiration time of tokens
Need to implements REFRESH_TOKEN for "Password Credential" Grant Type

mikbry · 2018-02-28T16:27:17Z

src/model/index.js

+            access = a; 
+            return true; 
+          } 
+          return false; 


This return false; is useless

willdurand · 2018-02-28T16:44:27Z

src/model/index.js

-          access = a;
-          return true;
+          const aDate = new Date(a.created);
+          aDate.setSeconds(aDate.getSeconds() + a.expires_in);


So why are you using Date here? You get unix time value in a.created, so you could simply do an addition (convert expires_in in milliseconds first then add it to created), and compare the result to Date.now().

I transform TimeStamp into Date for be able to use .setSeconds() and .getSeconds() methods.

But you're right, I can do that to use only TimeStamp:

const aDate = a.created + (a.expires_in*1000); if(aDate > new Date().getTime()) { access = a; return true; }

But if the database don't return TimeStamp it failed.

But if the database don't return TimeStamp it failed.

this should not happen.

and I think new Date().getTime() == Date.now()

The database stores the "created" attribute in DateTime format, which normally returns the date in ISO format , but we have seen that the database can return TimeStamp in some case. So I decided to turn all the variables in Date to avoid bugs.

It has since been fixed in zoapp-core 0.7.0.

So you're supposed to change this code to only deal with unix time since that's what zoapp-core returns. The database stores the dates correctly, and it also returns them correctly. It is zoapp-core that converts them to unix time values and we should now assume that these values will always be unix time values.

wrong review

willdurand · 2018-03-01T13:26:42Z

.gitignore

 lib
 *.DS_Store
 config
+test/config


tests wont pass if you do not provide a configuration.

Craaftx · 2018-03-01T13:33:31Z

Now, we need a config.js file for the test files, we use it to avoid sharing sensitive data by mistake.

Here is an exemple for config.js :

const config = {
  database: {
    datatype: "mysql",
    host: "",
    name: "",
    user: "",
    password: "",
    charset: "utf8mb4",
    version: "2",
  },
  api: {
    endpoint: "/api",
    version: "1",
    port: 8081,
  },
  auth: {
    endpoint: "/auth",
    version: "1",
    port: 8085,
  },
};

export default () => config;

willdurand · 2018-03-01T13:36:48Z

@Craaftx how do you expect Travis CI to run the test suite without a configuration file? Please add a configuration file for the test suite as done here: https://github.com/Zoapp/core/blob/master/tests/test-config.js.

willdurand · 2018-03-01T15:02:49Z

test/config.js

+  },
+};
+
+export default () => config;


You dont need to do that. export the config object on line 1 insteaad:

export const config = { };

For this it's requested by EsLint :
[eslint] Prefer default export. (import/prefer-default-export)

You can export default { your config object here }; or disable this rule for this file.

willdurand · 2018-03-01T15:03:45Z

test/test2.zoauthServer.js

+    datatype: testConfig().database.datatype,
+    host: testConfig().database.host,
+    name: testConfig().database.name,
+    user: testConfig().database.user,


do not use a function here. also, if you have the same config object in the other file, why are you creating a new config object here? can't you just use the same?

You're true, i change that

willdurand · 2018-03-01T15:54:49Z

src/model/index.js

-          access = a;
-          return true;
+          const aDate = new Date(a.created);
+          aDate.setSeconds(aDate.getSeconds() + a.expires_in);


So you're supposed to change this code to only deal with unix time since that's what zoapp-core returns. The database stores the dates correctly, and it also returns them correctly. It is zoapp-core that converts them to unix time values and we should now assume that these values will always be unix time values.

willdurand · 2018-03-01T15:57:20Z

test/config.js

+  endpoint: "/auth",
+};
+
+export default () => config;


export the config object directly as default or name your export and ignore the eslint rule, but do not return an anonymous function as a default export.

willdurand · 2018-03-01T15:57:32Z

test/test3.zoauthRouter.js

+const mysqlConfig = testConfig();
+
+if (Object.prototype.hasOwnProperty.call(testConfig().database, "password")) {
+  mysqlConfig.database.password = testConfig().database.password;


What are you trying to achieve here?

willdurand · 2018-03-01T16:56:05Z

src/model/index.js

        if (a.access_token === accessToken) {
-          access = a;
-          return true;
+          const aDate = a.created + (a.expires_in * 1000);


aDate could be name differently

willdurand · 2018-03-02T09:21:07Z

@Craaftx good job, now you should squash your commits.

willdurand · 2018-03-03T16:01:42Z

src/zoauthServer.js

+  async requestGrantTypeClientCredential(clientId, clientSecret) {
+    const response = {};
+    response.result = { error: "Function Empty" };
+    return response;


return { result: { error: "error message", }, };

Craaftx · 2018-03-03T16:08:01Z

But i don't know why continuous-integration/travis-ci/pr don't pass. Locally all test works

Craaftx · 2018-03-08T16:22:34Z

src/zoauthServer.js

      response.result = {
        access_token: session.access_token,
        expires_in: session.expires_in,
+        refresh_token: refreshToken,


I just see my mistake..
This result in that :

{ access_token: 'kYh57bSsGATo5IVyrba00SsRlozJa1sNfPSi1XsRHm9O5aad', expires_in: 3600, refresh_token: Promise { { refresh_token: 'xjj50ueS7FpG41OTXVnkSkofEEqNHG1T', refresh_expires_in: 'U2PkE8w8k2gtYF6i8yVAIElD2QTZFwsq', refresh_created: 1520525458550 } }, scope: 'default' }

Craaftx · 2018-03-16T15:42:03Z

We plan to put aside the client_credentials Grant type, and focus on the password and refresh_token Grant type.

Craaftx · 2018-03-22T16:50:01Z

Grant_Type refresh_token supported

code has changed

Add access_token expiration support Squash commit Fix Trailing spaces on src/model/index.js Delete "return false;" on src/model/index.js Add a config file for testing and add generateRefreshToken() generateRefreshToken() return a base(32) token Add default test/config.js Add "No Password" Support Re-Add test/config.js Change config.js and use of config.js Change on validateAccessToken() and improve config.js use change aDate var name to expirationDate and add refreshTokenExpiration var

willdurand · 2018-04-08T21:05:25Z

Is it still WIP?

willdurand

Looks quite OK, I dont know much about what should be done here but if it works as intended, I'd say let's merge it.

Craaftx · 2018-04-09T06:50:11Z

This used to add refresh_token support on ZoAuth server. I just needed your review so if you are Ok we can merge.

willdurand · 2018-04-09T08:26:46Z

go ahead. ping @mikbry

mikbry

Create a constants.js and put in GRANT_TYPE_PASSWORD and GRANT_TYPE_REFRESH_TOKEN and modify according zoauthServer.js and index.js

willdurand · 2018-04-12T09:33:27Z

src/zoauthServer.js

-const GRANT_TYPE_PASSWORD = "password";
-const GRANT_TYPE_REFRESH_TOKEN = "refresh_token";
+const GRANT_TYPE_PASSWORD = constants.grant_type.password;
+const GRANT_TYPE_REFRESH_TOKEN = constants.grant_type.refresh_token;


I dont think that's what @mikbry asked. Your constants were OK, just put them as is in the constants.js file.

Like this ?

export const constants = { GRANT_TYPE_PASSWORD: "password", GRANT_TYPE_REFRESH_TOKEN: "refresh_token", };

no, lik:

export const GRANT_TYPE_PASSWORD = "password";

mikbry · 2018-04-30T10:33:21Z

What is the status of this PR ?

Craaftx changed the title ~~[WIP] Change on ACCESS_TOKEN and implements REFRESH_TOKEN~~ [WIP] Changes on ACCESS_TOKEN and implements REFRESH_TOKEN Feb 28, 2018

mikbry suggested changes Feb 28, 2018

View reviewed changes

src/model/index.js Outdated

access = a;

return true;

}

return false;

Copy link

mikbry Feb 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This return false; is useless

willdurand previously approved these changes Feb 28, 2018

View reviewed changes

willdurand reviewed Mar 1, 2018

View reviewed changes

.gitignore Outdated

lib

*.DS_Store

config

test/config

Copy link

Contributor

willdurand Mar 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tests wont pass if you do not provide a configuration.

willdurand suggested changes Mar 1, 2018

View reviewed changes

willdurand reviewed Mar 1, 2018

View reviewed changes

Craaftx force-pushed the token_support branch 4 times, most recently from 5c5a9a8 to 1834661 Compare March 2, 2018 10:05

willdurand previously approved these changes Mar 2, 2018

View reviewed changes

mikbry previously approved these changes Mar 2, 2018

View reviewed changes

willdurand reviewed Mar 3, 2018

View reviewed changes

Craaftx commented Mar 8, 2018

View reviewed changes

Craaftx added 6 commits March 23, 2018 12:09

Establishment of Grant Type, test ok

a0a3b53

Improve Syntax

6412016

[WIP] Add Refresh_Token support to GrantType Password

807a6ce

Patch refresh_token return in getAccessToken()

462a56a

Add Const for Grant_Type

28ce46e

Craaftx added 8 commits March 23, 2018 12:16

Add createResultResponse()

dcc3271

Adding Grant_type constant

eb2d8a6

Adding usage Grant_Type constant, need to make test on it

0e2b622

Remove client_credentials and refactor some functions

006325f

Rebase manually and apply new standards to new code. OK local test

f69b2b2

Make getAccessToken more clear

764ab59

Add Refresh_Token methods

3ad0687

Add tests for refresh_token

11440c5

Craaftx force-pushed the token_support branch from bfcdb10 to 11440c5 Compare March 23, 2018 11:22

willdurand self-requested a review March 29, 2018 20:46

willdurand reviewed Apr 8, 2018

View reviewed changes

Craaftx changed the title ~~[WIP] Changes on ACCESS_TOKEN and implements REFRESH_TOKEN~~ Changes on ACCESS_TOKEN and implements REFRESH_TOKEN Apr 9, 2018

mikbry suggested changes Apr 9, 2018

View reviewed changes

Add constants.js

186065b

willdurand suggested changes Apr 12, 2018

View reviewed changes

Change constants.js

e18c6d3

mikbry approved these changes Apr 30, 2018

View reviewed changes

Changes on ACCESS_TOKEN and implements REFRESH_TOKEN #7

Are you sure you want to change the base?

Changes on ACCESS_TOKEN and implements REFRESH_TOKEN #7

Uh oh!

Conversation

Craaftx commented Feb 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Craaftx Feb 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Craaftx commented Mar 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

willdurand commented Mar 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

willdurand commented Mar 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Craaftx commented Mar 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Craaftx commented Mar 16, 2018

Uh oh!

Craaftx commented Mar 22, 2018

Uh oh!

willdurand commented Apr 8, 2018

Uh oh!

willdurand left a comment

Choose a reason for hiding this comment

Uh oh!

Craaftx commented Apr 9, 2018

Uh oh!

willdurand commented Apr 9, 2018

Uh oh!

mikbry left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Craaftx commented Feb 28, 2018 •

edited

Loading

Craaftx Feb 28, 2018 •

edited

Loading

Craaftx commented Mar 1, 2018 •

edited

Loading