Skip to content
This repository was archived by the owner on Apr 8, 2019. It is now read-only.

Assignment 4: add basic auth #121

Closed

Conversation

FennNaten
Copy link
Contributor

hueniversity assignment 4 (solves #118 )

added users.json: defines some dummy username/password couples

modified index.js: registers new private plugin
}
},
authStrategyName: 'private-basic',
authValidate: function(username, password, callback) {
Copy link
Contributor

Choose a reason for hiding this comment

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

personally I would declare the function outside of the object declaration as in internals.authValidate = funtion()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hum, that's what I did in the tests of the file, mainly because I copied their overall style from hapi-auth-basic plugin tests.
Naturally, I tend to define all of an object properties on the spot when their existence is unconditional. Could you please elaborate on why you would declare it outside? I don't see the benefits but I may miss something.

Copy link
Contributor

Choose a reason for hiding this comment

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

the only benefit is readability. If the function gets large or you have a lot of them it can get messy but of course this is more of a personal note than an official style from Hapi

adds return statement for consistency and removes unnecessary blank line in lib/private.js as pointed by @AdriVanHoudt
…tion. updates single user retrieval in lib/private.js to reflect the change and update tests to inject test users before each test and restore users module after each test
@FennNaten
Copy link
Contributor Author

I wasn't satisfied by directly using 'production' users in the tests, so I modified Users structure and tests to inject users before each test and switch back to original list after.
However, this still doesn't feel right. I'll give a shot at more refactoring to have Users module injected in the private plugin at server instantiation, so that mocking is made easier.
I committed anyway, because this is a learning experiment, so I wanted the PR to reflect thought process. ;)

@AdriVanHoudt
Copy link
Contributor

The users are just a placeholder right now, see the assignment, this will later probably be stored in a db which allows you to make a test db.
If you really want to do this you might be better of with having a users file for testing and require the right one base on process.env.NODE_ENV which standards to test when running Lab

@FennNaten
Copy link
Contributor Author

Yes, I know that's only a placeholder and the code will be further refactored later, if I was just prototyping and chaining tasks I think I would not care as much, and wait for the next round. However I have some time, so I use it to do some experiments ;)
I'm generally not a fan of swapping logic based on environment, except for a config file maybe, if this file's keys are used to feed explicit initialization of elements when an element is bootstrapped.
I know that feels a bit like over engineering at this stage ;)

@AdriVanHoudt
Copy link
Contributor

You can consider the users files config files ;)

…n as parameter by Private module. refactor tests accordingly.
@FennNaten
Copy link
Contributor Author

Here a version that feels better (to me), where users module is injected during application bootstrapping phase instead of being required in private module, so that fake users can be injected in tests.
Would still be best in my opinion to have some config files for the app, but I'll wait and see how things evolve on next assignments. ;)

@AdriVanHoudt
Copy link
Contributor

this only has a point if you at least pass in 'test users' instead of the same users you would normally use :D

@FennNaten
Copy link
Contributor Author

Indeed. The previous commit also took care of that ;)


// Declare internals

var internals = {};


exports.init = function (port, next) {
exports.init = function (config, next) {

Choose a reason for hiding this comment

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

passing the user file via config wasn't in the assignment.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants