Skip to content
This repository has been archived by the owner on Jul 22, 2019. It is now read-only.

Implement OpenID Connect authentication #48

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 45 additions & 0 deletions app/api/authentication.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,18 @@
from flask_login import login_user as login
from flask_login import logout_user as logout

import config
import constants.api
import database.user
from api.decorators import require_form_args
from api.decorators import require_local_auth
from modern_paste import app
from uri.authentication import *
from util.exception import *


@app.route(LoginUserURI.path, methods=['POST'])
@require_local_auth
@require_form_args(['username', 'password'])
def login_user():
"""
Expand Down Expand Up @@ -70,3 +73,45 @@ def auth_status():
'user_id': getattr(current_user, 'user_id', None),
},
}), constants.api.SUCCESS_CODE


def oidc_request_loader():
Copy link
Owner

Choose a reason for hiding this comment

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

This function is not an API handler and doesn't belong in this file. Maybe create a new file app/util/oidc.py and move this there?

"""
flask_login request loader for OpenID Connect.
"""
if current_user.is_authenticated:
return

from modern_paste import oidc
Copy link
Owner

Choose a reason for hiding this comment

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

Move this import to the top of the file?

info = None
if flask.g.oidc_id_token is not None:
# Auth succeeded with flask-oidc OIDC client flow
username = oidc.user_getfield('sub')
try:
login(database.user.get_user_by_username(username))
return
except UserDoesNotExistException:
info = oidc.user_getinfo(['sub', 'name', 'email'])

elif flask.request.headers.get('Authorization', '').startswith('Bearer '):
# An OAuth2 Bearer Token was sent
token = flask.request.headers['Authorization'].split(' ', 1)[1].strip()
valid = oidc.validate_token(token, config.AUTH_OIDC_SCOPE)
if valid is True:
Copy link
Owner

Choose a reason for hiding this comment

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

if valid:

username = flask.g.oidc_token_info['sub']
try:
login(database.user.get_user_by_username(username))
except UserDoesNotExistException:
info = oidc.user_getinfo(['sub', 'name', 'email'], token)
else:
return flask.jsonify(constants.api.AUTH_FAILURE), constants.api.AUTH_FAILURE_CODE
Copy link
Owner

Choose a reason for hiding this comment

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

This function isn't an API handler, what is the meaning of returning an API response here?


if info is not None:
new_user = database.user.create_new_user(
username=info['sub'],
password=None,
signup_ip=flask.request.remote_addr,
name=info.get('name'),
email=info.get('email')
)
login(new_user)
25 changes: 20 additions & 5 deletions app/api/decorators.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,10 @@ def view_function():
"""
@wraps(func)
def decorated_view(*args, **kwargs):
template, context = func(*args, **kwargs)
response = func(*args, **kwargs)
Copy link
Owner

Choose a reason for hiding this comment

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

This change seems to be unrelated to this PR. What is the motivation? @view_function is a deliberate abstraction requiring view functions to always return a tuple of (template name, context object) and this logic overrides that purpose.

if not isinstance(response, tuple):
return response
template, context = response
if 'config' in context:
context['config'].update(context_config())
else:
Expand Down Expand Up @@ -80,7 +83,7 @@ def require_login_api(func):
"""
A custom implementation of Flask-login's built-in @login_required decorator.
This decorator will allow usage of the API endpoint if the user is either currently logged in via the app
or if the user authenticates with an API key in the POST JSON parameters.
or if the user authenticates with an API key in the POST JSON parameters and local auth is used.
This implementation overrides the behavior taken when the current user is not authenticated by
returning the predefined AUTH_FAILURE JSON response with HTTP status code 401.
This decorator is intended for use with API endpoints.
Expand All @@ -91,7 +94,7 @@ def decorator(*args, **kwargs):
if current_user.is_authenticated:
return func(*args, **kwargs)
try:
if data and data.get('api_key'):
if config.AUTH_METHOD == 'local' and data and data.get('api_key'):
Copy link
Owner

Choose a reason for hiding this comment

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

Please add 'local' and other AUTH_METHOD constants to a new file in app/constants and import the string value from there

user = database.user.get_user_by_api_key(data['api_key'], active_only=True)
login_user(user)
del data['api_key']
Expand All @@ -107,7 +110,7 @@ def optional_login_api(func):
"""
This decorator is similar in behavior to require_login_api, but is intended for use with endpoints that offer
extended functionality with a login, but can still be used without any authentication.
The decorator will set current_user if authentication via an API key is provided, and will continue without error
The decorator will set current_user if authentication via an API key is provided and local auth is used, and will continue without error
otherwise.
This decorator is intended for use with API endpoints.
"""
Expand All @@ -117,7 +120,7 @@ def decorator(*args, **kwargs):
if current_user.is_authenticated:
return func(*args, **kwargs)
try:
if data and data.get('api_key'):
if config.AUTH_METHOD == 'local' and data and data.get('api_key'):
user = database.user.get_user_by_api_key(data['api_key'], active_only=True)
login_user(user)
del data['api_key']
Expand Down Expand Up @@ -160,3 +163,15 @@ def decorated_view(*args, **kwargs):
return func(*args, **kwargs)
return decorated_view
return decorator


def require_local_auth(func):
"""
Makes the current API endpoint only work with local auth, otherwise returns an error.
"""
@wraps(func)
def decorator(*args, **kwargs):
if config.AUTH_METHOD != 'local':
return jsonify(AUTH_METHOD_DISABLED_FAILURE), AUTH_METHOD_DISABLED_FAILURE_CODE
return func(*args, **kwargs)
return decorator
7 changes: 7 additions & 0 deletions app/api/user.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,12 @@
from flask_login import login_user
from api.decorators import require_form_args
from api.decorators import require_login_api
from api.decorators import require_local_auth
from util.exception import *


@app.route(UserCreateURI.path, methods=['POST'])
@require_local_auth
@require_form_args(['username', 'password'])
def create_new_user():
"""
Expand Down Expand Up @@ -59,6 +61,7 @@ def create_new_user():


@app.route(UserUpdateDetailsURI.path, methods=['POST'])
@require_local_auth
@require_login_api
def update_user_details():
"""
Expand Down Expand Up @@ -100,6 +103,7 @@ def update_user_details():


@app.route(UserDeactivateURI.path, methods=['POST'])
@require_local_auth
@require_login_api
def deactivate_user():
"""
Expand All @@ -117,6 +121,7 @@ def deactivate_user():


@app.route(UserAPIKeyRegenerateURI.path, methods=['POST'])
@require_local_auth
@require_login_api
def api_key_regenerate():
"""
Expand All @@ -133,6 +138,7 @@ def api_key_regenerate():


@app.route(CheckUsernameAvailabilityURI.path, methods=['POST'])
@require_local_auth
@require_form_args(['username'])
def check_username_availability():
"""
Expand All @@ -149,6 +155,7 @@ def check_username_availability():


@app.route(ValidateEmailAddressURI.path, methods=['POST'])
@require_local_auth
@require_form_args(['email'])
def validate_email_address():
"""
Expand Down
11 changes: 11 additions & 0 deletions app/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,17 @@
# This is useful for private or internal installations that aren't intended for public use.
REQUIRE_LOGIN_TO_PASTE = False

# Authentication method
# This selects between either local users or oidc (OpenID Connect)
AUTH_METHOD = 'local'

# OpenID Connect client secrets file
AUTH_OIDC_CLIENT_SECRETS = 'client_secrets.json'


# Required scope for OAuth2 API calls
AUTH_OIDC_SCOPE = 'modernpaste'

# AES key for generating encrypted IDs
# This is only relevant if USE_ENCRYPTED_IDS above is True. If not, this config parameter can be ignored.
# It is recommended, but not strictly required, for you to replace the string below with the output of os.urandom(32),
Expand Down
7 changes: 7 additions & 0 deletions app/constants/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,13 @@
}
USER_REGISTRATION_DISABLED_FAILURE_CODE = 403

AUTH_METHOD_DISABLED_FAILURE = {
RESULT: RESULT_FAULURE,
MESSAGE: 'The auth method you attempted to use is disabled on this server.',
FAILURE: 'auth_method_disabled_failure'
Copy link
Owner

Choose a reason for hiding this comment

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

Nit: trailing comma

}
AUTH_METHOD_DISABLED_FAILURE_CODE = 403

PASTE_ATTACHMENTS_DISABLED_FAILURE = {
RESULT: RESULT_FAULURE,
MESSAGE: 'The server administrator has disabled paste attachments.',
Expand Down
12 changes: 11 additions & 1 deletion app/modern_paste.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

app = Flask(__name__)
app.config.from_object('flask_config')

db = flask_sqlalchemy.SQLAlchemy(app, session_options={
'expire_on_commit': False,
})
Expand All @@ -12,10 +13,19 @@
login_manager = flask_login.LoginManager()
login_manager.init_app(app)


import models
from views import *

def setup_oidc():
from flask_oidc import OpenIDConnect
Copy link
Owner

Choose a reason for hiding this comment

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

Move these imports to the top of the file?

from api.authentication import oidc_request_loader
app.config['OIDC_CLIENT_SECRETS'] = config.AUTH_OIDC_CLIENT_SECRETS
oidc = OpenIDConnect(app)
app.before_request(oidc_request_loader)

if config.AUTH_METHOD == 'oidc':
Copy link
Owner

Choose a reason for hiding this comment

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

Ditto here about the 'oidc' constant

setup_oidc()


if __name__ == '__main__':
app.run(host='0.0.0.0', debug=config.BUILD_ENVIRONMENT == constants.build_environment.DEV)
8 changes: 5 additions & 3 deletions app/templates/base.html
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,11 @@
<p class="menu-panel-item dark-link-alt sans-serif regular white size-3 spaced">
<a href="{{ uri('user', 'UserLoginInterfaceURI') }}">LOGIN</a>
</p>
<p class="menu-panel-item dark-link-alt sans-serif regular white size-3 spaced">
<a href="{{ uri('user', 'UserRegisterInterfaceURI') }}">REGISTER</a>
</p>
{% if enable_user_registration %}
<p class="menu-panel-item dark-link-alt sans-serif regular white size-3 spaced">
<a href="{{ uri('user', 'UserRegisterInterfaceURI') }}">REGISTER</a>
</p>
{% endif %}
{% endif %}
<p class="menu-panel-item dark-link-alt sans-serif regular white size-3 spaced">
<a href="{{ uri('paste', 'PasteArchiveInterfaceURI') }}">ARCHIVE</a>
Expand Down
4 changes: 4 additions & 0 deletions app/templates/misc/api_documentation.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,10 @@
"failure_name": "auth_failure",
"description": "This error is returned when attempting to access an authentication-required endpoint without authentication, or when attempting to access an authentication-optional endpoint with an invalid API key."
},
{
"failure_name": "auth_method_disabled_failure",
"description": "This error is returned when attempting to access an API endpoint that's only used when local users are used, but this installation uses an external authentication mechanism."
},
{
"failure_name": "incomplete_params_failure",
"description": "At least one required request parameter for the endpoint is missing from the request. Even if all required parameters are supplied as keys, this error may still be returned if the value of one of the required parameters is blank (e.g., an empty string). This error may also be returned if the <span class=\"ubuntu-mono regular\">Content-Type</span> of the request isn't explicitly specified as <span class=\"ubuntu-mono regular\">application/json</span>."
Expand Down
14 changes: 10 additions & 4 deletions app/templates/user/account.html
Original file line number Diff line number Diff line change
Expand Up @@ -34,15 +34,19 @@
</p>
<a href="#" class="pastes-list-group-item settings-item list-group-item sans-serif semibold active" data-section="pastes-settings">MY PASTES</a>
<a href="#" class="user-profile-list-group-item settings-item list-group-item" data-section="user-profile-settings">USER PROFILE</a>
<a href="#" class="api-key-list-group-item settings-item list-group-item" data-section="api-key-settings">API KEY</a>
<a href="#" class="deactivate-list-group-item settings-item list-group-item" data-section="deactivate-settings">DEACTIVATE</a>
{% if auth_method == 'local' %}
<a href="#" class="api-key-list-group-item settings-item list-group-item" data-section="api-key-settings">API KEY</a>
<a href="#" class="deactivate-list-group-item settings-item list-group-item" data-section="deactivate-settings">DEACTIVATE</a>
{% endif %}
</div>

<div class="mobile-account-settings-list-group btn-group sans-serif regular gray less-spaced size-2" role="group">
<button type="button" class="pastes-list-group-item settings-item btn btn-default sans-serif semibold active" data-section="pastes-settings">MY PASTES</button>
<button type="button" class="user-profile-list-group-item settings-item btn btn-default" data-section="user-profile-settings">USER PROFILE</button>
<button type="button" class="api-key-list-group-item settings-item btn btn-default" data-section="api-key-settings">API KEY</button>
<button type="button" class="deactivate-list-group-item settings-item btn btn-default" data-section="deactivate-settings">DEACTIVATE</button>
{% if auth_method == 'local' %}
<button type="button" class="api-key-list-group-item settings-item btn btn-default" data-section="api-key-settings">API KEY</button>
<button type="button" class="deactivate-list-group-item settings-item btn btn-default" data-section="deactivate-settings">DEACTIVATE</button>
{% endif %}
</div>

<!--My pastes-->
Expand Down Expand Up @@ -113,6 +117,7 @@
</div>
</div>

{% if auth_method == 'local' %}
Copy link
Owner

Choose a reason for hiding this comment

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

Indent the following HTML block?

<!--API key-->
<div class="api-key-settings settings-section-container hidden-setting">
<div class="settings-header">
Expand Down Expand Up @@ -164,6 +169,7 @@
<button type="button" class="deactivate-account-button btn btn-danger sans-serif semibold white size-1 less-spaced" autocomplete="off">DEACTIVATE MY ACCOUNT</button>
</div>
</div>
{% endif %}
</div>

<!--Modal for paste deactivation confirmation-->
Expand Down
2 changes: 2 additions & 0 deletions app/util/cryptography.py
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,8 @@ def secure_hash(s, iterations=10000):
:param iterations: Number of hash iterations to use
:return: A string representing a secure hash of the string
"""
if s is None:
return None
hash_result = SHA256.new(data=str(s)).hexdigest()
for i in range(iterations):
hash_result = SHA256.new(data=hash_result).hexdigest()
Expand Down
14 changes: 14 additions & 0 deletions app/util/templating.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,20 @@
from modern_paste import app


@app.context_processor
def add_config():
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not sure if this is necessary; there is already an abstraction in place to provide the config to the template during server-side rendering in api.decorators.context_config()

"""
Templating utility to retrieve specific configuration information.

Currently injected:

enabled_user_registration
auth_method
"""
return dict(auth_method=config.AUTH_METHOD,
enable_user_registration=config.ENABLE_USER_REGISTRATION)


@app.context_processor
def import_static_resources():
"""
Expand Down
Loading