[wip] authn+authz #32
Conversation
* so now rather than supplying every single dependency every time you can call injector.withDefaults(db, …) which supplies all the defaults. * the second parameter still takes the same definition hash and overrides the defaults piecemeal. * use it in server.js.
* Ramda omit, it turns out, reifies prototype properties.
* eg: actor.can(‘create’, Actee.species(‘form’));
* is this actually a good idea? or should we exclusively rely on fromX static methods?
* we have some special actor classes that we want to keep track of; we already mark them as type: system, so we know which ones not to touch, but that doesn’t help us actually locate each particular system actor. * modifies an existing migration, but i doubt anybody ran it already. * naming is hard. up for debate.
* before, they took precisely one parameter each and tossed them into .where(). this meant only obj-style where syntax was possible. * we want to limit direct calls on db, and use simply where possible, so ideally we make this more flexible somehow. * now you can pass in objects, 3-tuple conditions, or an array containing many of either. * a small perf hit. debatable whether overloading or exposing more calls is better. but if you want a mix of condition types, you need to do the dynamic type check no matter what.
* new Session instance and query module.
* Session.decorate(expressHandler) will insert a Session instance onto
the request object. it contains an Option[Actor].
* if credentials are provided but invalid, it will fail early with a
401 authentication failed.
* debatable whether we want to simply make this an express prefilter
rather than wrap it when we need it (which is probably most of the
time).
* Actor and Session both get new method impls:
* can(verb, actee) returns Promise[Bool].
* ifCan(verb, actee).then(action) runs the action only if the actor
(or the actor implied by the session) meets the criteria; otherwise
it rejects with a 403 insufficient rights.
* Actor#can and Actor#ifCan are not actually used yet.
* both depend on the new Grant::can.
* grants.getByTriple upgraded:
* defensively takes either Actor or Option[Actor].
* accounts for Some(Actor) or None when resolving implied actors.
* all of this is put to use for POST /users, which is exemplary of the
intended interface.
* need to wrap "in (?)" with parens. * .then() needs a function.
* upgrade body-parser to actually be able to detect its failure signature. expressjs/body-parser#122 * also change sendError signature to match the standard one more.
* so the decimal shows up. dumb, maybe.
* not sure when the rowCount property disappeared from knex. sometime after the prototype was written, perhaps.
* some friction with our attempt at composable query blocks; if a native Promise is injected into the middle of the query chain, then because .then conflates map/flatMap it’ll just gobble up the FutureQuery and sit on it forever, not knowing that .end() needs to be called. * the mid-term solution is to create a new ExplicitPromise that can wrap a native Promise, and which exposes the same .end() method (now renamed to .point() to be a bit more generic - perhaps it’s a bad name as it is not /precisely/ an algebraic point operation), and will handle any FutureQueries it receives appropriately. * the basic resolve/reject have been move over to use ExplicitPromise (though actually reject is native as the chain won’t matter in a rejection case). * will have to engineer a better solution or be careful with libraries that return native Promises in the future (perhaps an .explicify() much like bluebird .promisify()?).
* at least for now, the problem can be solved elsewhere more cleanly, as session construction is pretty rigidly controlled so we can just make guarantees and assumptions upon it.
* since we might serialize for many different purposes, and we are not actually performing the obj-to-bytes serialization. * instead, what we are expressing is that this is the modification to the data that makes it appropriate for API consumption. * also changes user serialization format to merge actor data inline with the user object, rather than have user contain actor.
* please mama no sql injection.
* both return ExplicitPromises. * set bcrypt up to run on 12-round salt generation; seems like the new normal.
* don’t catch the same conditions twice; just immediately defer to finalize.
* eg:
User.getById(params.id)
.then(getOrNotFound) // returns 404 if Option.none.
.then((user) => /* guaranteed user */)
* used in subsequent commits.
* as users and actors will always be updated together, transactionally.
* checks authz. * updates only appropriate fields. * saves both actor and user records.
* PUT body json { "old": "old", "new": "new" } to change the password.
* verifies the old password before changing to the new.
* finally grew enough overlapping functionality that this made sense. * added a .transacting() method to upgrade a query tree to transacting.
* matches the syntax of query blocks, except: * it’s a call instead of a getter. style question to be resolved around whether to make them all getters or make them all calls. i am open to either, but they should certainly be consistent.
* apart from the lack of tests, this PR is ready for review.
|
Okay, please to review @lognaturel @matthew-white @ggalmazor. Commit messages are highly descriptive. Explicitly open questions:
Downriver questions that don't prevent this PR from merging:
More options exist for each of these; these are just the obvious paths in my mind. |
* now assumes identity.
* as discussed on slack; seems people like the more-permissive route.
* it was pretty much done already.
* as expected. * the POST actor must have a grant to 'create' on 'form'. * :id is by the xmlFormId.
|
Okay, now basic Form POST/GET are in there too since they were incredibly easy to add. |
| .canOrReject('create', Actee.species('form')) | ||
| .then(() => Form.fromXml(body)) | ||
| .then((form) => form.create()) | ||
| .then((savedForm) => (session.actor.isDefined() |
There was a problem hiding this comment.
Lacking pattern-matching, one wonders whether this can be made smoother.
|
|
||
| static getByBearerToken(token) { | ||
| return sessions.getByBearerToken(token) | ||
| // TODO: awkward commuting homework. |
There was a problem hiding this comment.
Need to go from Option[T] to Promise[Option[T]], which is awkward with the current machinery.
There was a problem hiding this comment.
There's a lot here and I know it was a lot of work, @cxlt! The highest-level comment which I also wrote inline is that creating new endpoints that ensures proper authentication and authorization is straightforward and should be comfortable to most people. 👍
Some of the machinery is hard for me to follow. I know some of what's at play is that I'm not too familiar with some of the syntax (e.g. ({ db }) => {...} as @ggalmazor explained in Slack) and of course, that there's some complexity inherent in what is being modeled.
The commit comments are really helpful and I would like to see a little bit of that become comments and/or dev docs. I know that @cxlt you were reluctant to get started with dev docs because understandably you expect things will change but I think just a little bit of high-level narrative now would help. A concrete way to start would be to take Instance.with(ActeeTrait)(({ Grant, actors, all, grants, simply }) => class and upack it a little bit. Link out to a good narrative on traits, describe that package.js is the dependency injection container, a paragraph on Option, things like that. I think a few minutes on that would help a lot with giving someone an idea of where to start. It doesn't have to go in this PR.
Small thing but I would personally like to see words like "easy", "simple", "just" removed from the comments in a later edit pass. They can be a bit demoralizing for people who don't find the code they refer to all that simple (e.g. me). Textbooks do this a lot and that is often why they get thrown out the window.
|
|
||
| // Instances are simple databags representing model records, with convenience | ||
| // methods sprinkled in to ease the readability of common tasks. | ||
| // | ||
| // InstanceBase is the only common superclass to them all. | ||
| class InstanceBase { | ||
| forCreate() { return this; } | ||
| forSerialize() { return this; } | ||
| forApi() { return this; } | ||
|
|
||
| // Returns a new copy of this instance with merged properties. | ||
| with(data) { |
There was a problem hiding this comment.
Just saw that with is reserved https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/with
There was a problem hiding this comment.
ES5 removed a lot of limitations with reserved keywords; they are only illegal in cases where the lexer could genuinely be confused. If you have another verb to suggest that is as descriptive as with I am happy to substitute, though.
lib/resources/users.js
Outdated
| service.get('/users/:id', endpoint(({ params }) => | ||
| User.getByActorId(params.id).then((user) => user.withActor()))); | ||
| // TODO: probably hide email addresses unless admin/self? | ||
| service.get('/users/:id', endpoint(({ params }) => User.getByActorId(params.id))); |
There was a problem hiding this comment.
I would prefer getting any info about a specific user be protected.
There was a problem hiding this comment.
Okay; sounds like future work though.
lib/resources/users.js
Outdated
| // TODO: probably hide email addresses unless admin/self? | ||
| service.get('/users/:id', endpoint(({ params }) => User.getByActorId(params.id))); | ||
|
|
||
| service.post('/users', endpoint(({ body, session }) => |
There was a problem hiding this comment.
Seems the biggest thing about this PR is making sure that the code here is approachable and useful. I think it is.
|
I hear what you're saying on "easy"/"just"/etc, but without such words it is difficult to express the justification for punting certain things down the road. |
|
I'm going to merge this just to simplify the cross-repository stuff and start working on subbranches but please don't let that stop anybody from continuing to review and comment. |
|
Hi everyone! I've reviewed the commit messages and I've also downloaded your branch and checked the code and I can't see any important stuff that we should have to take care of right now. I feel that with this PR, Jubilant is taking a good momentum. Huge step forward here, congratulations! :) |
|
Regarding the tests, I can't really add much to what I said in #29 at this moment. Maybe after you develop your ideas I can be more helpful. As I mentioned in Slack, I'll be focusing on Aggregate and Briefcase but I can still help with specific problems. |
No description provided.