Replace query to FAS with OIDC groups scope check.

This removes our query to FAS and fixes #304.

It is more flexible too, where we can now configure production to only
allow in members of the `modularity-wg` group, and then later open it up
to all packagers after F26 is out (as was agreed with FESCo).

In the process of working on this, I discovered that #305 is not
necessary.  We don't need our own scope; we can just use the `groups`
scope as done here.
This commit is contained in:
Ralph Bean
2017-02-10 15:50:41 -05:00
parent d093c5eef3
commit 88aca055ce
10 changed files with 120 additions and 181 deletions

1
.gitignore vendored
View File

@@ -11,3 +11,4 @@ test_module_build_service.db
.mbs_local_build.db
*.src.rpm
.test_module_build_service.db
.eggs

View File

@@ -7,6 +7,7 @@
"http://localhost:13747/"
],
"token_uri": "https://id.stg.fedoraproject.org/openidc/Token",
"token_introspection_uri": "https://id.stg.fedoraproject.org/openidc/TokenInfo"
"token_introspection_uri": "https://id.stg.fedoraproject.org/openidc/TokenInfo",
"userinfo_uri": "https://id.stg.fedoraproject.org/openidc/UserInfo"
}
}

View File

@@ -57,8 +57,10 @@ class BaseConfiguration(object):
PKGDB_API_URL = 'https://admin.stg.fedoraproject.org/pkgdb/api'
FAS_URL = 'https://admin.stg.fedoraproject.org/accounts'
REQUIRE_PACKAGER = True
ALLOWED_GROUPS = set([
'packager',
#'modularity-wg',
])
# Available backends are: console, file, journal.
LOG_BACKEND = 'journal'
@@ -109,14 +111,6 @@ class DevConfiguration(BaseConfiguration):
# user. See: https://infrastructure.fedoraproject.org/cgit/ansible.git/commit/?id=a28a93dad75248c30c1792ec35f588c8e317c067
KOJI_PROXYUSER = False
REQUIRE_PACKAGER = False
# You only need these FAS options if you turn on authorization
# with REQUIRE_PACKAGER=True
# FAS_USERNAME = 'put your fas username here'
# FAS_PASSWORD = 'put your fas password here....'
# FAS_PASSWORD = os.environ('FAS_PASSWORD') # you could store it here
# FAS_PASSWORD = commands.getoutput('pass your_fas_password').strip()
KOJI_CONFIG = path.join(confdir, 'koji.conf')
KOJI_PROFILE = 'staging'
KOJI_ARCHES = ['x86_64']
@@ -148,5 +142,4 @@ class TestConfiguration(BaseConfiguration):
class ProdConfiguration(BaseConfiguration):
FAS_USERNAME = 'TODO'
# FAS_PASSWORD = 'another password'
pass

View File

@@ -3,6 +3,11 @@ import socket
import os
import sys
try:
from urllib.parse import urlencode # py3
except ImportError:
from urllib import urlencode # py2
def listen_for_token():
"""
Listens on port 13747 on localhost for a redirect request by OIDC
@@ -65,7 +70,21 @@ print "Usage: submit_build.py [mbs_host] [oidc_token]"
print ""
if not token:
print "Provide token as command line argument or visit following URL to obtain the token:"
print "https://id.stg.fedoraproject.org/openidc/Authorization?response_type=token&response_mode=form_post&nonce=1234&scope=openid%20profile%20email&client_id=mbs-authorizer&state=af0ifjsldkj&redirect_uri=http://localhost:13747/"
query = urlencode({
'response_type': 'token',
'response_mode': 'form_post',
'nonce': '1234',
'scope': ' '.join([
'openid',
'profile',
'email',
'https://id.fedoraproject.org/scope/groups',
]),
'client_id': 'mbs-authorizer',
'state': 'blahblahblah',
}) + "&redirect_uri=http://localhost:13747/"
print "https://id.stg.fedoraproject.org/openidc/Authorization?" + query
print "We are waiting for you to finish the token generation..."
if not token:

View File

@@ -23,16 +23,14 @@
"""Auth system based on the client certificate and FAS account"""
from werkzeug.serving import WSGIRequestHandler
from module_build_service.errors import Unauthorized
from module_build_service import app, log
import fedora.client
import httplib2
import json
from six.moves.urllib.parse import urlencode
def _json_loads(content):
if not isinstance(content, str):
content = content.decode('utf-8')
@@ -72,9 +70,26 @@ def get_token_info(token):
return _json_loads(content)
def get_username(request):
def get_user_info(token):
"""
Returns the client's username based on the OIDC token provided.
Asks the userinfo_uri for more information on a user.
"""
if not client_secrets:
return None
headers = {'authorization': 'Bearer ' + token}
resp, content = httplib2.Http().request(
client_secrets['userinfo_uri'], 'GET',
headers=headers)
return _json_loads(content)
def get_user(request):
"""
Returns the client's username and groups based on the OIDC token provided.
"""
_load_secrets()
@@ -87,33 +102,19 @@ def get_username(request):
try:
data = get_token_info(token)
except Exception as e:
raise Unauthorized("Cannot verify OIDC token: %s" % str(e))
error = "Cannot verify OIDC token: %s" % str(e)
log.exception(error)
raise Unauthorized(error)
if not "active" in data or not data["active"]:
raise Unauthorized("OIDC token invalid or expired.")
#TODO: Once we will get our own scope registered in Fedora infra,
# we can start checking it here.
return data["username"]
try:
extended_data = get_user_info(token)
except Exception as e:
error = "Cannot verify determine user groups: %s" % str(e)
log.exception(error)
raise Unauthorized(error)
def assert_is_packager(username, fas_kwargs):
""" Assert that a user is a packager by consulting FAS.
When user is not a packager (is not in the packager FAS group), an
exception is raised.
Note that `fas_kwargs` must contain values for `base_url`, `username`, and
`password`. These are required arguments for authenticating with FAS.
(Rida needs its own service account/password to talk to FAS).
"""
FAS = fedora.client.AccountSystem(**fas_kwargs)
person = FAS.person_by_username(username)
# Check that they have even applied in the first place...
if not 'packager' in person['group_roles']:
raise Unauthorized("%s is not in the packager group" % username)
# Check more closely to make sure they're approved.
if person['group_roles']['packager']['role_status'] != 'approved':
raise Unauthorized("%s is not approved in the packager group" % username)
groups = set(extended_data['groups'])
return data["username"], groups

View File

@@ -196,22 +196,10 @@ class Config(object):
'type': str,
'default': '',
'desc': ''},
'fas_url': {
'type': str,
'default': '',
'desc': 'FAS URL'},
'fas_username': {
'type': str,
'default': '',
'desc': 'FAS username'},
'fas_password': {
'type': str,
'default': '',
'desc': 'FAS password'},
'require_packager': {
'type': bool,
'default': True,
'desc': 'Turn on authorization against FAS'},
'allowed_groups': {
'type': set,
'default': set(['packager']),
'desc': 'The set of groups allowed to submit builds.'},
'log_backend': {
'type': str,
'default': None,
@@ -327,7 +315,7 @@ class Config(object):
if key in self._defaults:
# type conversion for configuration item
convert = self._defaults[key]['type']
if convert in [bool, int, list, str]:
if convert in [bool, int, list, str, set]:
try:
setattr(self, key, convert(value))
except:

View File

@@ -91,13 +91,11 @@ class ModuleBuildAPI(MethodView):
raise NotFound('No such module found.')
def post(self):
username = module_build_service.auth.get_username(request)
username, groups = module_build_service.auth.get_user(request)
if conf.require_packager:
module_build_service.auth.assert_is_packager(username, fas_kwargs=dict(
base_url=conf.fas_url,
username=conf.fas_username,
password=conf.fas_password))
if conf.allowed_groups and not (conf.allowed_groups & groups):
raise Unauthorized("%s is not in any of %r, only %r" % (
username, conf.allowed_groups, groups))
try:
r = json.loads(request.get_data().decode("utf-8"))
@@ -126,17 +124,11 @@ class ModuleBuildAPI(MethodView):
return jsonify(module.json()), 201
def patch(self, id):
username = module_build_service.auth.get_username(request)
username, groups = module_build_service.auth.get_user(request)
if conf.require_packager:
module_build_service.auth.assert_is_packager(
username,
fas_kwargs=dict(
base_url=conf.fas_url,
username=conf.fas_username,
password=conf.fas_password
)
)
if conf.allowed_groups and not (conf.allowed_groups & groups):
raise Unauthorized("%s is not in any of %r, only %r" % (
username, conf.allowed_groups, groups))
module = models.ModuleBuild.query.filter_by(id=id).first()
if not module:

View File

@@ -33,25 +33,25 @@ import module_build_service.errors
class TestAuthModule(unittest.TestCase):
@raises(module_build_service.errors.Unauthorized)
def test_get_username_no_token(self):
def test_get_user_no_token(self):
request = mock.MagicMock()
request.cookies.return_value = {}
module_build_service.auth.get_username(request)
module_build_service.auth.get_user(request)
@raises(module_build_service.errors.Unauthorized)
@patch('module_build_service.auth.get_token_info')
def test_get_username_failure(self, get_token_info):
def test_get_user_failure(self, get_token_info):
def mocked_get_token_info(token):
return {"active": False}
get_token_info.return_value = mocked_get_token_info
request = mock.MagicMock()
request.cookies.return_value = {"oidc_token", "1234"}
module_build_service.auth.get_username(request)
module_build_service.auth.get_user(request)
@raises(module_build_service.errors.Unauthorized)
@patch('module_build_service.auth.get_token_info')
def test_get_username_good(self, get_token_info):
def test_get_user_good(self, get_token_info):
# https://www.youtube.com/watch?v=G-LtddOgUCE
name = "Joey Jo Jo Junior Shabadoo"
def mocked_get_token_info(token):
@@ -60,34 +60,5 @@ class TestAuthModule(unittest.TestCase):
request = mock.MagicMock()
request.cookies.return_value = {"oidc_token", "1234"}
result = module_build_service.auth.get_username(request)
result = module_build_service.auth.get_user(request)
eq_(result, name)
@mock.patch('fedora.client.AccountSystem')
def test_assert_is_packager(self, AccountSystem):
FAS = mock.MagicMock()
FAS.person_by_username.return_value = {
'group_roles': {
'packager': {
'role_status': 'approved',
},
},
}
AccountSystem.return_value = FAS
# This should not raise an exception
module_build_service.auth.assert_is_packager('ralph', dict())
@raises(module_build_service.errors.Unauthorized)
@mock.patch('fedora.client.AccountSystem')
def test_assert_is_packager_failure(self, AccountSystem):
FAS = mock.MagicMock()
FAS.person_by_username.return_value = {
'group_roles': {
'packager': {
'role_status': 'FAILLLL',
},
},
}
AccountSystem.return_value = FAS
# This should not raise an exception
module_build_service.auth.assert_is_packager('ralph', dict())

View File

@@ -32,7 +32,6 @@ from nose.tools import timed
import module_build_service.messaging
import module_build_service.scheduler.handlers.repos
from module_build_service import db, models, conf
from module_build_service.messaging import RidaModule
from mock import patch
@@ -45,6 +44,8 @@ import module_build_service.scheduler.consumer
base_dir = dirname(dirname(__file__))
cassette_dir = base_dir + '/vcr-request-data/'
user = ('Homer J. Simpson', set(['packager']))
class MockedSCM(object):
def __init__(self, mocked_scm, name, mmd_filename):
self.mocked_scm = mocked_scm
@@ -225,11 +226,9 @@ class TestBuild(unittest.TestCase):
self.vcr.__exit__()
@timed(30)
@patch('module_build_service.auth.get_username', return_value='Homer J. Simpson')
@patch('module_build_service.auth.assert_is_packager')
@patch('module_build_service.auth.get_user', return_value=user)
@patch('module_build_service.scm.SCM')
def test_submit_build(self, mocked_scm, mocked_assert_is_packager,
mocked_get_username):
def test_submit_build(self, mocked_scm, mocked_get_user):
"""
Tests the build of testmodule.yaml using TestModuleBuilder which
succeeds everytime.
@@ -281,11 +280,9 @@ class TestBuild(unittest.TestCase):
self.assertEqual(buildroot_groups, [])
@timed(30)
@patch('module_build_service.auth.get_username', return_value='Homer J. Simpson')
@patch('module_build_service.auth.assert_is_packager')
@patch('module_build_service.auth.get_user', return_value=user)
@patch('module_build_service.scm.SCM')
def test_submit_build_cancel(self, mocked_scm, mocked_assert_is_packager,
mocked_get_username):
def test_submit_build_cancel(self, mocked_scm, mocked_get_user):
"""
Submit all builds for a module and cancel the module build later.
"""
@@ -334,11 +331,9 @@ class TestBuild(unittest.TestCase):
self.assertTrue(build.task_id in cancelled_tasks)
@timed(30)
@patch('module_build_service.auth.get_username', return_value='Homer J. Simpson')
@patch('module_build_service.auth.assert_is_packager')
@patch('module_build_service.auth.get_user', return_value=user)
@patch('module_build_service.scm.SCM')
def test_submit_build_instant_complete(self, mocked_scm, mocked_assert_is_packager,
mocked_get_username):
def test_submit_build_instant_complete(self, mocked_scm, mocked_get_user):
"""
Tests the build of testmodule.yaml using TestModuleBuilder which
succeeds everytime.
@@ -366,11 +361,9 @@ class TestBuild(unittest.TestCase):
self.assertTrue(build.module_build.state in [models.BUILD_STATES["done"], models.BUILD_STATES["ready"]] )
@timed(30)
@patch('module_build_service.auth.get_username', return_value='Homer J. Simpson')
@patch('module_build_service.auth.assert_is_packager')
@patch('module_build_service.auth.get_user', return_value=user)
@patch('module_build_service.scm.SCM')
def test_submit_build_concurrent_threshold(
self, mocked_scm, mocked_assert_is_packager, mocked_get_username):
def test_submit_build_concurrent_threshold(self, mocked_scm, mocked_get_user):
"""
Tests the build of testmodule.yaml using TestModuleBuilder with
num_consecutive_builds set to 1.

View File

@@ -33,6 +33,11 @@ from tests import app, init_data
from module_build_service.models import ComponentBuild
import module_build_service.scm
user = ('Homer J. Simpson', set(['packager']))
other_user = ('some_other_user', set(['packager']))
class MockedSCM(object):
def __init__(self, mocked_scm, name, mmd_filenames):
"""
@@ -204,11 +209,9 @@ class TestViews(unittest.TestCase):
' was provided for the \"modified_after\" parameter')
self.assertEquals(data['status'], 400)
@patch('module_build_service.auth.get_username', return_value='Homer J. Simpson')
@patch('module_build_service.auth.assert_is_packager')
@patch('module_build_service.auth.get_user', return_value=user)
@patch('module_build_service.scm.SCM')
def test_submit_build(self, mocked_scm, mocked_assert_is_packager,
mocked_get_username):
def test_submit_build(self, mocked_scm, mocked_get_user):
mocked_scm_obj = MockedSCM(mocked_scm, "fakemodule", "fakemodule.yaml")
rv = self.client.post('/module-build-service/1/module-builds/', data=json.dumps(
@@ -234,11 +237,9 @@ class TestViews(unittest.TestCase):
mmd = _modulemd.ModuleMetadata()
mmd.loads(data["modulemd"])
@patch('module_build_service.auth.get_username', return_value='Homer J. Simpson')
@patch('module_build_service.auth.assert_is_packager')
@patch('module_build_service.auth.get_user', return_value=user)
@patch('module_build_service.scm.SCM')
def test_submit_componentless_build(self, mocked_scm, mocked_assert_is_packager,
mocked_get_username):
def test_submit_componentless_build(self, mocked_scm, mocked_get_user):
mocked_scm_obj = MockedSCM(mocked_scm, "fakemodule2", "fakemodule2.yaml")
rv = self.client.post('/module-build-service/1/module-builds/', data=json.dumps(
@@ -273,10 +274,8 @@ class TestViews(unittest.TestCase):
self.assertEquals(data['status'], 401)
self.assertEquals(data['error'], 'Unauthorized')
@patch('module_build_service.auth.get_username', return_value='Homer J. Simpson')
@patch('module_build_service.auth.assert_is_packager')
def test_submit_build_scm_url_error(self, mocked_assert_is_packager,
mocked_get_username):
@patch('module_build_service.auth.get_user', return_value=user)
def test_submit_build_scm_url_error(self, mocked_get_user):
rv = self.client.post('/module-build-service/1/module-builds/', data=json.dumps(
{'scmurl': 'git://badurl.com'}))
data = json.loads(rv.data)
@@ -285,11 +284,8 @@ class TestViews(unittest.TestCase):
self.assertEquals(data['status'], 401)
self.assertEquals(data['error'], 'Unauthorized')
@patch('module_build_service.auth.get_username', return_value='Homer J. Simpson')
@patch('module_build_service.auth.assert_is_packager')
def test_submit_build_scm_url_without_hash(self,
mocked_assert_is_packager,
mocked_get_username):
@patch('module_build_service.auth.get_user', return_value=user)
def test_submit_build_scm_url_without_hash(self, mocked_get_user):
rv = self.client.post('/module-build-service/1/module-builds/', data=json.dumps(
{'scmurl': 'git://pkgs.stg.fedoraproject.org/modules/'
'testmodule.git'}))
@@ -300,12 +296,9 @@ class TestViews(unittest.TestCase):
self.assertEquals(data['status'], 401)
self.assertEquals(data['error'], 'Unauthorized')
@patch('module_build_service.auth.get_username', return_value='Homer J. Simpson')
@patch('module_build_service.auth.assert_is_packager')
@patch('module_build_service.auth.get_user', return_value=user)
@patch('module_build_service.scm.SCM')
def test_submit_build_bad_modulemd(self, mocked_scm,
mocked_assert_is_packager,
mocked_get_username):
def test_submit_build_bad_modulemd(self, mocked_scm, mocked_get_user):
mocked_scm_obj = MockedSCM(mocked_scm, "bad", "bad.yaml")
rv = self.client.post('/module-build-service/1/module-builds/', data=json.dumps(
@@ -316,11 +309,10 @@ class TestViews(unittest.TestCase):
self.assertEquals(data['status'], 422)
self.assertEquals(data['error'], 'Unprocessable Entity')
@patch('module_build_service.auth.get_username', return_value='Homer J. Simpson')
@patch('module_build_service.auth.assert_is_packager')
@patch('module_build_service.auth.get_user', return_value=user)
@patch('module_build_service.scm.SCM')
def test_submit_build_scm_parallalization(self, mocked_scm,
mocked_assert_is_packager, mocked_get_username):
mocked_get_user):
def mocked_scm_get_latest(branch = "master"):
time.sleep(1)
return branch
@@ -352,11 +344,10 @@ class TestViews(unittest.TestCase):
# max to complete.
self.assertTrue(time.time() - start < 3)
@patch('module_build_service.auth.get_username', return_value='Homer J. Simpson')
@patch('module_build_service.auth.assert_is_packager')
@patch('module_build_service.auth.get_user', return_value=user)
@patch('module_build_service.scm.SCM')
def test_submit_build_scm_non_available(self, mocked_scm,
mocked_assert_is_packager, mocked_get_username):
mocked_get_user):
def mocked_scm_get_latest():
raise RuntimeError("Failed in mocked_scm_get_latest")
@@ -372,11 +363,10 @@ class TestViews(unittest.TestCase):
self.assertEquals(data['message'][:31], "Failed to get the latest commit")
self.assertEquals(data['error'], "Unprocessable Entity")
@patch('module_build_service.auth.get_username', return_value='Homer J. Simpson')
@patch('module_build_service.auth.assert_is_packager')
@patch('module_build_service.auth.get_user', return_value=user)
@patch('module_build_service.scm.SCM')
def test_submit_build_includedmodule(self, mocked_scm, mocked_assert_is_packager,
mocked_get_username):
def test_submit_build_includedmodule(self, mocked_scm,
mocked_get_user):
mocked_scm_obj = MockedSCM(mocked_scm, "includedmodules",
["includedmodules.yaml", "fakemodule.yaml"])
@@ -408,10 +398,8 @@ class TestViews(unittest.TestCase):
self.assertEquals(batches["bash"], 2)
self.assertEquals(batches["file"], 3)
@patch('module_build_service.auth.get_username', return_value='some_other_user')
@patch('module_build_service.auth.assert_is_packager')
def test_cancel_build(self, mocked_assert_is_packager,
mocked_get_username):
@patch('module_build_service.auth.get_user', return_value=other_user)
def test_cancel_build(self, mocked_get_user):
rv = self.client.patch('/module-build-service/1/module-builds/30',
data=json.dumps({'state': 'failed'}))
data = json.loads(rv.data)
@@ -420,10 +408,8 @@ class TestViews(unittest.TestCase):
self.assertEquals(data['state_reason'], 'Canceled by some_other_user.')
@patch('module_build_service.auth.get_username', return_value='Someone else')
@patch('module_build_service.auth.assert_is_packager')
def test_cancel_build_unauthorized(self, mocked_assert_is_packager,
mocked_get_username):
@patch('module_build_service.auth.get_user', return_value=('sammy', set()))
def test_cancel_build_unauthorized(self, mocked_get_user):
rv = self.client.patch('/module-build-service/1/module-builds/30',
data=json.dumps({'state': 'failed'}))
data = json.loads(rv.data)
@@ -431,10 +417,8 @@ class TestViews(unittest.TestCase):
self.assertEquals(data['status'], 401)
self.assertEquals(data['error'], 'Unauthorized')
@patch('module_build_service.auth.get_username', return_value='some_other_user')
@patch('module_build_service.auth.assert_is_packager')
def test_cancel_build_wrong_param(self, mocked_assert_is_packager,
mocked_get_username):
@patch('module_build_service.auth.get_user', return_value=other_user)
def test_cancel_build_wrong_param(self, mocked_get_user):
rv = self.client.patch('/module-build-service/1/module-builds/30',
data=json.dumps({'some_param': 'value'}))
data = json.loads(rv.data)
@@ -444,10 +428,8 @@ class TestViews(unittest.TestCase):
self.assertEquals(
data['message'], 'Invalid JSON submitted')
@patch('module_build_service.auth.get_username', return_value='some_other_user')
@patch('module_build_service.auth.assert_is_packager')
def test_cancel_build_wrong_state(self, mocked_assert_is_packager,
mocked_get_username):
@patch('module_build_service.auth.get_user', return_value=other_user)
def test_cancel_build_wrong_state(self, mocked_get_user):
rv = self.client.patch('/module-build-service/1/module-builds/30',
data=json.dumps({'state': 'some_state'}))
data = json.loads(rv.data)
@@ -457,10 +439,8 @@ class TestViews(unittest.TestCase):
self.assertEquals(
data['message'], 'The provided state change is not supported')
@patch('module_build_service.auth.get_username', return_value='Homer J. Simpson')
@patch('module_build_service.auth.assert_is_packager')
def test_submit_build_unsupported_scm_scheme(self, mocked_assert_is_packager,
mocked_get_username):
@patch('module_build_service.auth.get_user', return_value=user)
def test_submit_build_unsupported_scm_scheme(self, mocked_get_user):
scmurl = 'unsupported://example.com/modules/'
'testmodule.git?#0000000000000000000000000000000000000000'
rv = self.client.post('/module-build-service/1/module-builds/', data=json.dumps(