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

Contributions #15

Closed
lepture opened this issue Jun 19, 2013 · 32 comments

Comments

@lepture
Copy link
Owner

commented Jun 19, 2013

A directory for contributions.

__init__.py
client.py
provider/
contrib/

We could put SQLAlchemy built-in models in it, as well as a cache Grant token model.

Related #12.

@Taar

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2013

We'll need to make sure we give the option to use flask-sqlalchemy or just SQLAlchemy by its self. We'll also need to accept a Session object or if it's configured, using the Model.query method if no Session is passed.

@lepture

This comment has been minimized.

Copy link
Owner Author

commented Jun 19, 2013

@Taar

We'll need to make sure we give the option to use flask-sqlalchemy or just SQLAlchemy by its self

Do you have any idea on how to do it?

@Taar

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2013

@lepture yeah I have a pretty good idea on how to implement that. Both wtforms-alchemy and wtforms.ext.sqlalchemy have great examples in how to implement SQLAlchemy into the extension.

I think it would be best if the user defined their own models so we don't have to worry about that. We'll just have to make a handler class that uses those models. So possibly something like this (my rough idea on how it will all work):

from flask_oauthlib.provider import OAuth2Provider
from flask_oauthlib.contrib import (
    UserHandler,
    ClientHandler,
    TokenHandler,
    GrantHandler
)
from myapp import app, db
from myapp.models import User, Client, Token


oauth = OAuth2Provider(app)

# each handler will have validation so the user doesn't have to worry about it
# themselves :D
# Of course each handler will except the Model to have certain methods and
# properties which will be outlined in the documentation.
# `session` can be omitted which will cause the handler to use Model.query
# instead. If `session` was set and Model.query is avaliable than it will just
# user Model.query else sesseion.query(Model) will be used
user_handler = UserHandler(session=db.session, model=User)
client_handler = ClientHandler(session=db.session, model=Client)
token_handler = TokenHandler(session=db.session, model=Token)

# Like flask-cache the settings for each Cache system will be set in the app's
# config! All the user will have to do is set 'OAUTH_CACHE_TYPE' to the cache
# system they would like to use. Additional config can be passed in through the
# config param.
# the grant_handler will return a Grant object which will have all the correct
# properties and methods
grant_handler = GrantHandler(app=app, config=None)


@oauth.usergetter
def get_user(username=None, password=None):
    return user_handler.get(username, password)


@oauth.clientgetter
def get_client(client_id):
    return client_handler.get(client_id)


@oauth.tokengetter
def get_token(access_token=None, refresh_token=None):
    return token_handler.get(access_token=access_token,
                             refresh_token=refresh_token)


@oauth.tokensetter
def set_token(token, request, *args, **kwargs):
    token_handler.set(token, request.client, request.user)


@oauth.grantgetter
def get_grant(client_id, code):
    return grant_handler.get(client_id, code)


@oauth.grantsetter
def set_grant(client_id, code, request, *args, **kwargs):
    return grant_handler.set(client_id, code, request.user, request.scopes)

This approach still leaves the option to define your own getter and setter functions if the user so desires. And the user can also choose which handler they would like to use if they want to define their own getters and setters for a few but not all.

Having the user define their own models means we don't have to worry about if they are using flask-sqlalchemy or not since the handlers only need the session and Model class.

@lepture

This comment has been minimized.

Copy link
Owner Author

commented Jun 20, 2013

@Taar Great idea. I think we can do more:

oauth = OAuth2Provider(app)

user_handler = UserHandler(session=db.session, model=User, provider=oauth)
client_handler = ClientHandler(session=db.session, model=Client, provider=oauth)
token_handler = TokenHandler(session=db.session, model=Token, provider=oauth)

And then developers don't have to add those getters and setters.

@lepture

This comment has been minimized.

Copy link
Owner Author

commented Jun 20, 2013

@Taar GitHub can't find your commit log, you need to bind your email at https://github.com/settings/emails

@Taar

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2013

@lepture Yes that would work nicely. Now do you think we could combine those three classes into one (UserHandler, ClientHandler and TokenHandler)? Maybe something like this:

OAuth2SQLAlchemyHandler(session=db.session, user_model=User, client_model=Client,
                                    token_model=Token, provider=oauth) 

I'm not sold on the class name (OAuth2SQLAlchemyHandler). It needs to be something more descriptive, possibly shorter.

@lepture

This comment has been minimized.

Copy link
Owner Author

commented Jun 20, 2013

@Taar Why not just:

OAuth2Model(session=db.session, user=User, client=Client, grant=Grant, token=Token, provider=oauth)

We can have built-in Client and Token models, if you didn't assign a token parameter, we can use the default one.

We also has a built-in Grant model based on cache, but you can replace it in OAuth2Model.

@Taar

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2013

@lepture yeah that could work. Though I'm still not sure about the built-in Client and Token models, The OAuth2Model would have to check to see if the tables exists and if they don't than they will have to be created. However, I think this can be avoided if the there is a way for the user to create the tables before hand. That way OAuth2Model will assume they are already created and SQLAlchemy will throw an error if they aren't. Even so, I'm still not sure if there should be built in Models.

lepture added a commit that referenced this issue Jun 20, 2013

@lepture

This comment has been minimized.

Copy link
Owner Author

commented Jun 20, 2013

@Taar I think functions work well. Do we really need a class?

@Taar

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2013

@lepture I don't think it has to be a class. However, a class would allow the developer to extend the functionality if they so desire (Might not be very likely but I'm sure they'll be a couple cases).

Currently I have a rough prototype but I've ran into a couple issues. For the tokensetter the user will have to provide a get_current_user() method. I've tried to find a work around so that they wouldn't have to provide a means to get the current user but I've been unsuccessful. Do you have any ideas on how we could do this?

Secondly, for the usergetter we can't assume which fields are needed to get the user object. For example, most applications (all the ones I've worked on at least) use an email for the username which means the username property doesn't exist. A work around to this issue could be as follows:

# Method of OAuth2Model ...
    def get_user(self, username, password, *args, **kwargs):
        user = self.user_model.get(username)
        if user.check_password(password):
            return user
        return None

# models.py ...
class User(db.Base, UserMixin):

    id = Column(Integer, primary_key=True)
    email = Column(Unicode, unique=True, nullable=False)
    first_name = Column(Unicode, nullable=False)
    last_name = Column(Unicode, nullable=False)
    password = Column(String, nullable=False)
    admin = Column(Boolean, default=False)

    def __init__(self, first_name=None,
                 last_name=None, email=None):
        self.first_name = first_name
        self.last_name = last_name
        self.email = email

    def set_password(self, password):
        self.password = generate_password_hash(password)

    # This method is required
    def check_password(self, password):
        return check_password_hash(self.password, password)

    @classmethod
    def get(cls, username):
        return cls.query.filter_by(email=username).first()

I don't really think this approach would save the developer a whole lot of time. So the question is do we force the developer to have a username field in User or do something like I have above? Or should be omit the User model and have them define the usergetter?

@lepture

This comment has been minimized.

Copy link
Owner Author

commented Jun 25, 2013

You don't need a get_current_user for tokensetter, instead, you need it for grantsetter.

Tokensetter accepts (token, request, *args, **kwargs), and you can find user model on request.user.

For the second doubt, we can do like this:

def get_user(username, password, *args, **kwargs):
    if '@' in username and hasattr(cls.user_model, 'email'):
        return query_by_email
    elif '@' not in username and hasattr(cls.user_model, 'username'):
        return query_by_username
    return None
@Taar

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2013

@lepture Oh you're correct, I was thinking of the wrong setter.

I believe we shouldn't try to check if the User model has an username field or an email field but rather assume it has an username field and a check_password method. If the developer's User model has different methods or fields then they should use the oauth.usergetter decorator.

In the next few days I'm hoping to have everything complete and ready to go (with unit tests). I'll have to spend some additional time writing the documentation.

Below is the base usage for what I have so far:

oauth = OAuth2Provider(app)

# Any of the models can be omitted if the developer plans to write their own.
# If the grant model is passed so must the get_current_user function.
# session and provider are required arguments
SQLAlchemyHandler(db.session, oauth, user=User, client=Client, token=Token,
                               grant=Grant,  get_current_user=get_current_user)

# `GrantCacheHandler` handler that uses `Werkzeug.contrib.cache` system instead of
# a SQLAlchemy model for the grant
# All of the configuration for the cache system can be set in `app.config`
# eg. `app.config['OAUTH2_CACHE_REDIS_PORT'] = 7000`
# The developer can also pass in additional configuration in the `config` argument
# eg. GrantCacheHandler(app, get_current_user, config={'OAUTH2_CACHE_REDIS_DB': 1})
# They'll need to set `OAUTH2_CACHE_SYSTEM` to one of the following values:
# null, simple, redis, memcache, and filesystem. This will determine which cache system will be to used.
GrantCacheHandler(app, get_current_user, config=None)

It's definitely a work in progress! Still a few bugs I need to sort out. What do you think about the class names? I keep messing around with different names but I can't come up with something that seems to fit.

@lepture

This comment has been minimized.

Copy link
Owner Author

commented Jun 25, 2013

@Taar Great. I am focusing on OAuth1 provider now. And I am leaving this part to you. Agreed with the usergetter.

I am wondering why should we call it handler? I prefer binding instead.

@Taar

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2013

Just a quick update, the bindings are complete. I just need to write all the docs, write additional tests for all the use cases and add in more logging for debugging. You can see what I've done so for on my bindings branch. I'm hoping to have everything else complete over the weekend.

Once that's all done, I plan on adding in support for OAuth1 provider.

@lepture

This comment has been minimized.

Copy link
Owner Author

commented Jul 5, 2013

@Taar Great job.

There is a problem on the string format, because I am using the % pattern, and you are using the format method. I am not sure which is better, maybe I should change all % pattern to format method.

{}.format not working on Python 2.6, use {0}.format instead.

@lepture

This comment has been minimized.

Copy link
Owner Author

commented Jul 5, 2013

@Taar Suggestion on to_dict, we can implement the Grant:

class Grant(object):
    # ....
    def __getitem__(self, item):
        return getattr(self, item)

    def keys(self):
        return ['client_id', 'code', '...']

And then we can get the dict with dict(grant), which I think is better.

BTW, the title in doc string is wrong.

@Taar

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2013

@lepture Very cool, didn't know you could do something like that.

Yeah there are a lot of improvements to be made on the code base which I will address this evening.

As for the format method, I personally prefer it over %. I think using format makes the code much more readable. Here are some of the advantages of using format over % http://stackoverflow.com/questions/5082452/python-string-formatting-vs-format.

@lepture

This comment has been minimized.

Copy link
Owner Author

commented Jul 10, 2013

@Taar How is everything going? We are moving to 0.3.1 now.

@Taar

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2013

@lepture I should have everything complete and ready to go tonight.

@lepture

This comment has been minimized.

Copy link
Owner Author

commented Jul 10, 2013

@Taar That's good news.

One more thing, can we use the configuration of Flask-Cache? That means if we didn't set a OAUTH2_CACHE_TYPE we can guess from CACHE_TYPE.

@Taar

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2013

@lepture I'm not sure if that would be a good idea. The developer might want flask-oauthlib to use different settings then flask-cache.

I might not get the oauth1 bindings completely done tonight since I still have quite a bit of documentation to write for the oauth2 bindings and I also need to rework my tests to fit the new structure you're put in place. Worst case everything will be complete by friday evening.

@lepture

This comment has been minimized.

Copy link
Owner Author

commented Jul 10, 2013

@Taar

The developer might want flask-oauthlib to use different settings then flask-cache.

It is a fallback, if use has set a OAUTH2_CACHE_TYPE, use this one, if not use CACHE_TYPE.

I might not get the oauth1 bindings completely done tonight.

That's absolutely ok. You can split them in two pull requests, one for oauth2, the other for oauth1. You can send oauth2 first, so that I can have a review earlier.

Thanks for all you have done.

@Taar

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2013

@lepture Oh I see what you mean. I'll be sure to add that in.

No problem! It's been really fun working on this project!

@Taar

This comment has been minimized.

Copy link
Contributor

commented Jul 13, 2013

@lepture alright here is my final version! https://github.com/Taar/flask-oauthlib/tree/bindings.

Writing documentation isn't my strong suit so if there is something I should change please let me know! I'm planning on working on the OAuth1 bindings this weekend. I'm not really sure when I'll have it complete at this moment.

@lepture

This comment has been minimized.

Copy link
Owner Author

commented Jul 15, 2013

@Taar Sorry for the delay, it's weekend.

Why do you need get_session, why not just session?

@Taar

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2013

@lepture It appears that get_session isn't needed at all. I was under the impression that if stored in an property it would behaved similar to Model.query. I'll go ahead and update the code to just use session.

@lepture

This comment has been minimized.

Copy link
Owner Author

commented Jul 17, 2013

@Taar How should we name the oauth1 bindings? Should we rename bindings.py to oauth2.py?

@Taar

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2013

@lepture yeah I was planning on renaming binding.py to something else and split up oauth1 and oauth2 bindings into their own files.

@lepture

This comment has been minimized.

Copy link
Owner Author

commented Jul 17, 2013

@Taar They will share the same cache config, so OAUTH2_CACHE_XXX should be OAUTH_CACHE_XXX now.

And GrantCacheBinding should be a Cache just like Flask-Cache does.

@Taar

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2013

@lepture yup 👍 I'll probably have everything ready by the end of this weekend.

@lepture

This comment has been minimized.

Copy link
Owner Author

commented Jul 30, 2013

@Taar Any news on the OAuth1 part?

@Taar

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2013

@lepture sorry it's been taking so long. I've been extremely busy as of late and only have had time to work on it a little bit at a time. I'm hoping to have it done soon.

@lepture lepture closed this Nov 9, 2013

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