Skip to content

V2#24

Open
stefanpante wants to merge 80 commits intomasterfrom
v2
Open

V2#24
stefanpante wants to merge 80 commits intomasterfrom
v2

Conversation

@stefanpante
Copy link
Copy Markdown
Member

No description provided.

"license": "ISC",
"dependencies": {
"@digipolis/auth": "^1.1.0",
"@digipolis/auth": "git+https://github.com/digipolisantwerp/auth_module_nodejs.git",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Waarom is dit niet de gepubliceerde versie?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

omdat deze versie nog niet gepublished is.

Comment thread readme.md
```js
app.use(require('@digipolis/auth')(app, configuration));
```
You should use `express-session` in your application to enable session storage.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Misschien handig om achter express-session een linkje te leggen naar de juiste module:
https://www.npmjs.com/package/express-session

Comment thread src/controller.js Outdated
}

function logout(req, res) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Whiteline :-o

Comment thread src/controller.js
}

async function refreshToken(req, res, next) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Whiteline

Comment thread src/index.js
];

function validateConfig(config) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

whiteline

Comment thread test/loginCallback.js Outdated
});

res.on('end', () => {
// assert(res.redirect.calledWith('/'));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

oeps?

Comment thread test/sso.js Outdated


describe('test sso middleware', function onDescribe() {
// nockGetSession({ssoKey: 'fakessokey', payload: emptySessions})
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Mag weg.

- PORT=2000
- MONGO_CONNECTIONSTRING=mongodb://mongo:27017/basic-mongo-auth
- CLIENT_SECRET=
- CLIENT_ID
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Misschien handig om in te vullen met dummy code?

@Jan-Bart
Copy link
Copy Markdown
Member

Ik zou ook node v6 uit de test-builds halen en expliciet vermelden in de README dat we dit niet meer ondersteunen.

Copy link
Copy Markdown
Collaborator

@frankdsm frankdsm left a comment

Choose a reason for hiding this comment

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

Only es module imports pls

Comment thread src/controller.js Outdated
}

const newToken = await service.refresh(token);
req.session = Object.assign(req.session, { [tokenKey]: newToken });
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Rest operator can be used here

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Maakt rest operator er geen nieuw object van, ik kopieer hier de property op de req.session
Misschien is de assignment dan niet echt nodig.

Comment thread src/helpers.js
export function getHost(req) {
return `${req.protocol}://${req.get('host')}`
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Check for new lines with eslint

Comment thread test/isLoggedin.js Outdated
const user = require('./mocks/user.json');

describe('test #isLoggedin', function onDescribe() {
describe('GET /isLoggedin', function onDescribe() {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Lowercase urls pls

mongoConnectionString,
port = 2000
} = require('./config');
} = require('./old-config');
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why does te example use an old config ?

Comment thread package.json Outdated
Comment on lines +29 to +34
"@babel/core": "^7.8.4",
"@babel/preset-env": "^7.8.4",
"@babel/register": "^7.8.3",
"@istanbuljs/nyc-config-babel": "^3.0.0",
"@rollup/plugin-node-resolve": "^7.1.1",
"babel-plugin-istanbul": "^6.0.0",
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is there a reason to add babel to this?
I see a target to node 8 however i would tend to target the latest lts versions for new modules.
node 8 is passed its lifetime anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants