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

Conversation

betodealmeida
Copy link
Contributor

Currently, after a user goes through the authentication flow they are sent to the index page, instead of the page they tried to visit initially.

I improved the flow by storing the initial URL in a state parameter, that is signed and sent to the OAuth provider [see docs]. One the user has been authenticated, the state parameter received back from the provider is used to redirect them to the initial URL.

Note that the state is signed, to avoid a malicious provider of redirecting the user to a URL they control.

I tested the workflow with Superset, and it works as expected. I only tested with Google OAuth.

Copy link
Collaborator

@mistercrunch mistercrunch left a comment

Choose a reason for hiding this comment

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

This LGTM though I'm not an expert on auth/oauth/jwt, @dpgaspar should have the final word

@coveralls
Copy link

coveralls commented Feb 12, 2019

Coverage Status

Coverage decreased (-0.1%) to 70.224% when pulling 7d80f7e on betodealmeida:VIZ-237 into c665558 on dpgaspar:master.

@@ -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.

@dpgaspar
Copy link
Owner

Looks good, I'll test it with some other providers.

@dpgaspar
Copy link
Owner

Hi,

Testing with google oauth provider, using a simple login I get redirected to http://localhost:8080/None

Can you help?

@betodealmeida
Copy link
Contributor Author

Yeah, let me take a look.

@dpgaspar
Copy link
Owner

thks!

@betodealmeida
Copy link
Contributor Author

@dpgaspar I fixed the None issue. I tested with the Flask-AppBuilder-Skeleton repo and it works as expected now.

@betodealmeida
Copy link
Contributor Author

Ping @dpgaspar

@dpgaspar
Copy link
Owner

dpgaspar commented Mar 4, 2019

It fails on twitter:

2019-03-04 19:18:51,910:DEBUG:flask_appbuilder.security.views:User info retrieved from twitter: {'username': 'twitter_danielvazgaspar'}
2019-03-04 19:18:51,910:DEBUG:flask_appbuilder.security.views:No whitelist for OAuth provider
2019-03-04 19:18:52,013:INFO:flask_appbuilder.security.sqla.manager:Added user twitter_danielvazgaspar
2019-03-04 19:18:52,074:INFO:flask_appbuilder.security.sqla.manager:Updated user  
2019-03-04 19:18:52,098:INFO:werkzeug:127.0.0.1 - - [04/Mar/2019 19:18:52] "GET /oauth-authorized/twitter?oauth_token=XXX&oauth_verifier=YYYY HTTP/1.1" 500 -
Traceback (most recent call last):
  File "/home/dpgaspar/workarea/YYYY/venv3/lib/python3.6/site-packages/Flask-1.0.2-py3.6.egg/flask/app.py", line 2309, in __call__
    return self.wsgi_app(environ, start_response)
  File "/home/dpgaspar/workarea/YYYY/venv3/lib/python3.6/site-packages/Flask-1.0.2-py3.6.egg/flask/app.py", line 2295, in wsgi_app
    response = self.handle_exception(e)
  File "/home/dpgaspar/workarea/YYYY/venv3/lib/python3.6/site-packages/Flask-1.0.2-py3.6.egg/flask/app.py", line 1741, in handle_exception
    reraise(exc_type, exc_value, tb)
  File "/home/dpgaspar/workarea/YYYY/venv3/lib/python3.6/site-packages/Flask-1.0.2-py3.6.egg/flask/_compat.py", line 35, in reraise
    raise value
  File "/home/dpgaspar/workarea/YYYY/venv3/lib/python3.6/site-packages/Flask-1.0.2-py3.6.egg/flask/app.py", line 2292, in wsgi_app
    response = self.full_dispatch_request()
  File "/home/dpgaspar/workarea/YYYY/venv3/lib/python3.6/site-packages/Flask-1.0.2-py3.6.egg/flask/app.py", line 1815, in full_dispatch_request
    rv = self.handle_user_exception(e)
  File "/home/dpgaspar/workarea/YYYY/venv3/lib/python3.6/site-packages/Flask-1.0.2-py3.6.egg/flask/app.py", line 1718, in handle_user_exception
    reraise(exc_type, exc_value, tb)
  File "/home/dpgaspar/workarea/YYYY/venv3/lib/python3.6/site-packages/Flask-1.0.2-py3.6.egg/flask/_compat.py", line 35, in reraise
    raise value
  File "/home/dpgaspar/workarea/YYYY/venv3/lib/python3.6/site-packages/Flask-1.0.2-py3.6.egg/flask/app.py", line 1813, in full_dispatch_request
    rv = self.dispatch_request()
  File "/home/dpgaspar/workarea/YYYY/venv3/lib/python3.6/site-packages/Flask-1.0.2-py3.6.egg/flask/app.py", line 1799, in dispatch_request
    return self.view_functions[rule.endpoint](**req.view_args)
  File "/home/dpgaspar/workarea/YYYY/venv3/lib/python3.6/site-packages/Flask_AppBuilder-1.12.3-py3.6.egg/flask_appbuilder/security/views.py", line 563, in oauth_authorized
    request.args['state'],
  File "/home/dpgaspar/workarea/YYYY/venv3/lib/python3.6/site-packages/Werkzeug-0.14.1-py3.6.egg/werkzeug/datastructures.py", line 431, in __getitem__
    raise exceptions.BadRequestKeyError(key)
werkzeug.exceptions.HTTPException.wrap.<locals>.newcls: 400 Bad Request: KeyError: 'state'

As you can see auth works ok, and self registers the user, but there is no state on the request.args

@betodealmeida
Copy link
Contributor Author

Looks like state is not supported by Twitter (https://twittercommunity.com/t/is-the-state-parameter-supported/1889). The thread suggests using a callback, I'll have to read more about it to get it working.

@betodealmeida
Copy link
Contributor Author

@dpgaspar I fixed the Twitter redirect. It doesn't accept the state payload, but unlike Google/Facebook it allows it in the URL, so all I had to do was change the call to authorize.

I also fixed the Twitter registration: the returned user info has only the username.

@@ -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!!

@dpgaspar dpgaspar merged commit 2ac6a87 into dpgaspar:master Mar 8, 2019
@dpgaspar
Copy link
Owner

dpgaspar commented Mar 8, 2019

Looks good, nice work!

@dpgaspar dpgaspar removed the pending label Mar 9, 2019
@dpgaspar
Copy link
Owner

dpgaspar commented Mar 9, 2019

Just released 1.12.4 with this

@paulvic paulvic mentioned this pull request Aug 27, 2019
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants