-
Notifications
You must be signed in to change notification settings - Fork 71.9k
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
Base API - settings, basic auth, and experimental endpoints #35
Conversation
Work in progress, any help or feedback would be great. I'm planning to store the settings in mongo, and support any json values sent by the client. |
Slick work Jason, I like it! Should we add an advanced setting to enable Websockets over xhr-polling, when you know you can support it? "websockets": true? Are there any other settings we should add? 24 hour clock option? |
I'm planning to make the settings API very dumb, have it store/retrieve anything we throw at it, then when ever we need an option we can just use it with a default and let users override it. Trying to decide if we should let it handle nested json values and/or arrays. We could model that as a single settings doc in a settings collection, or more of a key/value model.
|
What do you think about my stab at extending your starting point? I have sample responses from: I have some validation happening on a parameter: Finally, I setup a PUT request test, which echos the JSON sent to the console. Use this CURL to test. |
I'll take a look it more in the morning, I'm thinking for this first PR we should do the bare minimum to get settings and maybe post/get entries in place. Probably some very basic auth too. Then as we need more settings, new data to post, etc it will be easy to extend. Also wondering if the phone post should be a post to entries with type = phone. |
I agree. I wasn't planning on trying to do everything at once. It is more of an experiment for my education. I just renamed my branch to api-experiments. https://github.com/brianhanifin/cgm-remote-monitor/tree/feature/api-experiments |
What were you thinking for authorization, OAuth2, HTTP Basic Authentication? Here is the opinion from the article Best Practices for Designing a Pragmatic RESTful API "However, this token-over-basic-auth method of authentication is only acceptable in cases where it's practical to have the user copy a token from an administration interface to the API consumer environment. In cases where this isn't possible, OAuth 2 should be used to provide secure token transfer to a third party. OAuth 2 uses Bearer tokens & also depends on SSL for its underlying transport encryption." Here are a couple of examples of OAuth2 implimentations: |
I was thinking of something much simpler than OAuth2. For OAuth2 you really should be exchange the authorization token for an access token server to server, if you do that client side your secret key is exposed. So you just did a bunch of work and really aren't any better off. For starters what if we just set a secret using an ENV VAR and then pass it to the api, if we're going to be using SSL that should be ok, any way better than doing nothing. Later we can add a admin section where we can create tokens, users, etc. Using HTTP Basic Auth as described in the link above seems good to me, it would still be hard to explore the api using a browser since we'll be using more than GET's. |
Setting an ENV VAR sounds OK, and easy enough to set on Azure or Heroku. The concern with passing the value in the get request is it could get stored in the server's log file. I have found a nice OS X GUI to use instead of constantly Googling Curl parameters. There is a tab to set Basic HTTP Auth. I also discovered a website to verify my responses are valid JSON. In my experimental branch I am currently experimenting with best practices for returning a validation error. Request zero entries
Request "ten" entries
|
I was also thinking of something a bit simpler, to build on what Jason
On the android side, you'd put the same secret phrase, and the code would This would provide a way to at least introduce the API and allow for some When we introduce better authorization and access controls, there are a lot -bewest |
So in Ben's scenerio, the request with a key of abcdefg woud look like this?
|
Something like that, if it's in the URL/query string it's easier to test, To be more clear, if the UPLOAD_SECRET=myupl0adsecret echo myupl0adsecret | sha1sum So the request would look something like this: GET http://localhost/api/.../c6eddc48a5cc43dbe45a1473925ea9aa79497b6c/... Changing the secrets across android phone and the web monitor is relatively The other places are the query string or the headers. The header is best With this approach the user would put the api base, and the secret phrase If upload_secret is defined, the sha1sum is used to expose an API. -bewest On Fri, Jul 4, 2014 at 1:20 PM, Brian Hanifin [email protected]
|
Instead of UPLOAD_SECRET I think it should be a general API_SECRET that can
|
Good name choice Jason. API_SECRET After some reading, it sounds like it is OK to send a 301 Permanent Redirect for API requests. Obviously, after the /api/v1/pebble endpoint is returning proper data we will need someone with a Pebble Watch to test it. If the redirect doesn't break the Pebble app, then we could lose the old pebble.js code without having to worry about breaking any Pebble apps that aren't using the new API.
This is fun, I'm really enjoying this collaboration. :) |
Why change the pebble endpoint now?
|
Agreed, no need to change /pebble. Lets add an /api/ similar to what people have been proposing. This is a simple addition of a feature, shouldn't really impact anything -bewest On Fri, Jul 4, 2014 at 2:15 PM, Jason Calabrese [email protected]
|
Because maintaining two copies of nearly identical code can cause problems in the long run. If my assumption is right, the /pebble and /entries return JSON is going to be identical (or nearly identical), then it would be easier to maintain one codebase instead of two. |
I don't think /entries will be identical, to /pebble, /entries will just
|
Yeah, I think of pebble as it's own app, not part of the api. In the future, after the api is in place, we can re-write the pebble -bewest On Fri, Jul 4, 2014 at 2:32 PM, Jason Calabrese [email protected]
|
I was operating under the assumption that /entries would return the records array, and /pebble would return the records array, wrapped in more information. See this Gist: |
But if we change the pebble app's behavior radically we don't want to Eg, if the pebble endpoint is not the same as the api, then we can add -bewest On Fri, Jul 4, 2014 at 2:37 PM, Brian Hanifin [email protected]
|
OK. I honestly thought the /pebble endpoint is already an API that simply returns JSON. I thought it was a natural addition to the API. I will leave /pebble alone unless otherwise directed. :) |
Ben, to clarify you are recommending an /api/v1/treatment endpoint? |
I think entries should be generic, and have a type. Then the collection is
|
Excuse me. I'm coming from a SQL database background so I'm new to loosey goosey databases. :) Where would this "event stream" be stored? In the primary collection? |
Yeah just throw it all in there and index the type and timestamp fields
|
Will all of the new data (e.g. settings) be stored in the same collection?
|
Settings are something else, so a different collection. Same for users or any other admin like things. I think all the data points that we want to show on the timeline should be
|
If we can populate the |
Howdy On Fri, Jul 4, 2014 at 4:29 PM, Jason Calabrese [email protected]
This might be a little tricky.
The visualization expects treatment elements to have the delivered from the Very open to ideas here. I worked out most of the obvious remaining bugs -bewest
|
I am trying to wrap my head around the recommended Git branching model. I think I have a decent understanding of it, but I don't see a "develop" branch. I am thinking about splitting the "feature/base-api" branch off into "feature/settings-ui", so I can start experimenting with a settings interface. If I do that should feature/base-api be merged into a "develop" branch. |
The actual name of the branch doesn't matter so much. Everyone seems pleased with If I were you I'd do this: git checkout feature/base-api
git pull
git checkout -b feature/settings-ui The really special things to keep in mind is that touching |
If we get more than 6 or 7 pull requests consistently at the same time from inside the repo, I'll ask people to host their branches on their own forks and pull into a more central |
Did someone start using a module "type-is"? I am getting an error this morning which was resolved by installing it. Did I do something wrong?
|
Strange, no stack trace? Did you try just |
|
Ah-ha! Depenencies of body-parser includes "type-is". So, does sudo npm update not install a package's dependancies? |
When you run Right thing to do after pulling merges is generally just |
See I tried doing npm install but I have been getting errors, which went away when I added sudo. "npm install" output
|
Ah, ok, quick fix: $ sudo rm -rf node_modules
$ npm install Here's what happens:
All of these files are owned by you. Then, the first time you use
Watch what happens when a merge update expects
So if you completely remove your node modules and just do |
Sorry to be a pain, but I deleted my node_modules folder and now running *npm install gives me this, which includes the recommendation "npm ERR! Please try running this command again as root/Administrator."
|
OK, I have an idea. I am trying to add my dev user to the "Sharing and Permissions" list for that folder on OS X. |
Best solution is to delete the files in user owned directories that are created by npm but owned by root, and installing again without In your scenario, you can follow up with:
And probably try again with success. |
Setting up sharing might work but defeats the point of having the permissions... the insane modal interface corrupted your permissions, once fixed it should work again. |
If you are using a separate dev account, I recommend creating a group and adding you and your dev account to the same group, then your group will have shared read/write and things will also work well. |
Thank you! That last command did the trick! I have a Macbook Pro with the OS and apps on an SSD and everything else on a larger hard disk. I certainly didn't mean to do get things all screwed up. I am so used to running Unix commands using sudo I must have done it out of habit. |
I've done the Exact. Same. Thing. Repeatedly. Modal interfaces considered harmful. I now avoid using |
Want to add tests to this, should be ready for testing, will hopefully get a chance to get Brandon's rig using this soon. |
Battery level? Interesting. I could play with adding a battery glyph. |
@jasoncalabrese pointed out that some agents do odd things with underscores and that using a hyphen is more common. This also adds tests to ensure that the API_SECRET handling works as expected.
Cherry-picked in the wrong order This reverts commit 8e56ef1.
Add Settings
Base API - settings, basic auth, and experimental endpoints
Navid 2022 11 16 test
No description provided.