Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/16 limit domains #21

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

jordandenison
Copy link

@jordandenison jordandenison commented Aug 22, 2017

Resolves #16

also added "static" dir to lint ignore in package.json and included a docker-compose.yml file that hasn't been used/tested successfully yet but might come in handy

try {
whiteListedDomains = env('WHITELISTED_DOMAINS').split(',').map(str => str.toLowerCase().trim())
} catch (e) {
console.log(`WHITELISTED_DOMAINS not set as comma delimited string of email addresses properly, error: ${e.stack}`)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please replace these with console.error. I've been trying to relegate console.log to development, so that we can tell if dev logging was accidentally left behind.

README.md Outdated
@@ -176,7 +178,7 @@ jwt.sign({ userId: user.id })

## Security

This microservice is intended to be very secure.
This microservice is intented to be very secure. User accounts can be limited to certain domains by configuring the `WHITELISTED_DOMAINS` env variable.
Copy link
Collaborator

Choose a reason for hiding this comment

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

intented -> intended

I actually fixed this one earlier today, but I think git just automerged the conflict.

Copy link
Author

Choose a reason for hiding this comment

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

yeah I saw a conflict on that line but for some reason my eyes couldn't detect that change - thanks for pointing that out : )

},
"standard": {
"ignore": [
"static/**"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you! I should've done this but neglected to.

console.log(`WHITELISTED_DOMAINS not set as comma delimited string of email addresses properly, error: ${e.stack}`)
}

const isDomainAuthorized = domain => !whiteListedDomains.length || whiteListedDomains.includes(domain)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add a comment explaining this line -- at first blush, I thought "shouldn't that be whiteListedDomains.length &&, so what it's accomplishing is not as self-evident as I'd like.

if (!isDomainAuthorized(getEmailDomain(email))) {
const error = `non-whitelisted user "${email}" attempted to login`
console.log(error)
return Promise.reject(error)
Copy link
Contributor

Choose a reason for hiding this comment

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

We should use the reject method from line 25? It has to be used with a string like this: reject('INVALID_CREDENTIALS') and then you will find a key in the i18n file for the message.

@@ -58,6 +76,13 @@ module.exports = (env, jwt, database, sendEmail, mediaClient, validations) => {
},
register (params, client) {
const email = normalizeEmail(params.email)

if (!isDomainAuthorized(getEmailDomain(email))) {
const error = `non-whitelisted user "${email}" attempted to register`
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, we should use the reject method from line 25.

Copy link
Contributor

@gimenete gimenete left a comment

Choose a reason for hiding this comment

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

Great work! Sorry, I thought I added a comment to the PR and it doesn't seem like that.

There are a couple of small changes I want to add. If you don't have time, let me know and I'll do it.

Thanks!

@gimenete gimenete removed their assignment Jan 3, 2020
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.

Limiting users to certain domains
3 participants