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

Add JSON APIs for Camera and Sample File Dirs #66

Open
wants to merge 12 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@jgautier
Copy link

commented Mar 19, 2019

This is my attempt to start on #35 it has a new API for adding Cameras and Sample File Dirs via JSON endpoints. Its not finished yet but I figured it would be good to submit and get feedback. Things I still need to do

[ ] - Write documentation
[ ] - Look into server refactoring: the Streamer and Syncer interfaces in the server don't have any teardown logic, aren't exposed to the web UI in any way, etc.
[ ] - Authentication. I think this is already done?
[ ] - Bootstrap authentication?

I am happy to work on the front end JS as well but figured that could be a separate PR after the APIs are implemented. This is my first chunk of Rust code so feedback is definitely appreciated.

Thanks for the awesome project!

@scottlamb
Copy link
Owner

left a comment

Nice!

btw, in the future, it might be wise to check in with me before spending significant time on a pull request, in case I'm working on something that significantly overlaps with it. In this case, I'm not, and your work is very welcome. (I'm currently poking at #28, very very slowly because I'm pretty busy with work and family lately.)

I left some code comments, but I think it's looking good (especially for your first Rust code).

Regarding a couple of your unchecked boxes:

Server refactoring: currently the Streamer and Syncer stuff is all just set up in src/cmds/run.rs. Also, the rotation offset is calculated considering the total number of streams, which doesn't work with adding and subtracting them freely. I think it might also get messy to support adjustments while actually streaming the camera, or even using the sample file dir. Hmm...the simplest thing to do might be to just have a "config" vs "active" mode system-wide, only allow these changes to be made while in "config" mode, and have a way to shift between modes. I imagine folks will reconfigure infrequently enough this wouldn't be too much of a problem. What do you think?

Authentication: yeah, I made progress on authentication since filing the issue. I think the missing bit is that there's still no notion of permissions. So you can't have someone who is authorized to view streams but not reconfigure the cameras, which is not great. The users table has a flags field; we could just claim one as "admin" or some such. There are more grand things we could do (finer-grained permissions/scopes at the user and session levels) but it's nice to start simple, and we'd need a schema change to do anything else anyway. Easiest to separate that out from the rest of the work when possible.

src/web.rs Outdated
}
}
}
let result = db.add_camera(c);

This comment has been minimized.

Copy link
@scottlamb

scottlamb Mar 19, 2019

Owner

I'd do let camera_id = db.add_camera(c).map_err(internal_server_err)? here, and skip all the match logic. Besides being more terse, this will return the serialized Error to the client.

src/web.rs Outdated
})
.and_then(|b| {
let body_vec = b.into_bytes().to_vec();
let json_str = String::from_utf8(body_vec).unwrap();

This comment has been minimized.

Copy link
@scottlamb

scottlamb Mar 19, 2019

Owner

unwrap() means this will crash the server on invalid UTF-8. I'd I'd do bad_req instead.

src/web.rs Outdated
};
Ok((req, json_obj))
})
.map(|ret| {

This comment has been minimized.

Copy link
@scottlamb

scottlamb Mar 19, 2019

Owner

Both this .map and .map_err seem to be no-ops? can you just remove them?

@@ -73,11 +73,13 @@ enum Path {
TopLevel, // "/api/"
Request, // "/api/request"
InitSegment([u8; 20], bool), // "/api/init/<sha1>.mp4{.txt}"
Camera(Uuid), // "/api/cameras/<uuid>/"
SaveCamera, // "/api/cameras"

This comment has been minimized.

Copy link
@scottlamb

scottlamb Mar 19, 2019

Owner

can you extend the paths test with the new possibilities?

src/web.rs Outdated
let mut db = self.db.lock();
for stream in &c.streams {
let id = match stream.sample_file_dir_id {
Some(id) => {

This comment has been minimized.

Copy link
@scottlamb

scottlamb Mar 19, 2019

Owner

nit: the braces aren't necessary in this case. You can write it as simply Some(id) => id,

src/web.rs Outdated

let c: CameraChange = serde_json::from_value(body).map_err(|_| bad_req("missing fields"))?;
if c.streams.len() == 0 {
*resp.status_mut() = StatusCode::BAD_REQUEST;

This comment has been minimized.

Copy link
@scottlamb

scottlamb Mar 19, 2019

Owner

I'd do return Err(bad_req("missing streams")) to have a more informative error message. Same below with no such dir case.

src/web.rs Outdated
resp.headers_mut().insert(header::CONTENT_TYPE,
HeaderValue::from_static("application/json"));
let mut db = self.db.lock();
match body["path"].is_string() {

This comment has been minimized.

Copy link
@scottlamb

scottlamb Mar 19, 2019

Owner

I think it'll be more readable to add a new type to json.rs and avoid all this is_string() + unwrap() stuff.

src/web.rs Outdated
///
/// Use with `and_then` to chain logic which consumes the json body.
fn with_json_body(&self, mut req: Request<hyper::Body>)
-> Box<Future<Item = (Request<hyper::Body>, serde_json::Value),

This comment has been minimized.

Copy link
@scottlamb

scottlamb Mar 19, 2019

Owner

I think you could have the second part be a String, so you don't have to convert through the json::Value intermediary. The chained future would call serde_json::from_str() instead of serde_json::from_value() then.

src/web.rs Outdated
let resp = cli.post(&login_url).form(&p).send().unwrap();
assert_eq!(resp.status(), http::StatusCode::NO_CONTENT);

// should reutrn 405 when sending a get

This comment has been minimized.

Copy link
@scottlamb

scottlamb Mar 19, 2019

Owner

nit: s/reutrn/return/; same a few place below.

src/web.rs Outdated
@@ -1233,6 +1470,41 @@ mod tests {
.send().unwrap();
assert_eq!(resp.status(), http::StatusCode::BAD_REQUEST);
}
#[test]
fn save_sample_file_dir(){

This comment has been minimized.

Copy link
@scottlamb

scottlamb Mar 19, 2019

Owner

nit: space before the {

@jgautier

This comment has been minimized.

Copy link
Author

commented Mar 20, 2019

@scottlamb Thanks! really appreciate your feedback. I think I have addressed most if not all of your comments and they definitely made the code a bit cleaner. I still need to write the API docs for the new endpoints which I think should probably be done before this gets merged, hopefully I can get to it by the end of this week.

For the Streamer and the Syncer stuff I think I need to look over that code before I can have an opinion about it but I do agree the configuring/reconfiguring would be pretty infrequent so having different modes seems reasonable. This sounds like a large change which I am happy to tackle when I have time but might be better off as a separate PR to keep things smaller?

The authentication stuff does not sound to complicated. I am happy to do implement it for this PR but I think it could also be a separate one. Let me know if I should tackle that before this PR gets merged.

Thanks!

@scottlamb

This comment has been minimized.

Copy link
Owner

commented Mar 20, 2019

I agree having the Streamer and Syncer stuff in a later PR makes sense. I haven't thought through the change in detail, so I don't want to claim it's small.

I think I don't want to commit this PR before the authentication change, because it's a regression in the sense that you can no longer have users who can view cameras but not mess with the config. I never deliberately regress the master branch, even if the fix commit happens immediately afterward.

We could introduce the admin flag (and ability to set it via the hopefully-soon-to-be-obsolete moonfire-nvr config ncurses UI) in one small PR, committed first. Then actually use it in this one. Or I suppose do it all in this PR; as you say it shouldn't be too complicated.

@jgautier

This comment has been minimized.

Copy link
Author

commented Mar 21, 2019

I'll add the authentication change to this PR and in addition I'll add a new api to add users in web.rs. Couple questions for you though since I have not looked much into this yet.

  1. Via moonfire-nvr config I never setup any users yet I am still able to access everything without logging in. I am guessing there is some code that doesn't require login if there are no users. Do we want to start requiring user accounts at some point before you can access the web interface? Do we want to allow management of cameras etc w/o having user accounts?

  2. For installs that already have user accounts since there is no admin flag yet how do we want to handle migrating from not having admins to having admins? Could we do something like automatically mark the first created as an admin if there are no admins?

@scottlamb

This comment has been minimized.

Copy link
Owner

commented Mar 22, 2019

  1. Great question. Hmm, the main reason I don't require authentication from the beginning is that I don't think it's good to use non-https sessions for authentication, and https is complex to setup (especially prior to #27). But allowing management of cameras etc w/o user accounts might be an unpleasant surprise. Maybe we could have the commandline argument say what to do explicitly. Something like --http-mode={user-auth,admin,viewer} (vs the current --require-auth boolean). (I'm open to other phrasings but something along those lines anyway.) The more secure default is user-auth anyway (the current default is insecure).

  2. Hmm, the first user might not be the right one to upgrade (folks might want a separate root user, for example). I think we can just not have any admins on upgrade (again, default to secure). We probably shouldn't actually get rid of the current moonfire-nvr config until we have the Unix domain socket method I mentioned, so there should always be a way to make someone an admin.

@jgautier

This comment has been minimized.

Copy link
Author

commented Mar 27, 2019

Just wanted to give an update on this. I haven't had time to work on this for a while but I still plan on getting to it hopefully this weekend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.