Skip to content
This repository has been archived by the owner on Nov 5, 2019. It is now read-only.

Flask 3LO helper #226

Merged
merged 1 commit into from
Jul 31, 2015
Merged

Flask 3LO helper #226

merged 1 commit into from
Jul 31, 2015

Conversation

theacodes
Copy link
Contributor

This should fix #224 once merged.

Currently rough and looking for feedback. This intentionally deviates from the appengine/webapp2 decorator to be more idiomatic for flask users.

  1. The credentials are currently only stored on the session. I'd like some feedback on how to allow the user to specify storage, and how the session storage should fix into that (should it store both in the session and within the user provided storage?)
  2. I'm aware that it doesn't quite fit with the style of the rest of the codebase, I'll adjust to 2 spaces etc before merge.
  3. The scopes default to 'email' and 'profile', and there's a mechanism to fetch the user's full profile from google+. I can remove this if we feel it's bloated (users can easily add it back via authorize_callback).

Tests will be added once I validate this approach.

@theacodes theacodes force-pushed the master branch 2 times, most recently from 26112aa to eaeab39 Compare July 23, 2015 22:54
@elibixby
Copy link
Contributor

Looks great!

First thoughts:

I like configurable defaults. Specifically, you might want to to be able to do something like

    @app.route('/login/<custom_return_path>')
    @oauth2.required(scopes=['email', 'somethingelse'], return=custom_return_path)
    def login():
         # Do stuff

Which would make it really easy to implement e.g. a login widget that you throw on every page, but lands you on a specific page (not necesarily the same one) based on where you logged in.

@dhermes
Copy link
Contributor

dhermes commented Jul 24, 2015

Change flask.py to something else, like flask_util.py or similar?

# limitations under the License.

"""Utilities for the Flask web framework
"""

This comment was marked as spam.

This comment was marked as spam.

@theacodes theacodes force-pushed the master branch 3 times, most recently from 6b5ac6a to 204850f Compare July 29, 2015 21:24

app.register_blueprint(self._create_blueprint())

#app.after_request(self._update_session)

This comment was marked as spam.

This comment was marked as spam.

@theacodes theacodes force-pushed the master branch 2 times, most recently from 444d0a3 to f7cd1b2 Compare July 29, 2015 22:44
@theacodes
Copy link
Contributor Author

@anthmgoogle modified so that state is now json and stores the return url there to bring it in-line with your recommendations. I still need your feedback on the scopes parameter for incremental auth.

@nathanielmanistaatgoogle
Copy link
Contributor

LGTM; @anthmgoogle will also review.

@theacodes
Copy link
Contributor Author

Thanks, @nathanielmanistaatgoogle and @dhermes for the thorough review. 👍

DEFAULT_SCOPES = ('email',)


class OAuth2(object):

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@anthmgoogle
Copy link

Finished a review pass. A could of comments related to the public surface.

@theacodes theacodes force-pushed the master branch 2 times, most recently from f4d337c to 8eb598d Compare July 30, 2015 22:47
@theacodes
Copy link
Contributor Author

@anthmgoogle Alright, that should do it. Can you take a final look?

@anthmgoogle
Copy link

LGTM.

@dhermes
Copy link
Contributor

dhermes commented Jul 31, 2015

I have merge privileges. @jonparrott did you get everyone's sign off?

@theacodes
Copy link
Contributor Author

We have you, @nathanielmanistaatgoogle and @anthmgoogle. That's everyone.

Thanks, everyone! 🎆

@dhermes
Copy link
Contributor

dhermes commented Jul 31, 2015

Wow. The saga is over!

dhermes added a commit that referenced this pull request Jul 31, 2015
@dhermes dhermes merged commit bb65453 into googleapis:master Jul 31, 2015
@dhermes dhermes mentioned this pull request Aug 26, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement Decorator 3LO pattern for Flask
7 participants