445 add metadata field to entities #466

cianfoley-nearform · 2018-02-19T18:16:01Z

Lots of changes here guys, all the tests still pass and the new ones I wrote pass...

Migration to 7
Fixtures updated (thought prob should have just handled in test)
Updates to core, validation, swagger for hapi for org, teams, users
Updates to the routes for org, teams, users
updated end to end and core tests too.

@dberesford @mihaidma

C.

coveralls · 2018-02-19T18:19:02Z

Coverage decreased (-0.005%) to 96.272% when pulling 99c9f34 on 445-add-metadata-field-to-entities into 247408d on master.

dberesford

LGTM - good work @cianfoley-nearform
Just double checking, this is not a breaking change right? I.e. anyone using existing API does not need to update any of their code when they upgrade to this version of udaru?

dberesford · 2018-02-19T19:41:39Z

database/loadVolumeData.js

+      key1: val1,
+      key2: val2
+    }
+    return "'" + JSON.stringify(obj) + "'::JSONB"


Why does ::JSONB need to be appended to the stringified json?

Also swagger needs to be regenerated in order to update /docs, doesn't have to be done as part of this PR but we should review documentation before this gets published.

I committed swagger docs in the latest commit :-)

please make sure you release the client, although in this particular case it gives no side effects.

client.query('SELECT NOW()', (err, res) => { console.log(err, res) client.end() })

ok on termination of process? I'm using the same client throughout.

the connection will probably time out after a while but it is a good practice to release it when not used anymore

added to final callback just before process ends in try catch with no output at least to make an attempt at closing... if I call after query callback it will terminate for subsequent queries

cianfoley-nearform · 2018-02-19T22:31:17Z

Not sure Damien, I just saw that in fixures so I kept with the convention.... it;s going directly into a sql insert statement in volume fixtures and not using udaru core

…

On Mon, Feb 19, 2018 at 7:58 PM, Damian Beresford ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In database/loadVolumeData.js <#466 (comment)>: > @@ -148,15 +149,27 @@ function loadPolicies (startId, orgId, teamId, callback) { }) } +function getMetaData (val1, val2) { + if (ADD_METADATA) { + var obj = { + key1: val1, + key2: val2 + } + return "'" + JSON.stringify(obj) + "'::JSONB" Also swagger needs to be regenerated in order to update /docs, doesn't have to be done as part of this PR but we should review documentation before this gets published. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#466 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AiXnQwjvOw8RnV-HxmM3Egbc-ZNRQBv8ks5tWdJLgaJpZM4SK8KE> .

cianfoley-nearform · 2018-02-19T22:34:37Z

- EDIT - it is a breaking change now with 409 conflict response for unique constraint No it's not a breaking change so minor version number update... all existing tests pass, core and end-to-end so unless there's features that are untested it should be same? If there is no meta field, it isn't returned at all, and if not present in params, it works fine... There are some functions where updates could be optional such as name, description, meta potentially otherwise the caller of API will have to pass back the same data, i noticed in tests the full object being passed back at times even though the params are limited to 2 or 3, i wondered if this should be allowed but it is convenient. C.

…

On Mon, Feb 19, 2018 at 7:49 PM, Damian Beresford ***@***.***> wrote: ***@***.**** approved this pull request. LGTM - good work @cianfoley-nearform <https://github.com/cianfoley-nearform> Just double checking, this is not a breaking change right? I.e. anyone using existing API does not need to update any of their code when they upgrade to this version of udaru? ------------------------------ In database/loadVolumeData.js <#466 (comment)>: > @@ -148,15 +149,27 @@ function loadPolicies (startId, orgId, teamId, callback) { }) } +function getMetaData (val1, val2) { + if (ADD_METADATA) { + var obj = { + key1: val1, + key2: val2 + } + return "'" + JSON.stringify(obj) + "'::JSONB" Why does ::JSONB need to be appended to the stringified json? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#466 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AiXnQ12uXi_5zZpqfwfMjf5fakmt0Hkzks5tWdA9gaJpZM4SK8KE> .

cianfoley-nearform · 2018-02-20T11:04:44Z

@dberesford @mihaidma @mirceaalexandru
The following issue is catered for in the latest commit of this branch
#450

…ement of allow/deny on policies

cianfoley-nearform · 2018-02-26T10:07:20Z

Issue #475 fix committed to this branch

cianfoley-nearform · 2018-02-26T10:08:35Z

Issue #470 and other swagger related issues dealt with in this branch

cianfoley-nearform · 2018-02-26T12:03:49Z

lib/core/lib/ops/validation.js

+const ResourcesArray = Joi.array().max(10).items(requiredString.description('A single resource')).single().required().description('A list of Resources').label('ResourcesArray')
+
+const StatementObject = Joi.object({
+  Effect: Joi.string().valid('Allow', 'Deny').label('Effect'),


…nsert/update conflict constraints

feugy · 2018-02-28T11:47:28Z

database/dbscripts/addTeamAndPolicyNameConstraints.sql

@@ -0,0 +1,5 @@
+ALTER TABLE teams
+    ADD CONSTRAINT "Unique Team Names" UNIQUE (name, org_id);


Oups, sorry to jump in, but this will be a problem in Energy Insights.

As we use nested team, this situation is legit:

team 1

team A

team 2

team A

What is the reason to make names unique?

I second this ^^: there is actually nothing that comes to mind why team names should be unique. The id is unique (to be able to manage them) while the name is just for readability purposes. If we are looking at what udaru is supposed to do: ids would have been enough to be able to manage access. The name for a team (and others entities) was added as a "bonus" - bare UM functionality.

Hi @feugy, @floridemai

This is an optional script, not part of default :-)

Best wishes,

Cian

Perfect, I wasn't aware of the optional characteristics of those scripts.
Sorry for the interruption!

why would we commit the scripts? no point to put them in the project repo

might be better to document that we can handle multiple constraints in readme?

This was put in based on this issue: #474

@feugy, @floridemai no prob, probably best to not have in there as mihai suggests, just thought it might be useful to those that need such constraints set up

i agree with the opinions above, let's not complicate the code with adding constraints there (even if they are on both team and parent id). If the user will add from UI two teams with the same name he can always go and edit names.

mihaidma · 2018-02-28T11:43:58Z

database/migrations/007.undo..sql

@@ -0,0 +1,6 @@
+ALTER TABLE public.organizations
+    DROP COLUMN meta;


incorrect column name

also the migration file name is wrong

fixing now...

mihaidma · 2018-02-28T15:02:18Z

database/nameConstraints.js

@@ -0,0 +1,32 @@
+'use strict'


i am of the opinion that we should no add these for the Udaru users, they will make their own constraints if they really want but probably that will cause issues with keeping the migration up to date.

yes if we change constraints in future migrations theirs will be dropped...

mihaidma · 2018-03-01T08:06:38Z

database/loadVolumeData.js

+      key1: val1,
+      key2: val2
+    }
+    return "'" + JSON.stringify(obj) + "'::JSONB"


the connection will probably time out after a while but it is a good practice to release it when not used anymore

mihaidma · 2018-03-01T08:20:02Z

lib/core/lib/ops/validation.js

@@ -4,14 +4,39 @@ const Joi = require('joi')

 const requiredString = Joi.string().required()
 const requiredStringId = Joi.string().required().max(128)
+const MetaData = Joi.object().optional().description('Arbitrary Metadata').label('MetaData')


should we remove "arbitrary" from the description?

mihaidma · 2018-03-01T08:20:49Z

lib/core/lib/ops/validation.js


+const PolicyIdObject = Joi.object({
+  id: Joi.string().required().description('Policy Id'),
+  variables: Joi.object().description('A list of the veriables with their fixed values').label('variables')


veriables->variables

done, need spellchecjker

mihaidma · 2018-03-01T08:58:24Z

lib/core/lib/ops/validation.js

+const PolicyIdObject = Joi.object({
+  id: Joi.string().required().description('Policy Id'),
+  variables: Joi.object().description('A list of the veriables with their fixed values').label('variables')
+}).required().description('Policy Id Object, alternatively supply string instead').label('PolicyIdObject')


Policy Id Object, alternatively supply string instead
i would say here something along the lines of:
Policy Object (a {key, variable} pair) or a string representing the policy id
to make it easier for the user to understand what it does

how about
'Policy object ({id, variables}) OR policy id string'

if we say key variable pair they might mix up with key value pair...

moved out to the array because not showing in docs anyway

mihaidma · 2018-03-01T09:00:47Z

lib/core/lib/ops/validation.js


+const PolicyIdObject = Joi.object({
+  id: Joi.string().required().description('Policy Id'),


from what i saw the IDs are of type requiredStringId that has a max length limitation
Joi.string().required().description('Policy Id') -> requiredStringId.description('PolicyId')

i'm gonna create PolicyId as a requiredId String and reuse that within the PolicyObject too

I'm going to raise a change request for updating our models to share between validation.js and swagger.js

mihaidma · 2018-03-01T09:03:50Z

lib/core/lib/ops/validation.js

+const PolicyIdsArray = Joi.array().required().items(requiredPolicy).description('can be array of objects with id:string, variables:object properties OR an array of string ids').label('PolicyIdsArray')
+const UsersArray = Joi.array().required().items(requiredString).description('User IDs').label('UsersArray')
+const TeamsArray = Joi.array().required().items(requiredString).description('Teams IDs').label('TeamsArray')
+const ResourcesArray = Joi.array().max(10).items(requiredString.description('A single resource')).single().required().description('A list of Resources').label('ResourcesArray')


why is this limit of 10 added? where does this come from? does pbac impose any limits on this? we should remove this limit

max(10) wasn't added, it was declared previously inline on resources: property,

not its declared in specific object to prevent repeating model (see line 41 below)

resources: Joi.array().max(10).items(requiredString.description('A single resource')).single().required().description('A list of Resources'),

ps I noticed the limit and thought it was odd, don't know if there is a limit on PBAC, if not it should definitely be removed

removing, it's not a restriction on our output models, though that wouldn't break anything, input rules more important as if we restrict output and input is different we're in trouble...

mihaidma · 2018-03-01T09:05:46Z

lib/core/lib/ops/validation.js

+const StatementObject = Joi.object({
+  Effect: Joi.string().valid('Allow', 'Deny').label('Effect'),
+  Action: Joi.array().items(Joi.string()).label('Action'),
+  Resource: Joi.array().items(Joi.string()).label('Resource'),


both action and resource should have a minimum one element

do you want me to add for this version?

doesn't break tests, done.

mihaidma · 2018-03-01T09:06:28Z

lib/core/lib/ops/validation.js

+  Condition: Joi.object().label('Condition')
+}).label('StatementObject')
+
+const StatementsArray = Joi.array().items(StatementObject).label('StatementsArray')


minimum one element

mihaidma · 2018-03-01T11:29:17Z

lib/plugin/swagger.js

+  if (listType === TEAMS) return Teams
+  if (listType === TEAMREFS) return TeamRefs
+  if (listType === USERS) return Users
+  if (listType === ORGANIZATIONS) return Organizations


we could put these on an object and just say return mapper[listType] instead of all the ifs

yes, i was being silly there, much tidier now, i don't even need lookup, i can pass directly to list getList

Add request id for the resource builder

cianfoley-nearform added 3 commits February 19, 2018 11:04

support for optional metadata field, team, user, org read only

7a7d45b

get, post and put updated for optional metadata field

f275b48

test cases on core added for create, update, read

9cbe943

cianfoley-nearform requested review from dberesford and mihaidma February 19, 2018 18:16

dberesford approved these changes Feb 19, 2018

View reviewed changes

#450 solved: authorization pre-check required on teams payload object

42268a6

updates to validatation models for better swagger definitions, enforc…

534787f

…ement of allow/deny on policies

added test for issue #475 (invalid effect data)

a485077

cianfoley-nearform commented Feb 26, 2018

View reviewed changes

added a note to pagedteamslist to state policies and users not populated

1484954

This was referenced Feb 26, 2018

Internal server error when trying to update teams for non-existing user #450

Closed

Add a json field called metadata to Organizations, Teams and Users #445

Closed

updates to all entities to throw 409 conflict instead of 400 for db i…

ece3eed

…nsert/update conflict constraints

mihaidma force-pushed the 445-add-metadata-field-to-entities branch from ece3eed to 614a9a7 Compare February 28, 2018 11:36

feugy reviewed Feb 28, 2018

View reviewed changes

mihaidma reviewed Feb 28, 2018

View reviewed changes

fix migrate 7 uninstall script

1d72f1a

mihaidma force-pushed the 445-add-metadata-field-to-entities branch from 844da43 to 1d72f1a Compare February 28, 2018 12:36

mihaidma reviewed Feb 28, 2018

View reviewed changes

mihaidma reviewed Mar 1, 2018

View reviewed changes

cianfoley-nearform and others added 2 commits March 1, 2018 12:23

pre-merge issues addressed

30fff68

fix: add request id for the resource builder

243c7c0

Merge pull request #478 from nearform/fix/get-user-teams

99c9f34

Add request id for the resource builder

mihaidma merged commit 124d081 into master Mar 1, 2018

mihaidma deleted the 445-add-metadata-field-to-entities branch March 1, 2018 16:37

		@@ -0,0 +1,5 @@
		ALTER TABLE teams
		ADD CONSTRAINT "Unique Team Names" UNIQUE (name, org_id);

		@@ -0,0 +1,6 @@
		ALTER TABLE public.organizations
		DROP COLUMN meta;


		const PolicyIdObject = Joi.object({
		id: Joi.string().required().description('Policy Id'),

445 add metadata field to entities #466

445 add metadata field to entities #466

Conversation

cianfoley-nearform commented Feb 19, 2018

coveralls commented Feb 19, 2018 • edited Loading

dberesford left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cianfoley-nearform commented Feb 19, 2018 via email

cianfoley-nearform commented Feb 19, 2018 via email • edited Loading

cianfoley-nearform commented Feb 20, 2018

cianfoley-nearform commented Feb 26, 2018

cianfoley-nearform commented Feb 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cianfoley-nearform Feb 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cianfoley-nearform Feb 28, 2018 • edited Loading

Choose a reason for hiding this comment

mihaidma Feb 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cianfoley-nearform Mar 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Feb 19, 2018 •

edited

Loading

cianfoley-nearform commented Feb 19, 2018 via email •

edited

Loading

cianfoley-nearform Feb 28, 2018 •

edited

Loading

cianfoley-nearform Feb 28, 2018 •

edited

Loading

mihaidma Feb 28, 2018 •

edited

Loading

cianfoley-nearform Mar 1, 2018 •

edited

Loading