PR#289: Migrate authentication to OIDC - fedora-qa/blockerbugs

fedora-qa / blockerbugs

#289 Migrate authentication to OIDC

Opened 3 months ago by abompard. Modified 2 months ago

fedora-qa/ abompard/blockerbugs oidc into develop

Migrate authentication to OIDC

Aurélien Bompard • 2 months ago

a5f78fd

blockerbugs/__init__.py

file modified

+5 -1

		`@@ -6,6 +6,7 @@`

		`from flask import Flask, render_template`
		`from flask_sqlalchemy import SQLAlchemy`
		`+ import flask_oidc`
		`import sqlalchemy`

		`from . import config`
		`@@ -63,7 +64,7 @@`
		`app.config["DEBUG"] = True`

		`# Make sure config URLs end with a slash, so we have them always in the same format`
		`- for key in ['BODHI_URL', 'FAS_BASE_URL', 'PAGURE_URL', 'PAGURE_API']:`
		`+ for key in ['BODHI_URL', 'PAGURE_URL', 'PAGURE_API']:`
		`if not app.config[key].endswith('/'):`
		`app.config[key] = app.config[key] + '/'`

		`@@ -127,6 +128,9 @@`
		`)`
		`db: SQLAlchemy = SQLAlchemy(app=app, metadata=metadata)`

		`+ # Authentication`
		`+ oidc = flask_oidc.OpenIDConnect(app)`
		`+`

		`# === Infra tweaks ===`

blockerbugs/config.py

file modified

+17 -12

		`@@ -35,16 +35,24 @@`
		`BUGZILLA_URL = 'https://bugzilla.stage.redhat.com'`
		`BUGZILLA_API_KEY = ''`
		`BODHI_URL = 'https://bodhi.stg.fedoraproject.org/'`
		`- FAS_ENABLED = False`
		`+ OIDC_ENABLED = False`
		`"""When FAS is not enabled, a fake stub is used instead, which allows the user to log in (under`
		`the credentials+groups defined here in this config) without authentication."""`
		`FAS_ADMIN_GROUP = 'qa-admin'`
		`- FAS_USER = 'developer'`
		`+ OIDC_TESTING_PROFILE = {`
		`+ 'nickname': 'developer',`
		`+ 'groups': [FAS_ADMIN_GROUP],`
		`+ }`
		`"""This is mostly useful for developers, when they want to simulate a login for a certain`
		`user."""`
		`- FAS_HTTPS_REQUIRED = False`
		`- FAS_CHECK_CERT = False`
		`- FAS_BASE_URL = 'https://admin.stg.fedoraproject.org/accounts/'`
		`+ # https://flask-oidc.readthedocs.io/en/latest/#settings-reference`
		`+ OIDC_CLIENT_SECRETS = "conf/oidc.json"`
		`+ OIDC_SCOPES = (`
		`+ 'openid email profile '`
		`+ 'https://id.fedoraproject.org/scope/groups '`
		`+ 'https://id.fedoraproject.org/scope/agreements'`
		`+ )`
		`+ OIDC_USER_INFO_ENABLED = True`
		`LOGFILE = '/var/log/blockerbugs/blockerbugs.log'`
		`FILE_LOGGING = False`
		`SYSLOG_LOGGING = False`
		`@@ -85,13 +93,12 @@`
		`DEBUG = False`
		`BUGZILLA_URL = 'https://bugzilla.redhat.com'`
		`BODHI_URL = 'https://bodhi.fedoraproject.org/'`
		`- FAS_ENABLED = True`
		`- FAS_HTTPS_REQUIRED = True`
		`- FAS_BASE_URL = 'https://admin.fedoraproject.org/accounts/'`
		`+ OIDC_ENABLED = True`
		`PAGURE_URL = "https://pagure.io/"`
		`PAGURE_API = "https://pagure.io/api/0/"`
		`PAGURE_REPO = "fedora-qa/blocker-review"`
		`SHOW_DB_URI = False`
		`+ OIDC_CLIENT_SECRETS = "/etc/blockerbugs/oidc.json"`


		`class DevelopmentConfig(Config):`
		`@@ -127,7 +134,7 @@`
		`additional_env_keys = ["FAS_ADMIN_GROUP", "PAGURE_REPO_TOKEN", "PAGURE_REPO_WEBHOOK_KEY",`
		`"PAGURE_REPO", "PAGURE_BOT_USERNAME", "PAGURE_BOT_ENABLED", "PAGURE_URL", "PAGURE_API",`
		`"BUGZILLA_URL", "BUGZILLA_API_KEY", "BODHI_URL", "BLOCKERBUGS_URL", "BLOCKERBUGS_API",`
		`- "SECRET_KEY", "FAS_BASE_URL"]`
		`+ "SECRET_KEY"]`
		`missing_data = False`

		`for key in additional_env_keys:`
		`@@ -151,7 +158,5 @@`

		`# Adjust testing config to match staging`
		`if openshift_production == "0":`
		`- config_object["FAS_FLASK_COOKIE_REQUIRES_HTTPS"] = False`
		`- config_object["FAS_CHECK_CERT"] = False`
		`config_object["PRODUCTION"] = False`
		`- config_object["FAS_ENABLED"] = True`
		`+ config_object["OIDC_ENABLED"] = True`

blockerbugs/controllers/admin/__init__.py

file modified

+1 -1

		`@@ -29,7 +29,7 @@`
		`from blockerbugs.models.release import Release`
		`from blockerbugs.models.milestone import Milestone`
		`from blockerbugs.controllers.admin.auth import FasAuthModelView`
		`- from blockerbugs.controllers.users import check_admin_rights`
		`+ from blockerbugs.util.login import check_admin_rights`


		`class AdminIndexViewSqla(flask_admin.AdminIndexView):`

blockerbugs/controllers/admin/auth.py

file modified

+1 -1

		`@@ -22,7 +22,7 @@`

		`from flask_admin.contrib import sqla`

		`- from blockerbugs.controllers.users import check_admin_rights`
		`+ from blockerbugs.util.login import check_admin_rights`


		`class FasAuthModelView(sqla.ModelView):`

blockerbugs/controllers/main.py

file modified

+6 -7

		`@@ -23,11 +23,10 @@`
		`from flask import Blueprint, render_template, redirect, url_for, abort, g, make_response, request`
		`import datetime`
		`from sqlalchemy import or_`
		`- from flask_fas_openid import fas_login_required`
		`import json`
		`import itertools`

		`- from blockerbugs import app, db, __version__`
		`+ from blockerbugs import app, db, oidc, __version__`
		`from blockerbugs.util import bz_interface, pagure_bot, misc, discussion_sync, bug_sync`
		`from blockerbugs.models.bug import Bug`
		`from blockerbugs.models.milestone import Milestone`
		`@@ -235,14 +234,14 @@`


		`def user_voted(bug_votes):`
		`- if not g.fas_user:`
		`+ if not g.oidc_user.logged_in:`
		`return False`

		`voters = set()`
		`for tracker_votes in bug_votes.values():`
		`voters.update(itertools.chain(*tracker_votes.values()))`

		`- return g.fas_user.username in voters`
		`+ return g.oidc_user.name in voters`


		`def web_voting_info(bugz, milestone):`
		`@@ -455,7 +454,7 @@`
		`discussion_sync.create_discussions_links(milestone, [misc.bug_from_db(bugid, milestone)])`

		`@main.route('/propose_bug', methods=['GET', 'POST'])`
		`- @fas_login_required`
		`+ @oidc.require_login`
		`def propose_bug():`
		`current_milestone = get_current_milestone()`
		`bugform = BugProposeForm()`
		`@@ -464,7 +463,7 @@`

		`# we have to process the form after setting the milestone default so that it is properly set`
		`# by passing in the request.form, anything submitted as part of a POST will be inserted as form data`
		`- bugform.process(request.form, fas_login=g.fas_user.username)`
		`+ bugform.process(request.form, fas_login=g.oidc_user.name)`

		`if bugform.validate_on_submit():`
		`app.logger.debug('bugid: %i' % bugform.bugid.data)`
		`@@ -505,7 +504,7 @@`
		`bugform.freeze_exception.errors = ['Bug %i is already proposed as a freeze exception' % bugid]`

		`if len(bugform.errors) == 0:`
		`- user = 'Fedora user %s' % g.fas_user.username`
		`+ user = 'Fedora user %s' % g.oidc_user.name`
		`app.logger.info('%s proposing %i as %s for %s' % (`
		`user, bugid, proposal.get_tracker_type(), selected_milestone.name))`
		`try:`

blockerbugs/controllers/users.py

file removed

-62

		`@@ -1,62 +0,0 @@`
		`- # Copyright 2013, Red Hat, Inc`
		`- #`
		`- # This program is free software; you can redistribute it and/or modify`
		`- # it under the terms of the GNU General Public License as published by`
		`- # the Free Software Foundation; either version 2 of the License, or`
		`- # (at your option) any later version.`
		`- #`
		`- # This program is distributed in the hope that it will be useful,`
		`- # but WITHOUT ANY WARRANTY; without even the implied warranty of`
		`- # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the`
		`- # GNU General Public License for more details.`
		`- #`
		`- # You should have received a copy of the GNU General Public License along`
		`- # with this program; if not, write to the Free Software Foundation, Inc.,`
		`- # 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.`
		`- #`
		`- # Authors:`
		`- # Tim Flink <tflink@redhat.com>`
		`- #`
		`- # Liberally borrows from the sample app in Flask-FAS maintained by Ian Weller`
		`-`
		`- """Handles logins and other user interaction"""`
		`-`
		`- from flask import redirect, url_for, request, g, abort`
		`- from flask_fas_openid import fas_login_required`
		`-`
		`- from blockerbugs import app`
		`- import blockerbugs.util.login`
		`-`
		`- fas = blockerbugs.util.login.getFAS()`
		`-`
		`-`
		`- @app.route('/auth_login')`
		`- def auth_login():`
		`- if 'next' in request.args:`
		`- next_url = request.args['next']`
		`- else:`
		`- next_url = url_for('main.index')`
		`-`
		`- groups = []`
		`- groups.append(app.config['FAS_ADMIN_GROUP'])`
		`- return fas.login(return_url=next_url, groups=groups)`
		`-`
		`-`
		`- @app.route('/auth_logout')`
		`- def auth_logout():`
		`- if g.fas_user:`
		`- app.logger.info('Logging out user %s' % g.fas_user.username)`
		`- fas.logout()`
		`- else:`
		`- app.logger.debug('No FAS user to logout')`
		`- return redirect(url_for('main.index'))`
		`-`
		`-`
		`- @fas_login_required`
		`- def check_admin_rights():`
		`- if app.config['FAS_ADMIN_GROUP'] in g.fas_user.groups:`
		`- return None`
		`-`
		`- app.logger.info('Failed admin access to {url} by {user} (not in group "{admin}")'.format(`
		`- url=request.url, user=g.fas_user.username, admin=app.config['FAS_ADMIN_GROUP']))`
		`- abort(401)`

blockerbugs/templates/layout.html

file modified

+3 -3

		`@@ -57,13 +57,13 @@`
		`</div>`
		`<div class="col-lg-2 col-md-4 col-sm-4 col-3 d-flex justify-content-end">`
		`<div class='login-header'>`
		`- {% if g.fas_user %}`
		`+ {% if g.oidc_user.logged_in %}`
		`<span class='login-link'>`
		`- <a href="{{ url_for('auth_logout') }}" class="btn btn-sm">Logout {{ g.fas_user.username }}</a>`
		`+ <a href="{{ url_for('oidc_auth.logout') }}" class="btn btn-sm">Logout {{ g.oidc_user.name }}</a>`
		`</span>`
		`{% else %}`
		`<span class='login-link'>`
		`- <a href="{{ url_for('auth_login', next=request.url) }}" class="btn btn-sm">Login</a>`
		`+ <a href="{{ url_for('oidc_auth.login', next=request.url) }}" class="btn btn-sm">Login</a>`
		`</span>`
		`{% endif %}`
		`</div>`

blockerbugs/util/login.py

file modified

+10 -57

		`@@ -17,65 +17,18 @@`
		`# Authors:`
		`# Martin Krizek <mkrizek@redhat.com>`

		`- """A login wrapper"""`
		`+ """Tools for authentication and authorization."""`

		`- import munch`
		`- from flask import g, request, redirect`
		`- from flask_fas_openid import FAS`
		`+ from flask import g, request, abort`

		`- from blockerbugs import app`
		`+ from blockerbugs import app, oidc`


		`- _fas = None`
		`- """A singleton FAS instance. Use getFAS() to access it."""`
		`+ @oidc.require_login`
		`+ def check_admin_rights():`
		`+ if app.config['FAS_ADMIN_GROUP'] in g.oidc_user.groups:`
		`+ return None`

		`-`
		`- def getFAS():`
		`- """Return either the real or a fake FAS object depending on the app config"""`
		`- global _fas`
		`- if _fas is not None:`
		`- return _fas`
		`-`
		`- if app.config['FAS_ENABLED']:`
		`- _fas = FAS(app)`
		`- else:`
		`- _fas = FakeFAS(app)`
		`-`
		`- return _fas`
		`-`
		`-`
		`- class FakeFAS(object):`
		`- """A FAS stub class. Allows a login without any authentication. It uses`
		`- a username provided in the app config (or 'developer'), and automatically`
		`- puts it into the admin group (as defined in the app config).`
		`- """`
		`- fake_user = munch.Munch(`
		`- username=app.config['FAS_USER'],`
		`- groups=[app.config['FAS_ADMIN_GROUP']])`
		`-`
		`- def __init__(self, app):`
		`- app.before_request(self._set_fas_user)`
		`- self.logged_in = False`
		`-`
		`- def _set_fas_user(self):`
		`- if self.logged_in:`
		`- g.fas_user = self.fake_user`
		`- else:`
		`- g.fas_user = None`
		`-`
		`- def _return(self, args, *kwargs):`
		`- return_url = kwargs.get('return_url')`
		`- if return_url is None:`
		`- if 'next' in request.args.values():`
		`- return_url = request.args.values['next']`
		`- else:`
		`- return_url = request.url`
		`- return redirect(return_url)`
		`-`
		`- def login(self, args, *kwargs):`
		`- self.logged_in = True`
		`- return self._return(args, *kwargs)`
		`-`
		`- def logout(self, args, *kwargs):`
		`- self.logged_in = False`
		`- return self._return(args, *kwargs)`
		`+ app.logger.info('Failed admin access to {url} by {user} (not in group "{admin}")'.format(`
		`+ url=request.url, user=g.oidc_user.name, admin=app.config['FAS_ADMIN_GROUP']))`
		`+ abort(403, "You are not a member of the required group.")`

blockerbugs/util/update_sync.py

file modified

+19 -1

		`@@ -23,7 +23,6 @@`
		`import logging`
		`from typing import Optional, Any, Iterable, Sequence`

		`- from fedora.client import ServerError`
		`from bodhi.client.bindings import BodhiClient`
		`import flask_sqlalchemy`

		`@@ -33,6 +32,25 @@`
		`from blockerbugs.models.release import Release`


		`+ class ServerError(Exception):`
		`+ '''Unable to talk to the server properly.`
		`+`
		`+ This includes network errors and 500 response codes. If the error was`
		+ generated from an http response, :attr:`code` is the HTTP response code.
		+ Otherwise, :attr:`code` will be -1.
		`+ '''`
		`+ def __init__(self, url, status, msg):`
		`+ self.filename = url`
		`+ self.code = status`
		`+ self.msg = msg`
		`+`
		`+ def __str__(self):`
		`+ return 'ServerError(%s, %s, %s)' % (self.filename, self.code, self.msg)`
		`+`
		`+ def __repr__(self):`
		`+ return 'ServerError(%r, %r, %r)' % (self.filename, self.code, self.msg)`
		`+`
		`+`
		`class UpdateSync(object):`
		`"""The main class for perfoming Update synchronization with Bodhi."""`

conf/oidc.json.example

file added

@@ -0,0 +1,1 @@

+ {"web": {"client_id": "your-oidc-client-id", "client_secret": "your-oidc-client-secret", "auth_uri": "https://provider.example.com/Authorization", "token_uri": "https://provider.example.com/Token", "userinfo_uri": "https://provider.example.com/UserInfo", "redirect_uris": ["https://blockerbugs.example.com/authorize"], "issuer": "https://provider.example.com/"}}

conf/settings.py.example

file modified

+1 -1

		`@@ -2,7 +2,7 @@`
		`# cli to generate a config file`
		`SECRET_KEY = 'replace-me-with-something-random'`
		`SQLALCHEMY_DATABASE_URI = 'postgresql+psycopg://dbuser:dbpassword@dbhost:dbport/dbname'`
		`- FAS_ENABLED = False`
		`+ OIDC_ENABLED = False`
		`FAS_ADMIN_GROUP = "magic-fas-group"`
		`BUGZILLA_URL = 'https://bugzilla.stage.redhat.com'`
		`BUGZILLA_API_KEY = '' # INSERT YOUR SECRET API KEY FROM BUGZILLA USER SETTINGS`

requirements.txt

file modified

+1 -1

		`@@ -7,6 +7,7 @@`
		`bodhi-client >= 5.7.5`
		`email_validator`
		`Flask-Admin`
		`+ flask-oidc >= 2.3.1`
		`Flask-SQLAlchemy`
		`Flask-WTF`
		`iso8601`
		`@@ -23,7 +24,6 @@`
		`# bugzilla 3.2.0 because of API auth changes:`
		`# https://listman.redhat.com/archives/bugzilla-announce-list/2022-February/msg00000.html`
		`python-bugzilla >= 3.2.0`
		`- python-fedora`
		`python-openid-cla`
		`python-openid-teams`
		`python3-openid`

setup.cfg

file modified

+1 -2

		`@@ -12,7 +12,7 @@`
		`ignore_missing_imports = True`
		`[mypy-flask_wtf.*]`
		`ignore_missing_imports = True`
		`- [mypy-flask_fas_openid.*]`
		`+ [mypy-flask_oidc.*]`
		`ignore_missing_imports = True`
		`[mypy-flask_admin.*]`
		`ignore_missing_imports = True`
		`@@ -41,7 +41,6 @@`
		`ignore::DeprecationWarning:ast`
		`ignore::DeprecationWarning:dateutil`
		`ignore::DeprecationWarning:flask_admin`
		`- ignore::DeprecationWarning:flask_fas_openid`
		`ignore::DeprecationWarning:flask_sqlalchemy`
		`ignore::DeprecationWarning:openid`
		`ignore::DeprecationWarning:pkg_resources`

setup.py

file modified

+1 -1

		`@@ -45,6 +45,7 @@`
		`'Flask-Admin',`
		`'Flask-SQLAlchemy',`
		`'Flask-WTF',`
		`+ 'flask-oidc',`
		`'Flask',`
		`'iso8601',`
		`'Jinja2',`
		`@@ -54,7 +55,6 @@`
		`# bugzilla 3.2.0 because of API auth changes:`
		`# https://listman.redhat.com/archives/bugzilla-announce-list/2022-February/msg00000.html`
		`'python-bugzilla >= 3.2.0',`
		`- 'python-fedora',`
		`'python-openid-cla',`
		`'python-openid-teams',`
		`'python3-openid',`

testing/conftest.py

file modified

+2 -1

		`@@ -32,7 +32,7 @@`


		`@pytest.fixture`
		`- def app_ctx():`
		`+ def app_ctx(monkeypatch):`
		`"""Run the decorated function/method under the Flask app context (necessary whenever touching`
		`the DB).`
		`Read more at:`
		`@@ -49,5 +49,6 @@`
		`"""`
		`# this import must happen here, because pytest_configure() must run first`
		`from blockerbugs import app`
		`+ monkeypatch.setattr(app, 'secret_key', 'testing-secret')`
		`with app.app_context():`
		`yield`

testing/test_login.py

file removed

-52

		`@@ -1,52 +0,0 @@`
		`- """Test blockerbugs/util/login.py"""`
		`-`
		`- import pytest`
		`- import mock`
		`- import flask_fas_openid`
		`-`
		`- from blockerbugs.util import login`
		`- from blockerbugs import app`
		`-`
		`-`
		`- @pytest.fixture(autouse=True)`
		`- def reset_fas(monkeypatch):`
		`- """Reset login._fas to None for all tests, so that they can test its initialization. Also, it is`
		`- a global variable, we don't want to change the real value, it could affect other test modules.`
		`- """`
		`- monkeypatch.setattr(login, '_fas', None)`
		`-`
		`-`
		`- @pytest.fixture(autouse=True)`
		`- def avoid_flask_setup(monkeypatch):`
		`- """Flask aborts, if a setup method is called after it already handled some requests (which it`
		`- already did in other test modules). We must avoid calling such setup methods.`
		`- """`
		`- monkeypatch.setattr(app, 'before_request', mock.MagicMock())`
		`- monkeypatch.setattr(app, 'add_url_rule', mock.MagicMock())`
		`- monkeypatch.setattr(app, 'route', mock.MagicMock())`
		`-`
		`-`
		`- class TestLogin:`
		`- def test_default_fakefas(self):`
		`- """During a test suite run, FakeFAS should be used by default"""`
		`- assert isinstance(login.getFAS(), login.FakeFAS)`
		`-`
		`- def test_singleton(self):`
		`- """A singleton _fas should not be overwritten with subsequent calls"""`
		`- fas = login.getFAS()`
		`- assert fas is login._fas`
		`- fas2 = login.getFAS()`
		`- assert fas is fas2 is login._fas`
		`-`
		`- def test_config_fas_enabled(self, monkeypatch):`
		`- """Config option FAS_ENABLED should affect the returned object"""`
		`- monkeypatch.setitem(app.config, 'FAS_ENABLED', False)`
		`- assert isinstance(login.getFAS(), login.FakeFAS)`
		`-`
		`- login._fas = None`
		`- monkeypatch.setitem(app.config, 'FAS_ENABLED', '')`
		`- assert isinstance(login.getFAS(), login.FakeFAS)`
		`-`
		`- login._fas = None`
		`- monkeypatch.setitem(app.config, 'FAS_ENABLED', True)`
		`- assert isinstance(login.getFAS(), flask_fas_openid.FAS)`

testing/testfunc_update_sync.py

file modified

+1 -2

		`@@ -5,13 +5,12 @@`
		`import pytest`
		`from munch import Munch`
		`from mock import patch, Mock`
		`- from fedora.client import ServerError`

		`from blockerbugs import db`
		`from blockerbugs.models.milestone import Milestone`
		`from blockerbugs.models.release import Release`
		`from blockerbugs.models.update import Update`
		`- from blockerbugs.util.update_sync import UpdateSync`
		`+ from blockerbugs.util.update_sync import UpdateSync, ServerError`
		`from testing.test_controllers import add_bug`

abompard commented 3 months ago

Authentication now uses Flask-OIDC instead of the deprecated FAS OpenID library.

Fixes: #288

kparal commented 2 months ago

Thanks, @abompard ! This is very appreciated. We're in the middle of a Fedora 42 release, so I won't be able to look at this immediately, but I'll do it as soon as I can. Thanks once again.

Metadata Update from @kparal:
- Request assigned

2 months ago

rebased onto 1b27536

2 months ago

lbrabec commented 2 months ago

It seems to be working!

@kparal, as you'll probably want to try it yourself:

To test this I set up tiny-stage, generated client secrets with command:

oidc-register https://ipsilon.tinystage.test/idp/openidc/ http://localhost:9999/authorize
and copied the generated client_secrets.json to blockerbugs' conf/oidc.json

note: I had to append the contents of tiny-stage's synced_folders/ipa/ipa/ca.crt to certifi's env_blockerbugs/lib/python3.11/site-packages/certifi/cacert.pem to get the oidc-register to work. However, that can be problem with my setup as the tiny-stage is running on computer somewhere else and I'm routing there through VPN.

Finally, I enabled OIDC in blockerbugs by putting OIDC_ENABLED = True in conf/settings.py and run the app as usual: ./run app.

Thanks, @abompard, did I test it right or is there a better way?

abompard commented 2 months ago

Yes, thanks for going through all these hoops! I could provide a Vagrant file and ansible playbooks to automate the integration with tinystage if you want, but since it's not necessary for regular hacking (you just set OIDC_ENABLED to False) and since we want to migrate away from Vagrant, maybe it's not something you'd be interested in.
I used it for testing so it's almost already there, just tell me if you're interested :-)