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

Feature: redirect after login/register with OAuth #910

Merged
merged 4 commits into from Mar 8, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions flask_appbuilder/security/decorators.py
@@ -1,7 +1,7 @@
import logging
import functools

from flask import flash, redirect, url_for, make_response, jsonify
from flask import flash, redirect, url_for, make_response, jsonify, request
from .._compat import as_unicode
from ..const import LOGMSG_ERR_SEC_ACCESS_DENIED, FLAMSG_ERR_SEC_ACCESS_DENIED, PERMISSION_PREFIX

Expand All @@ -27,7 +27,7 @@ def wraps(self, *args, **kwargs):
else:
log.warning(LOGMSG_ERR_SEC_ACCESS_DENIED.format(permission_str, self.__class__.__name__))
flash(as_unicode(FLAMSG_ERR_SEC_ACCESS_DENIED), "danger")
return redirect(url_for(self.appbuilder.sm.auth_view.__class__.__name__ + ".login"))
return redirect(url_for(self.appbuilder.sm.auth_view.__class__.__name__ + ".login", next=request.url))

Choose a reason for hiding this comment

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

I see this fix for only AuthOAuthView, but how do ensure this next=request.url variable will be utilized in AuthDBView, AuthLDAPView,
AuthRemoteUserView and they are sent to the index page, instead of the page they tried to visit initially.

Copy link
Owner

Choose a reason for hiding this comment

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

True @ankursinghal2005, One would have to replicate the same logic to all child classes.

Hey @betodealmeida are willing to do it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not the right person to do it, since I don't have access to an LDAP auth server. Maybe @ankursinghal2005 can do it for the auth system they're using?

Copy link

Choose a reason for hiding this comment

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

@betodealmeida
I have tested below LDAP test server. It is working fine. You can see it for your reference. I can further work on remaining child classes after this PR gets merged into master.

Online LDAP Test Server Link
https://www.forumsys.com/tutorials/integration-how-to/ldap/online-ldap-test-server/

When using LDAP Auth, setup the ldap server

AUTH_TYPE = AUTH_LDAP
AUTH_LDAP_SERVER = "ldap://ldap.forumsys.com:389"
AUTH_LDAP_SEARCH = "dc=example,dc=com"
AUTH_LDAP_BIND_USER = "uid=riemann,dc=example,dc=com"
AUTH_LDAP_BIND_PASSWORD = "password"

Copy link
Owner

Choose a reason for hiding this comment

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

nice!!

f._permission_name = permission_str
return functools.update_wrapper(wraps, f)

Expand Down Expand Up @@ -56,7 +56,7 @@ def wraps(self, *args, **kwargs):
'severity': 'danger'}), 401)
response.headers['Content-Type'] = "application/json"
return response
return redirect(url_for(self.appbuilder.sm.auth_view.__class__.__name__ + ".login"))
return redirect(url_for(self.appbuilder.sm.auth_view.__class__.__name__ + ".login", next=request.url))
f._permission_name = permission_str
return functools.update_wrapper(wraps, f)

Expand Down
6 changes: 3 additions & 3 deletions flask_appbuilder/security/manager.py
Expand Up @@ -839,9 +839,9 @@ def auth_user_oauth(self, userinfo):
if not user:
user = self.add_user(
username=userinfo['username'],
first_name=userinfo['first_name'],
last_name=userinfo['last_name'],
email=userinfo['email'],
first_name=userinfo.get('first_name', ''),
last_name=userinfo.get('last_name', ''),
email=userinfo.get('email', ''),
role=self.find_role(self.auth_user_registration_role)
)
if not user:
Expand Down
30 changes: 28 additions & 2 deletions flask_appbuilder/security/views.py
Expand Up @@ -7,6 +7,7 @@
from wtforms.validators import EqualTo
from flask_babel import lazy_gettext
from flask_login import login_user, logout_user
import jwt

from ..views import ModelView, SimpleFormView, expose
from ..baseviews import BaseView
Expand Down Expand Up @@ -503,11 +504,23 @@ def login(self, provider=None, register=None):
appbuilder=self.appbuilder)
else:
log.debug("Going to call authorize for: {0}".format(provider))
state = jwt.encode(
request.args.to_dict(flat=False),
self.appbuilder.app.config['SECRET_KEY'],
algorithm='HS256')
try:
if register:
log.debug('Login to Register')
session['register'] = True
return self.appbuilder.sm.oauth_remotes[provider].authorize(callback=url_for('.oauth_authorized',provider=provider, _external=True))
if provider == 'twitter':
return self.appbuilder.sm.oauth_remotes[provider].authorize(
callback=url_for
('.oauth_authorized', provider=provider, _external=True, state=state))
else:
return self.appbuilder.sm.oauth_remotes[provider].authorize(
callback=url_for
('.oauth_authorized', provider=provider, _external=True),
state=state)
except Exception as e:
log.error("Error on OAuth authorize: {0}".format(e))
flash(as_unicode(self.invalid_login_message), 'warning')
Expand Down Expand Up @@ -550,7 +563,20 @@ def oauth_authorized(self, provider):
return redirect('login')
else:
login_user(user)
return redirect(self.appbuilder.get_url_for_index)
try:
state = jwt.decode(
request.args['state'],
self.appbuilder.app.config['SECRET_KEY'],
algorithms=['HS256'])
except jwt.InvalidTokenError:
raise AuthenticationError('State signature is not valid!')

try:
next_url = state['next'][0]
except (KeyError, IndexError):
next_url = self.appbuilder.get_url_for_index

return redirect(next_url)


class AuthRemoteUserView(AuthView):
Expand Down
Expand Up @@ -5,8 +5,9 @@

<script type="text/javascript">

var baseLoginUrl = {{url_for('AuthOAuthView.login')}};
var baseRegisterUrl = {{url_for('AuthOAuthView.login')}};
var baseLoginUrl = "{{url_for('AuthOAuthView.login')}}";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was interesting... we're missing the quotes, so this actually gets translated to:

var baseLoginUrl = /login/;

Which works because it defines a regular expression that later gets cast to a string when it's concatenated to currentSelection!

Copy link
Owner

Choose a reason for hiding this comment

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

God...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's ok, I blame Javascript. :)

Copy link

Choose a reason for hiding this comment

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

Hello @dpgaspar @betodealmeida ,
I have following test case scenario , is it taken care in testing or not?

If some one logged in to FlaskAppBuilder using twitter account and in between someone delete/disable his twitter account then is user still able to use FlaskAppBuilder app (if he did not logout manually) or get Access Denied Error and How we are validating user authenticity in running application.

var baseRegisterUrl = "{{url_for('AuthOAuthView.login')}}";
var next = "?next={{request.args.get('next', '')}}"

var currentSelection = "";

Expand All @@ -20,13 +21,13 @@

function signin() {
if (currentSelection != "") {
window.location.href = baseLoginUrl + currentSelection;
window.location.href = baseLoginUrl + currentSelection + next;
}
}

function register() {
if (currentSelection != "") {
window.location.href = baseRegisterUrl + currentSelection + '/register';
window.location.href = baseRegisterUrl + currentSelection + '/register' + next;
}
}

Expand Down
1 change: 1 addition & 0 deletions rtd_requirements.txt
Expand Up @@ -8,3 +8,4 @@ Flask-Login>=0.3,<0.5
Flask-OpenID>=1.2.5,<2
Flask-SQLAlchemy>=2.3,<3
Flask-WTF>=0.14.2,<1
PyJWT>=1.7.1
1 change: 1 addition & 0 deletions setup.py
Expand Up @@ -44,6 +44,7 @@ def desc():
'Flask-SQLAlchemy>=2.3,<3',
'Flask-WTF>=0.14.2,<1',
'python-dateutil>=2.3,<3',
'PyJWT>=1.7.1',
],
tests_require=[
'nose>=1.0',
Expand Down