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

First working API implementation #51

Closed
wants to merge 27 commits into from

Conversation

jameswthorne
Copy link
Contributor

This is my attempt at adding an API to SnapPass. I used this implementation about 8 months ago when I needed to generate a lot of SnapPass URLs for a project at my previous employer.

It was previously mentioned by one of the maintainers they wanted to keep apart the web form code and any API code. This doesn't currently do that, but I am interested in discussing the appropriate way to do that.

@jameswthorne
Copy link
Contributor Author

Now that SnapPass supports multi-line passwords/secrets, I don't know the best way to handle that when POSTing via curl.

Copy link
Collaborator

@jparise jparise left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the error handling could use a rethink. Flask has a nice errorhandle system that delegates error response building to registered functions. That way, the view functions can just abort(status_code).

snappass/main.py Outdated
@@ -6,7 +6,7 @@
import redis
from redis.exceptions import ConnectionError

from flask import abort, Flask, render_template, request
from flask import abort, Flask, render_template, request, jsonify
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we retain the alphabetized order?

snappass/main.py Outdated
resp = jsonify(data)
resp.status_code = 200

return resp
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can just do:

return jsonify(data)

... because the response defaults to 200.

snappass/main.py Outdated
if NO_SSL:
base_url = request.url_root
else:
base_url = request.url_root.replace("http://", "https://")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These four lines are repeated in multiple endpoint functions. Can we factor them into a common make_base_url() function?

Alternatively, we could just require flask-sslify, which would move this concern out of snappass.

snappass/main.py Outdated
@@ -90,12 +90,53 @@ def request_is_valid(request):
"""
return not SNEAKY_USER_AGENTS_RE.search(request.headers.get('User-Agent', ''))

def not_found_api(error=None):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error parameter doesn't appear to be passed or used in any code paths.

snappass/main.py Outdated
resp = jsonify(message)
resp.status_code = 404

return resp
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flask has a nice shortcut for this pattern:

return make_response(jsonify(message), 404)

@jameswthorne
Copy link
Contributor Author

I'm working on applying your suggested fixes.

Is there a way to not have the abort() function output HTML?

@jparise
Copy link
Collaborator

jparise commented Jan 13, 2017

Is there a way to not have the abort() function output HTML?

Yes, that's where the errorhandler bit comes in:

@app.errorhandler(404)
def not_found(error):
    message = {
        'status': error.status_code,
        'message': 'Not Found: ' + request.url,
    }

    return make_response(jsonify(message), error.status_code)

@nichochar
Copy link
Collaborator

Hey just letting you know this is not dead. I just need to find a little time to review it, probably in the next week or so!

@jameswthorne
Copy link
Contributor Author

Hey @nichochar, no problem! No rush at all. I haven't had a chance to dive back into this yet.

@ghost
Copy link

ghost commented Nov 1, 2017

Any progress on this one?

@bryanpost
Copy link

Any progress on this getting rolled into the main build?

snappass/main.py Outdated
if not len(password) > 0:
return bad_request_api()
else:
return bad_request_api()
Copy link
Contributor

@coyotwill coyotwill Feb 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty complex logic here. How about something like :

password = payload.get('password', None)
if not password: #edit: not need to check len since empty strings are falsy   
    return bad_request_api()

snappass/main.py Outdated
ttl = time_conversion[time_period]
else:
# Set ttl to one week if not specified in the JSON
ttl = 604800
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here, could do time_period = payload.get('ttl', 'week').lower()

snappass/main.py Outdated
base_url = make_base_url()

return "Generate a password share link with the following command: \n\n" \
"curl -X POST -d \'{\"password\":\"password-here\",\"ttl\":\"week | day | hour\"}\' -H \"Content-Type:application/json\" " + base_url + "api\n"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: using single quotes to quote the string would allow to put unescaped double quotes in it. Makes it easier to read. 'curl -X POST -d \'{"password":"password-here","ttl":"week | day | hour"}\' -H ...'

@nichochar
Copy link
Collaborator

Looking back at this, it looks pretty good. One thing that is missing that I'd love to see is some tests.

I think that with a rebase and some tsts we can pull it in @jameswthorne. Let me know if you're still interested in contributing, we've been a little slow to respond! If not I'll take this over and push it through, since I think an API is a good idea.

@jameswthorne
Copy link
Contributor Author

Hey @nichochar

It's been sometime since I've looked over the code. I will try and find sometime this week to rebase and review.

@nichochar
Copy link
Collaborator

Awesome thanks @jameswthorne. Let me know if this is hard for you and I'm happy to commandeer the PR, it's up to you!

@jameswthorne
Copy link
Contributor Author

I rebased from the latest master and incorporated suggested fixes from @coyotwill.

I still need to incorporate the necessary tests.

@jameswthorne
Copy link
Contributor Author

How life gets in the way. I'm planning on rebasing from master and writing the tests this month.

@silverl
Copy link

silverl commented Jul 5, 2019

Came here looking for an API for SnapPass. Any progress on getting this baked in? Thank you.

@Dragonpark
Copy link

Was hoping there would be an update to this, but it seems the PR author has gone silent. Would be great if somebody could pick this back up.

gsacre pushed a commit to gsacre/snappass that referenced this pull request Mar 22, 2021
The API implementation from jameswthorne (https://github.com/jameswthorne) has
been re-imported into the latest version of snappass.

Pull request from jameswthorne can be found here:

	pinterest#51
@jameswthorne
Copy link
Contributor Author

I have no plans to revisit this PR. Someone else please feel free to adapt my code. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants