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

Auth features for server #9190

Merged
merged 36 commits into from Sep 11, 2019

Conversation

@bryevdv
Copy link
Member

commented Aug 25, 2019

  • issues: fixes #9140
  • tests added / passed
  • release document entry (if new feature or API change)

This is WIP, would like to get some early feed back on how the usage feels.

You can now supply an auth modue, e.g.


import tornado
from tornado.web import RequestHandler

# could also define async_get_user
def get_user(request_handler):
    return request_handler.get_secure_cookie("user")

# could also define get_login_url func (but give up LoginHandler)
login_url = "/login"

# optional login page at login_url
class LoginHandler(RequestHandler):

    def get(self):
        try:
            errormessage = self.get_argument("error")
        except:
            errormessage = ""
        self.render("login.html", errormessage=errormessage)

    def check_permission(self, password, username):
        if username == "bokeh" and password == "bokeh":
            return True
        return False

    def post(self):
        username = self.get_argument("username", "")
        password = self.get_argument("password", "")
        auth = self.check_permission(password, username)
        if auth:
            self.set_current_user(username)
            self.redirect(self.get_argument("next", u"/"))
        else:
            error_msg = u"?error=" + tornado.escape.url_escape("Login incorrect")
            self.redirect(login_url + error_msg)

    def set_current_user(self, user):
        if user:
            self.set_secure_cookie("user", tornado.escape.json_encode(user))
        else:
            self.clear_cookie("user")

and a login.html template, and you can only get to the app if you pass login.

Some notes:

  • always have to supply --auth-module separately, i.e. can't make auth.py in a directoy app work because there could be multiple directory apps and that makes no sense (also other reasons)
  • LoginHandler is optional, "serious" deploys would redirect to some external login probably
  • Since any cookie get/set happens in the auth module, users can control max age and expiry there themselves when they call those funcs
  • Do we need to support a LogoutHandler? Presumably apps could clear the cookie themselved from a Button push, etc. Maybe later?

Initial thoughts, @bokeh/dev ?

@bryevdv bryevdv added the status: WIP label Aug 25, 2019

@bryevdv bryevdv force-pushed the bryanv/9140_tornado_auth branch from 8d3a2f9 to e240b82 Aug 25, 2019

@birdsarah

This comment has been minimized.

Copy link
Member

commented Aug 25, 2019

Sorry I'm being slow, can you spell out the rest of the incantation that would mean this is all hooked up?

@bryevdv

This comment has been minimized.

Copy link
Member Author

commented Aug 25, 2019

Sorry, run like this:

BOKEH_COOKIE_SECRET='my super secret' bokeh serve --auth-module=/Users/bryan/tmp/auth.py --show sliders.py

I used this template:

<div id="main-container">
    <div id="main">
        <div id="login-form">
            <form action="/login" method="post" id="login_form">
                <fieldset>
                    <label for="username">Username</label>
                    <input autocapitalize="off" autocorrect="off" class="text-input" id="username" name="username" tabindex="1" type="text" value="">
                </fieldset>

                <fieldset>
                    <label for="password">Password</label>
                    <input class="text-input" id="password" name="password" tabindex="2" type="password" value="">
                </fieldset>
                <fieldset>
                    <span class="errormessage">{{errormessage}}</span>
                </fieldset>

                <div id="form_btn">
                    <input id="signin-btn" class="btn btn-blue" type="submit" value="Sign In" tabindex="3">
                </div>
            </form>
        </div>
    </div>
</div>
bokeh/server/tornado.py Outdated Show resolved Hide resolved
@birdsarah

This comment has been minimized.

Copy link
Member

commented Aug 29, 2019

This looms great. Just some clear docs like the other stuff you've done for server. The only bit that I'm left wondering is which class is bokeh looking for - anything that overrides the requesthandler? Or is it the loginhandler class specifically which needs to override the requesthandler.

The enterprise I imagine is the bokeh server being behind the corporate ldap/oauth where you go to the bokeh page, get redirected to oauth, and then get let in if valid, in that case no template would be required its just routing and checking.

The other scenario I imagine is where your bokeh app is embedded in site that has login. In that case the bokeh plot would be an embedded frame that would gate until author handling / checking was complete.

In both of these scenarios the login template itself wouldn't be used. But I think its great to include your scenario too wherethe only server is bokeh / tornado and it serves up the login.

@birdsarah

This comment has been minimized.

Copy link
Member

commented Aug 29, 2019

Maybe this leads to a more general 'hookig into tornado', I'm not sure.

@bryevdv

This comment has been minimized.

Copy link
Member Author

commented Sep 2, 2019

@p-himik I believe I have addressed your change in 851f00c

@birdsarah @hhuuggoo I think something needs to be done re: logging out. I am just not sure what exactly the best workflow is. I think it would be simple to add a logout_url and optional LogoutHandler analagous to the login case. That handler could clear cookies or whatever, etc. Then a js_on_click or session timeout callback could redirect to that URL. Does that sound reasonable? (With this basic setup, it's incumbent on the app writer to do the right page redirect, I suppose the question is whether we need more fully developed API do do this automatically based on the configuration in the auth module)

Also, tried to make a slightly prettier example login:

Screen Shot 2019-09-02 at 1 52 58 PM

@birdsarah

This comment has been minimized.

Copy link
Member

commented Sep 2, 2019

I think something needs to be done re: logging out. I am just not sure what exactly the best workflow is. I think it would be simple to add a logout_url and optional LogoutHandler analagous to the login case. That handler could clear cookies or whatever, etc.

Good point. Yes. I don't remember ever worrying about logging people out specifically. Typically you just set the cookie expiry to be what you want. I don't know what would happen to bokeh once the cookie expired. Regardless the analagous logout hook would be great, and presumably it's easy.

@p-himik
p-himik approved these changes Sep 3, 2019
@bryevdv

This comment has been minimized.

Copy link
Member Author

commented Sep 7, 2019

Ok I added the logout url/handler support:

ScreenFlow

Imagine if we had added this three years ago 🤦‍♂

I've been in a bit of a slump the last two weeks but I am going to do my best to get these things done this weekend:

  • Factor all the auth things from the auth module into an AuthProvider class. There are lots of bits and they are all currently smeared all over the tornado handler
  • Add tests (first task will help with this)
  • User guide docs

Hoping to get some final thoughts/review and merge early next week.

@bryevdv bryevdv force-pushed the bryanv/9140_tornado_auth branch from c61175d to 4b7d881 Sep 7, 2019

@bryevdv bryevdv force-pushed the bryanv/9140_tornado_auth branch from 5c85db1 to f7d705b Sep 8, 2019

@bryevdv bryevdv force-pushed the bryanv/9140_tornado_auth branch from f7d705b to 720b49e Sep 8, 2019

@bryevdv

This comment has been minimized.

Copy link
Member Author

commented Sep 8, 2019

OK one more small feature, --enable-xsrf-cookies flag to turn to Tornado's XSRF protection.

bokeh/settings.py Outdated Show resolved Hide resolved
@philippjfr

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2019

The code, API and documentation all look good to me so I've simply reviewed the PR for typos.

@jbednar
Copy link
Contributor

left a comment

Looks great. Also only typo fixes.

bokeh/command/subcommands/serve.py Outdated Show resolved Hide resolved
authentication will be required to access Bokeh server endpoints.
.. warning::
The contents of the auth module will be executed!

This comment has been minimized.

Copy link
@jbednar

jbednar Sep 10, 2019

Contributor

Can you explain the implications here, i.e. why that's a warning? Maybe:

Suggested change
The contents of the auth module will be executed!
The contents of the auth module will be executed, so you should ensure that this module is trustworthy and only includes code you are comfortable with.
bokeh/command/subcommands/serve.py Outdated Show resolved Hide resolved
bokeh/command/subcommands/tests/test_serve.py Outdated Show resolved Hide resolved
the Bokeh server.
.. warning::
The contents of this module will be executed!

This comment has been minimized.

Copy link
@jbednar

jbednar Sep 10, 2019

Contributor

(Same as above comment)

sphinx/source/docs/user_guide/server.rst Outdated Show resolved Hide resolved
sphinx/source/docs/user_guide/server.rst Outdated Show resolved Hide resolved
sphinx/source/docs/user_guide/server.rst Outdated Show resolved Hide resolved
sphinx/source/docs/user_guide/server.rst Outdated Show resolved Hide resolved
bryevdv and others added 16 commits Sep 11, 2019
Update sphinx/source/docs/user_guide/server.rst
Co-Authored-By: James A. Bednar <jbednar@users.noreply.github.com>
Update sphinx/source/docs/user_guide/server.rst
Co-Authored-By: James A. Bednar <jbednar@users.noreply.github.com>
Update sphinx/source/docs/user_guide/server.rst
Co-Authored-By: James A. Bednar <jbednar@users.noreply.github.com>
Update sphinx/source/docs/user_guide/server.rst
Co-Authored-By: James A. Bednar <jbednar@users.noreply.github.com>
Update bokeh/command/subcommands/tests/test_serve.py
Co-Authored-By: James A. Bednar <jbednar@users.noreply.github.com>
Update bokeh/command/subcommands/serve.py
Co-Authored-By: James A. Bednar <jbednar@users.noreply.github.com>
Update bokeh/command/subcommands/serve.py
Co-Authored-By: James A. Bednar <jbednar@users.noreply.github.com>
Update bokeh/server/auth_provider.py
Co-Authored-By: Philipp Rudiger <prudiger@anaconda.com>
Update bokeh/server/auth_provider.py
Co-Authored-By: Philipp Rudiger <prudiger@anaconda.com>
Update bokeh/server/auth_provider.py
Co-Authored-By: Philipp Rudiger <prudiger@anaconda.com>
Update bokeh/command/subcommands/serve.py
Co-Authored-By: Philipp Rudiger <prudiger@anaconda.com>
Update bokeh/command/subcommands/serve.py
Co-Authored-By: Philipp Rudiger <prudiger@anaconda.com>
Update bokeh/settings.py
Co-Authored-By: Philipp Rudiger <prudiger@anaconda.com>
Update sphinx/source/docs/user_guide/server.rst
Co-Authored-By: Philipp Rudiger <prudiger@anaconda.com>
Update sphinx/source/docs/user_guide/server.rst
Co-Authored-By: Philipp Rudiger <prudiger@anaconda.com>
Update sphinx/source/docs/user_guide/server.rst
Co-Authored-By: Philipp Rudiger <prudiger@anaconda.com>
@bryevdv

This comment has been minimized.

Copy link
Member Author

commented Sep 11, 2019

Ok I am going to go ahead and merge this. Release is still at least a few weeks out so there is certainly time for any follow on work.

@bryevdv bryevdv merged commit fb762f2 into master Sep 11, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.