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

Controller methods & railtie #2

Merged
merged 10 commits into from
Oct 1, 2014
Merged

Controller methods & railtie #2

merged 10 commits into from
Oct 1, 2014

Conversation

eveadele
Copy link
Contributor

@eveadele eveadele commented Oct 1, 2014

I made a module for current_user and user_logged_in? methods, and added a railtie to include that module and the rack middleware.

def current_user
if request.env['prx.auth']
User.find_by(id: request.env['prx.auth'].user_id)
else
Copy link
Contributor

Choose a reason for hiding this comment

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

this else clause should be redundant

@cqr
Copy link
Contributor

cqr commented Oct 1, 2014

Looks mostly good.

First, I don't think we can realistically define a current_user method because it's likely to be application dependent how that gets defined. I'd say the method we should define is prx_auth_token, which just returns the wrapper you have defined here.

Also, instead of user_logged_in, let's call it prx_authenticated? and we should add some tests

@cqr
Copy link
Contributor

cqr commented Oct 1, 2014

@eveadele Looks great to me.

@kookster did you have something else in mind for the controller stuff? I don't know that it makes sense to go much further than this (except maybe in the future doing things like automatically querying the /userinfo endpoint) since we don't know whether we'll have access to the users table or something like it.

cqr added a commit that referenced this pull request Oct 1, 2014
Controller methods & railtie
@cqr cqr merged commit f07ef9d into master Oct 1, 2014
@cqr cqr deleted the controller_methods branch October 1, 2014 18:59
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.

2 participants