Authentication config file for in-memory backend #8
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| { | ||
| "accounts": [{ | ||
| "name": "Bart", | ||
| "email": "sampleaccount1@sampling.com", | ||
| "arn": "aws::iam:123456789012:root", | ||
| "canonicalID": "79a59df900b949e55d96a1e698fbacedfd6e09d98eacf8f8d5218e7cd47ef2be", | ||
| "shortid": "123456789012", | ||
| "keys": [{ | ||
| "access": "accessKey1", | ||
| "secret": "verySecretKey1" | ||
| }], | ||
| "users": [{ | ||
| "name": "Bart Jr", | ||
| "email": "user1.sampleaccount2@sampling.com", | ||
| "arn": "aws::iam:123456789013:bart", | ||
| "keys": [{ | ||
| "access": "accessKey1/1", | ||
| "secret": "verySecretKey1" | ||
| }] | ||
| }] | ||
| }, { | ||
| "name": "Lisa", | ||
| "email": "sampleaccount2@sampling.com", | ||
| "arn": "aws::iam:accessKey2:user/Lisa", | ||
| "canonicalID": "79a59df900b949e55d96a1e698fbacedfd6e09d98eacf8f8d5218e7cd47ef2bf", | ||
| "shortid": "123456789012", | ||
| "keys": [{ | ||
| "access": "accessKey2", | ||
| "secret": "verySecretKey2" | ||
| }] | ||
| }] | ||
| } | ||
|
Contributor
There was a problem hiding this comment. I suggest this file be moved to tests/utils or tests/conf in order to avoid having default values. The values can still be used by setting the env var. I've done a related comment in Config.json.
Author
There was a problem hiding this comment. I'm not fond of this, for two main reasons:
Author
There was a problem hiding this comment. Note that the tests should not be relying on this default conf, but rather define their own, if we wanted to isolate things properly. |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,180 @@ | ||
| import Logger from 'werelogs'; | ||
|
|
||
| // Here, we expect the logger to have already been configured in S3 | ||
| const log = new Logger('S3'); | ||
|
Contributor
There was a problem hiding this comment. Please use the logger utility in lib/utilities so the log level is set by the config.
Author
There was a problem hiding this comment. will do, thanks (and will check how the circular dep behaves there) |
||
|
|
||
| function incr(count) { | ||
| if (count !== undefined) { | ||
| return count + 1; | ||
| } | ||
| return 1; | ||
| } | ||
|
|
||
| /** | ||
| * @param {object} data - the error collector object | ||
| * @param {string} container - the name of the entity that contains | ||
| * what we're checking | ||
| * @param {string} name - the name of the entity we're checking for | ||
| * @param {string} type - expected typename of the entity we're checking | ||
| * @param {object} obj - the object we're checking the fields of | ||
| * @return {boolean} true if the type is Ok and no error found | ||
| * false if an error was found and reported | ||
| */ | ||
| function checkType(data, container, name, type, obj) { | ||
| if ((type === 'array' && !Array.isArray(obj[name])) | ||
| || (type !== 'array' && typeof obj[name] !== type)) { | ||
| data.errors.push({ | ||
| txt: 'property is not of the expected type', | ||
| obj: { | ||
| entity: container, | ||
| property: name, | ||
| type: typeof obj[name], | ||
| expectedType: type, | ||
| }, | ||
| }); | ||
| return false; | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
| /** | ||
| * @param {object} data - the error collector object | ||
| * @param {string} container - the name of the entity that contains | ||
| * what we're checking | ||
| * @param {string} name - the name of the entity we're checking for | ||
| * @param {string} type - expected typename of the entity we're checking | ||
| * @param {object} obj - the object we're checking the fields of | ||
| * @return {boolean} true if the field exists and type is Ok | ||
| * false if an error was found and reported | ||
| */ | ||
| function checkExists(data, container, name, type, obj) { | ||
| if (obj[name] === undefined) { | ||
| data.errors.push({ | ||
| txt: 'missing property in auth entity', | ||
| obj: { | ||
| entity: container, | ||
| property: name, | ||
| }, | ||
| }); | ||
| return false; | ||
| } | ||
| return checkType(data, container, name, type, obj); | ||
| } | ||
|
|
||
| function checkUser(data, userObj) { | ||
| if (checkExists(data, 'User', 'arn', 'string', userObj)) { | ||
| // eslint-disable-next-line no-param-reassign | ||
|
Contributor
There was a problem hiding this comment. perhaps we should reconsider this rule at some point...
Author
There was a problem hiding this comment. I am personally split. I am accepting of the middle-ground saying that whenever it's actually done willingly, a suppressing comment is acceptable. We can launch an issue on guidelines to discuss that if you wish to, I'm not against reviewing this specific rule.
Author
There was a problem hiding this comment. PS: Note that I'd rather explicitly silent this linter warning than silenting it through "re-binding" of the argument into a temp variable.
Contributor
There was a problem hiding this comment. Leaving aside the convenience (or lack of) of this rule, Object.assign can be used to avoid disabling the linter
Author
There was a problem hiding this comment. @jmunoznaranjo the point is to avoid doing a copy, since in this case, we want to actually modify the parameter, to update it. Object.assign is a "functional programming" way to do this, but this also means copying a lot of stuff, and killing performances when not necessary.
Contributor
There was a problem hiding this comment. Ok didn't consider perf |
||
| data.arns[userObj.arn] = incr(data.arns[userObj.arn]); | ||
| } | ||
| if (checkExists(data, 'User', 'email', 'string', userObj)) { | ||
| // eslint-disable-next-line no-param-reassign | ||
| data.emails[userObj.email] = incr(data.emails[userObj.email]); | ||
| } | ||
| checkExists(data, 'User', 'keys', 'array', userObj); | ||
|
|
||
| if (userObj.keys) { | ||
| if (checkType(data, 'User', 'keys', 'array', userObj)) { | ||
| userObj.keys.forEach(keyObj => { | ||
| // eslint-disable-next-line no-param-reassign | ||
| data.keys[keyObj.access] = incr(data.keys[keyObj.access]); | ||
| }); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| function checkAccount(data, accountObj) { | ||
| if (checkExists(data, 'Account', 'email', 'string', accountObj)) { | ||
| // eslint-disable-next-line no-param-reassign | ||
| data.emails[accountObj.email] = incr(data.emails[accountObj.email]); | ||
| } | ||
| if (checkExists(data, 'Account', 'arn', 'string', accountObj)) { | ||
| // eslint-disable-next-line no-param-reassign | ||
| data.arns[accountObj.arn] = incr(data.arns[accountObj.arn]); | ||
| } | ||
| if (checkExists(data, 'Account', 'canonicalID', 'string', accountObj)) { | ||
| // eslint-disable-next-line no-param-reassign | ||
| data.canonicalIds[accountObj.canonicalID] = | ||
| incr(data.canonicalIds[accountObj.canonicalID]); | ||
| } | ||
|
|
||
| if (accountObj.users) { | ||
| if (checkType(data, 'Account', 'users', 'array', accountObj)) { | ||
| accountObj.users.forEach(userObj => checkUser(data, userObj)); | ||
| } | ||
| } | ||
|
|
||
| if (accountObj.keys) { | ||
| if (checkType(data, 'Account', 'keys', 'array', accountObj)) { | ||
| accountObj.keys.forEach(keyObj => { | ||
| // eslint-disable-next-line no-param-reassign | ||
| data.keys[keyObj.access] = incr(data.keys[keyObj.access]); | ||
| }); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| function dumpCountError(property, obj) { | ||
| let count = 0; | ||
| Object.keys(obj).forEach(key => { | ||
| if (obj[key] > 1) { | ||
| log.error('property should be unique', { | ||
|
Contributor
There was a problem hiding this comment. This happens only once i.e., during startup, so it should be ok. Also, the logger config is validated before the authChecker runs, so there should be no issue using logger here.
Author
There was a problem hiding this comment. Btw, how will it behave ? I mean, the Config.js is using the authchecker inside. Which means that the authchecker will be called while the Config is being initialized. Isn't there a risk of stack overflow through recursion here ? I'll try it anyways.
Contributor
There was a problem hiding this comment. I was thinking that it should be ok as the config has been parsed and the var should be available to setup the logger, I don't see a recursion happening here, I could be wrong.
Author
There was a problem hiding this comment. I was thinking of the following inclusion cycle:
Then, when we instanciate the server:
I didn't check yet, but I do think there is a risk here.
Contributor
There was a problem hiding this comment. I found this in node docs
Author
There was a problem hiding this comment. Which is a good news, but there's still the point of how much can the config object be used, if it's incompletely built... |
||
| property, | ||
| value: key, | ||
| count: obj[key], | ||
| }); | ||
| ++count; | ||
| } | ||
| }); | ||
| return count; | ||
| } | ||
|
|
||
| function dumpErrors(checkData) { | ||
| let nerr = dumpCountError('CanonicalID', checkData.canonicalIds); | ||
| nerr += dumpCountError('Email', checkData.emails); | ||
| nerr += dumpCountError('ARN', checkData.arns); | ||
| nerr += dumpCountError('AccessKey', checkData.keys); | ||
|
|
||
| if (nerr > 0) { | ||
| return true; | ||
| } | ||
|
|
||
| if (checkData.errors.length === 0) { | ||
| return false; | ||
| } | ||
|
|
||
| checkData.errors.forEach(msg => { | ||
| log.error(msg.txt, msg.obj); | ||
| }); | ||
|
|
||
| log.fatal('invalid authentication config file (cannot start)'); | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
| /** | ||
| * @param {object} authdata - the authentication config file's data | ||
| * @return {boolean} true on erroneous data | ||
| * false on success | ||
| */ | ||
| export default function check(authdata) { | ||
| const checkData = { | ||
| errors: [], | ||
| emails: [], | ||
| arns: [], | ||
| canonicalIds: [], | ||
| keys: [], | ||
| }; | ||
|
|
||
| if (authdata.accounts === undefined) { | ||
| checkData.errors.push({ | ||
| txt: 'no "accounts" array defined in Auth config', | ||
| }); | ||
| return dumpErrors(checkData); | ||
| } | ||
|
|
||
| authdata.accounts.forEach(account => { | ||
| checkAccount(checkData, account); | ||
| }); | ||
|
|
||
| return dumpErrors(checkData); | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simpson ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're the first person to notice 😃