#208 Port hubs to use openid-connect
Merged 7 years ago by pingou. Opened 7 years ago by pingou.
pingou/fedora-hubs port_oidc  into  develop

file modified
+9 -1
@@ -47,6 +47,10 @@ 

  

      $ pip install -r requirements.txt

  

+ Configure the project to authenticate against iddev.fedorainfraclouid.org::

+ 

+     oidc-register --debug https://iddev.fedorainfracloud.org/ http://localhost:5000

+ 

  With that, try running the app with::

  

      $ python populate.py  # To create the db
@@ -77,7 +81,11 @@ 

  You can run it with::

  

      $ pip install -r test-requirements.txt

-     $ PYTHONPATH=. nosetests

+     $ ./runtest.sh

+ 

+ See the options available with::

+ 

+     $ ./runtest.sh --help

  

  Some credentials...

  -------------------

file modified
+65 -70
@@ -3,6 +3,7 @@ 

  import json

  import logging

  import os

+ import urlparse

  import uuid

  

  import flask
@@ -12,7 +13,7 @@ 

  import pygments.formatters

  import six

  

- from flask.ext.openid import OpenID

+ from flask.ext.oidc import OpenIDConnect

  

  import fmn.lib

  import hubs.models
@@ -75,6 +76,17 @@ 

          and flask.g.auth.logged_in

  

  

+ def is_safe_url(target):

+     """ Checks that the target url is safe and sending to the current

+     website not some other malicious one.

+     """

+     ref_url = urlparse.urlparse(flask.request.host_url)

+     test_url = urlparse.urlparse(

+         urlparse.urljoin(flask.request.host_url, target))

+     return test_url.scheme in ('http', 'https') and \

+         ref_url.netloc == test_url.netloc

+ 

+ 

  @app.template_filter('commas')

  def commas(numeric):

      return "{:,.2f}".format(numeric)
@@ -89,7 +101,7 @@ 

  @app.route('/')

  def index():

      if not authenticated():

-         return flask.redirect(flask.url_for('login_fedora'))

+         return flask.redirect(flask.url_for('login'))

  

      return flask.redirect(flask.url_for('hub', name=flask.g.auth.nickname))

  
@@ -97,7 +109,7 @@ 

  @app.route('/groups')

  def groups():

      if not authenticated():

-         return flask.redirect(flask.url_for('login_fedora'))

+         return flask.redirect(flask.url_for('login'))

  

      # Get the list of promoted and non-promoted group hubs from the DB

      promoted_names = app.config.get('PROMOTED_GROUPS')
@@ -446,69 +458,43 @@ 

      return flask.redirect(SOURCE_URL + fname)

  

  

- # Set up OpenID in stateless mode

- oid = OpenID(app,

-              safe_roots=[],

-              store_factory=lambda: None,

-              url_root_as_trust_root=True)

+ # Set up OpenIDConnect

+ OIDC = OpenIDConnect(app, credentials_store=flask.session )

  

  

  @app.route('/login/', methods=('GET', 'POST'))

  @app.route('/login', methods=('GET', 'POST'))

- @oid.loginhandler

+ @OIDC.require_login

  def login():

      default = flask.url_for('index')

      next_url = flask.request.args.get('next', default)

-     if authenticated():

-         return flask.redirect(next_url)

- 

-     openid_server = flask.request.form.get('openid', None)

-     if openid_server:

-         return oid.try_login(

-             openid_server, ask_for=['email', 'fullname', 'nickname'],

-             ask_for_optional=[])

- 

-     return flask.render_template(

-         'login.html', next=oid.get_next_url(), error=oid.fetch_error())

+     if is_safe_url(next_url):

+         return_point = next_url

+     else:

+         return_point = default

  

+     hubs.models.User.get_or_create(

+         session, username=flask.g.auth.nickname, fullname=flask.g.auth.fullname)

  

- @app.route('/login/fedora/')

- @app.route('/login/fedora')

- @oid.loginhandler

- def login_fedora():

-     # default = flask.url_for('profile_redirect')

-     # next_url = flask.request.args.get('next', default)

-     return oid.try_login(

-         'https://id.fedoraproject.org',

-         ask_for=['email', 'fullname', 'nickname'],

-         ask_for_optional=[])

+     return flask.redirect(return_point)

  

  

  @app.route('/logout/')

  @app.route('/logout')

  def logout():

-     if 'openid' in flask.app.session:

-         flask.app.session.pop('openid')

-     return flask.redirect(flask.url_for('index'))

+     next_url = flask.url_for('index')

+     if 'next' in flask.request.values:  # pragma: no cover

+         if is_safe_url(flask.request.args['next']):

+             next_url = flask.request.values['next']

  

+     if next_url == flask.url_for('login'):  # pragma: no cover

+         next_url = flask.url_for('index')

  

- @oid.after_login

- def after_openid_login(resp):

-     default = flask.url_for('index')

-     if not resp.identity_url:

-         return flask.redirect(default)

- 

-     openid_url = resp.identity_url

-     flask.app.session['openid'] = openid_url

-     flask.app.session['fullname'] = resp.fullname

-     flask.app.session['nickname'] = resp.nickname or resp.fullname

-     flask.app.session['email'] = resp.email

- 

-     openid = openid_url.strip('/').split('/')[-1]

-     hubs.models.User.get_or_create(

-         session, openid=openid, fullname=resp.fullname)

+     if authenticated():

+         OIDC.logout()

+         flask.session.auth = None

+         flask.flash('You have been logged out')

  

-     next_url = flask.request.args.get('next', default)

      return flask.redirect(next_url)

  

  
@@ -521,7 +507,7 @@ 

          if not authenticated():

              flask.flash('Login required', 'errors')

              return flask.redirect(flask.url_for(

-                 'login_fedora', next=flask.request.url))

+                 'login', next=flask.request.url))

  

          return function(*args, **kwargs)

  
@@ -530,21 +516,30 @@ 

  

  @app.before_request

  def check_auth():

+     flask.session.permanent = True

      flask.g.fedmsg_config = fedmsg_config

-     flask.g.auth = munch.Munch(logged_in=False)

-     if 'openid' in flask.session:

-         openid = flask.session.get('openid')

-         if isinstance(openid, six.binary_type):

-             openid = openid.decode('utf-8')

-         openid = openid.strip('/').split('/')[-1]

-         flask.g.auth.logged_in = True

-         flask.g.auth.openid = openid

-         flask.g.auth.user = hubs.models.User.by_openid(session, openid)

-         flask.g.auth.openid_url = flask.session.get('openid')

-         flask.g.auth.fullname = flask.session.get('fullname', None)

-         flask.g.auth.nickname = flask.session.get('nickname', None)

-         flask.g.auth.email = flask.session.get('email', None)

-         flask.g.auth.avatar = username2avatar(flask.g.auth.nickname)

+ 

+     if OIDC.user_loggedin:

+         if not hasattr(flask.session, 'auth') or not flask.session.auth:

+             flask.session.auth = munch.Munch(

+                 fullname=OIDC.user_getfield('name'),

+                 nickname=OIDC.user_getfield('nickname') \

+                     or OIDC.user_getfield('sub'),

+                 email=OIDC.user_getfield('email'),

+                 timezone=OIDC.user_getfield('zoneinfo'),

+                 cla_done=\

+                     'http://admin.fedoraproject.org/accounts/cla/done' \

+                     in OIDC.user_getfield('cla'),

+                 groups=OIDC.user_getfield('groups'),

+                 logged_in=True,

+             )

+             flask.session.auth.avatar=username2avatar(

+                 flask.session.auth.nickname)

+             flask.session.auth.user = hubs.models.User.by_username(

+                 session, flask.session.auth.nickname)

+         flask.g.auth = flask.session.auth

+     else:

+         flask.g.auth = munch.Munch(logged_in=False)

  

  

  def get_hub(session, name):
@@ -581,7 +576,7 @@ 

  @login_required

  def hub_subscribe(hub):

      hub = get_hub(session, hub)

-     user = hubs.models.User.by_openid(session, flask.g.auth.openid)

+     user = hubs.models.User.by_username(session, flask.g.auth.nickname)

      hub.subscribe(session, user)

      session.commit()

      return flask.redirect(flask.url_for('hub', name=hub.name))
@@ -591,7 +586,7 @@ 

  @login_required

  def hub_unsubscribe(hub):

      hub = get_hub(session, hub)

-     user = hubs.models.User.by_openid(session, flask.g.auth.openid)

+     user = hubs.models.User.by_username(session, flask.g.auth.nickname)

      try:

          hub.unsubscribe(session, user)

      except KeyError:
@@ -604,7 +599,7 @@ 

  @login_required

  def hub_star(hub):

      hub = get_hub(session, hub)

-     user = hubs.models.User.by_openid(session, flask.g.auth.openid)

+     user = hubs.models.User.by_username(session, flask.g.auth.nickname)

      hub.subscribe(session, user, role='stargazer')

      session.commit()

      return flask.redirect(flask.url_for('hub', name=hub.name))
@@ -614,7 +609,7 @@ 

  @login_required

  def hub_unstar(hub):

      hub = get_hub(session, hub)

-     user = hubs.models.User.by_openid(session, flask.g.auth.openid)

+     user = hubs.models.User.by_username(session, flask.g.auth.nickname)

      try:

          hub.unsubscribe(session, user, role='stargazer')

      except KeyError:
@@ -627,7 +622,7 @@ 

  @login_required

  def hub_join(hub):

      hub = get_hub(session, hub)

-     user = hubs.models.User.by_openid(session, flask.g.auth.openid)

+     user = hubs.models.User.by_username(session, flask.g.auth.nickname)

      hub.subscribe(session, user, role='member')

      session.commit()

      return flask.redirect(flask.url_for('hub', name=hub.name))
@@ -637,7 +632,7 @@ 

  @login_required

  def hub_leave(hub):

      hub = get_hub(session, hub)

-     user = hubs.models.User.by_openid(session, flask.g.auth.openid)

+     user = hubs.models.User.by_username(session, flask.g.auth.nickname)

      try:

          hub.unsubscribe(session, user, role='member')

      except KeyError:

file modified
+13
@@ -1,3 +1,5 @@ 

+ import os

+ 

  SECRET_KEY = 'changemeforreal'

  

  PROMOTED_GROUPS = [
@@ -6,3 +8,14 @@ 

  ]

  

  HUB_OF_THE_MONTH = 'commops'

+ 

+ 

+ OIDC_CLIENT_SECRETS = os.path.join(os.path.dirname(

+     os.path.abspath(__file__)), '..', 'client_secrets.json')

+ # This settings means that the application needs to be run behind http*s* for

+ # the cookie to be saved. For development you will likely need to make it

+ # `False`

Perhaps instead just have runserver.py set this to False?
Since runserver itself is meant for development setups.

I'm more thinking on improving the runserver to allow specifying a configuration file to use for dev (and which would allow setting this configuration key to False while keeping the default correct). Sounds good?

+ OIDC_ID_TOKEN_COOKIE_SECURE = True

+ OIDC_REQUIRE_VERIFIED_EMAIL = False

+ OIDC_OPENID_REALM = 'http://localhost:5000/oidc_callback'

+ OIDC_SCOPES = ['openid', 'email', 'profile', 'fedora']

file modified
+11 -20
@@ -45,13 +45,13 @@ 

  

  

  class HubsBase(object):

-     def notify(self, openid, changed):

+     def notify(self, username, changed):

          obj = type(self).__name__.lower()

          topic = obj + ".update"

          fedmsg.publish(

              topic=topic,

              msg=dict(

-                 openid=openid,

+                 username=username,

                  changed=changed,

              )

          )
@@ -103,7 +103,7 @@ 

                         sa.ForeignKey('hubs.name'),

                         primary_key=True)

      user_id = sa.Column(sa.Text,

-                         sa.ForeignKey('users.openid'),

+                         sa.ForeignKey('users.username'),

                          primary_key=True)

      role = sa.Column(sa.Enum(*roles), primary_key=True)

  
@@ -350,14 +350,13 @@ 

  

  class User(BASE):

      __tablename__ = 'users'

-     openid = sa.Column(sa.Text, primary_key=True)

+     username = sa.Column(sa.Text, primary_key=True)

      fullname = sa.Column(sa.Text)

      created_on = sa.Column(sa.DateTime, default=datetime.datetime.utcnow)

  

      def __json__(self, session):

          return {

              'username': self.username,

-             'openid': self.openid,

              'avatar': username2avatar(self.username),

              'fullname': self.fullname,

              'created_on': self.created_on,
@@ -396,32 +395,24 @@ 

              and assoc.hub.name != self.username

          ])), key=operator.attrgetter('name'))

  

-     @property

-     def username(self):

-         return self.openid.split('.')[0]

- 

      @classmethod

      def by_username(cls, session, username):

-         return cls.by_openid(session, "%s.id.fedoraproject.org" % username)

- 

-     @classmethod

-     def by_openid(cls, session, openid):

-         return session.query(cls).filter_by(openid=openid).first()

+         return session.query(cls).filter_by(username=username).first()

  

-     get = by_openid

+     get = by_username

  

      @classmethod

      def all(cls, session):

          return session.query(cls).all()

  

      @classmethod

-     def get_or_create(cls, session, openid, fullname):

-         if not openid:

-             raise ValueError("Must provide openid, not %r" % openid)

+     def get_or_create(cls, session, username, fullname):

+         if not username:

+             raise ValueError("Must provide an username, not %r" % username)

  

-         self = cls.by_openid(session, openid)

+         self = cls.by_username(session, username)

          if not self:

-             self = cls(openid=openid, fullname=fullname)

+             self = cls(username=username, fullname=fullname)

              session.add(self)

              if not Hub.by_name(session, self.username):

                  Hub.create_user_hub(session, self.username, self.fullname)

@@ -3,8 +3,3 @@ 

  

  import Feed from './components/Feed.jsx'

  

- 

- render(

-     <Feed matches={[]} />,

-     document.getElementById('feed')

- );

file modified
+1 -1
@@ -37,7 +37,7 @@ 

          </div>

          <img id="chaticon" class="pull-xs-right m-r-2" src="{{ url_for('static', filename='img/irc_icon.png') }}" alt="IRC Chats">

        {% else %}

-         Not logged in.  Click to <a href="{{url_for('login_fedora')}}">login</a>.

+         Not logged in.  Click to <a href="{{url_for('login')}}">login</a>.

        {% endif %}

      </div>

      <div class="col-sm-4 col-sm-pull-4 col-xs-12">

file modified
+2 -4
@@ -42,10 +42,9 @@ 

  

      def populate(self):

          for user in ['devyani7', 'dhrish', 'shalini', 'ralph', 'decause']:

-             openid = '%s.id.fedoraproject.org' % user

              fullname = user.title()

              hubs.models.User.get_or_create(

-                 hubs.app.session, openid=openid, fullname=fullname)

+                 hubs.app.session, username=user, fullname=fullname)

  

          hubs.app.session.flush()

  
@@ -106,6 +105,7 @@ 

  

      def handler(sender, **kwargs):

          g.auth = auth

+         g.oidc_id_token = None

This is just a workaround for a bug in flask where it doesn't run before_request.

(Which is going to break your setting of flask.g.auth as well).

I still consider this more a bug in flask-oidc than flask and even if it's not the OS we target will likely not be updated to a newer flask any time soon

Well, it's Flask that's not executing the before_request...

And flask-oidc assuming having something in g while it may not be true :)

It is inserting it on every request before_request. The flask documentation says: "Registers a function to run before each request.". Mark the "Each request" part.

Is there a ticket upstream I could follow?

So.... this is because you actively clear all before_requests in tests/init.py: APP.before_request_funcs[None] = []

          if not auth:

              g.auth = munch.Munch(logged_in=False)

  
@@ -122,7 +122,6 @@ 

              supposed to be.

          """

          self.username = username

-         self.openid = username + '.id.fedoraproject.org'

          self.booksmarks = []

  

      def __getitem__(self, key):
@@ -140,7 +139,6 @@ 

          self.logged_in = True

          self.fullname = 'fullname: ' + username

          self.email = 'email: ' + username

-         self.openid = username + '.id.fedoraproject.org'

          self.user = FakeUser(username)

          self.avatar = 'avatar_src_url'

          self.nickname = username

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

+ {

+ 	"web": {

+ 		"redirect_uris": ["http://localhost:5002/oidc_callback"],

+ 		"token_uri": "https://iddev.fedorainfracloud.org/openidc/Token",

+ 		"auth_uri": "https://iddev.fedorainfracloud.org/openidc/Authorization",

+ 		"client_id": "client_id",

+ 		"client_secret": "client_secret",

+ 		"userinfo_uri": "https://iddev.fedorainfracloud.org/openidc/UserInfo",

+ 		"token_introspection_uri": "https://iddev.fedorainfracloud.org/openidc/TokenInfo",

+ 		"issuer": "https://iddev.fedorainfracloud.org/openidc/"

+ 	}

+ }

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

+ ### Secret key for the Flask application

+ SECRET_KEY='<The web application secret key>'

+ 

+ ### url to the database server:

+ import os

+ DB_URL = 'sqlite:///%s/test.db' % (os.path.dirname(os.path.abspath(__file__)))

+ #DB_URL='sqlite:////tmp/fedocal_dev.sqlite'

+ 

+ import os

+ OIDC_CLIENT_SECRETS = os.path.join(os.path.dirname(

+     os.path.abspath(__file__)), 'client_secrets.json')

@@ -54,7 +54,6 @@ 

              'plugin': 'feed'

          }

          with self.app.session_transaction() as sess:

-             sess['openid'] = 'atelic@fedoraproject.org'

              sess['nickname'] = 'atelic'

  

          with hubs.tests.auth_set(app, self.user):
@@ -71,7 +70,6 @@ 

              'plugin': 'feed'

          }

          with self.app.session_transaction() as sess:

-             sess['openid'] = 'atelic@fedoraproject.org'

              sess['nickname'] = 'atelic'

  

          with hubs.tests.auth_set(app, self.user):

@@ -18,7 +18,7 @@ 

          resp = self.app.post('/api/hub/{}/subscribe'.format(hub.name),

                               follow_redirects=False)

          self.assertEqual(resp.status_code, 302)

-         self.assertEqual(urlparse(resp.location).path, '/login/fedora')

+         self.assertEqual(urlparse(resp.location).path, '/login')

  

      def test_subscribe_when_logged_in(self):

          hub = hubs.models.Hub.by_name(self.session, 'infra')
@@ -39,7 +39,7 @@ 

          resp = self.app.post('/api/hub/{}/unsubscribe'.format(hub.name),

                               follow_redirects=False)

          self.assertEqual(resp.status_code, 302)

-         self.assertEqual(urlparse(resp.location).path, '/login/fedora')

+         self.assertEqual(urlparse(resp.location).path, '/login')

  

      def test_unsubscribe_when_logged_in(self):

          hub = hubs.models.Hub.by_name(self.session, 'infra')
@@ -65,7 +65,7 @@ 

          resp = self.app.post('/api/hub/{}/star'.format(hub.name),

                               follow_redirects=False)

          self.assertEqual(resp.status_code, 302)

-         self.assertEqual(urlparse(resp.location).path, '/login/fedora')

+         self.assertEqual(urlparse(resp.location).path, '/login')

  

      def test_star_when_logged_in(self):

          hub = hubs.models.Hub.by_name(self.session, 'infra')
@@ -86,7 +86,7 @@ 

          resp = self.app.post('/api/hub/{}/unstar'.format(hub.name),

                               follow_redirects=False)

          self.assertEqual(resp.status_code, 302)

-         self.assertEqual(urlparse(resp.location).path, '/login/fedora')

+         self.assertEqual(urlparse(resp.location).path, '/login')

  

      def test_unstar_when_logged_in(self):

          hub = hubs.models.Hub.by_name(self.session, 'infra')
@@ -113,7 +113,7 @@ 

          resp = self.app.post('/api/hub/{}/join'.format(hub.name),

                               follow_redirects=False)

          self.assertEqual(resp.status_code, 302)

-         self.assertEqual(urlparse(resp.location).path, '/login/fedora')

+         self.assertEqual(urlparse(resp.location).path, '/login')

  

      def test_join_when_logged_in(self):

          hub = hubs.models.Hub.by_name(self.session, 'infra')
@@ -134,7 +134,7 @@ 

          resp = self.app.post('/api/hub/{}/leave'.format(hub.name),

                               follow_redirects=False)

          self.assertEqual(resp.status_code, 302)

-         self.assertEqual(urlparse(resp.location).path, '/login/fedora')

+         self.assertEqual(urlparse(resp.location).path, '/login')

  

      def test_star_when_logged_in(self):

          hub = hubs.models.Hub.by_name(self.session, 'infra')

@@ -21,7 +21,7 @@ 

      def test_index_logged_out(self):

          result = self.app.get('/', follow_redirects=False)

          self.assertEqual(result.status_code, 302)

-         self.assertEqual(urlparse(result.location).path, "/login/fedora")

+         self.assertEqual(urlparse(result.location).path, "/login")

  

      def test_index_logged_in(self):

          user = tests.FakeAuthorization('ralph')
@@ -31,10 +31,12 @@ 

              # assert the status code of the response

              self.assertEqual(result.status_code, 200)

              self.assertFalse('Not logged in.  Click to <a href="'

-                              '/login/fedora">login</a>' in result.data)

+                              '/login">login</a>' in result.data)

  

      def test_hub_logged_out(self):

          with app.test_request_context('/ralph'):

+             import flask

+             flask.g.oidc_id_token = None

              # need to manually call the @app.before_request

              # since unittest don't call it

              hubs.app.check_auth()
@@ -42,7 +44,7 @@ 

              # assert the status code of the response

              self.assertEqual(result.status_code, 200)

              str_expected = 'Not logged in.  Click to ' \

-                            '<a href="/login/fedora">login</a>'

+                            '<a href="/login">login</a>'

              self.assertTrue(str_expected in result.data)

  

      def test_groups_logged_out(self):
@@ -50,7 +52,7 @@ 

          # assert the status code of the response

          self.assertEqual(result.status_code, 302)

          # this will redirect to fedora.login

-         self.assertEqual(urlparse(result.location).path, "/login/fedora")

+         self.assertEqual(urlparse(result.location).path, "/login")

  

      def test_groups_logged_in(self):

          user = tests.FakeAuthorization('ralph')
@@ -66,7 +68,7 @@ 

              result = self.app.get('/ralph', follow_redirects=True)

              self.assertEqual(result.status_code, 200)

              self.assertFalse('Not logged in.  Click to <a href="'

-                              '/login/fedora">login</a>' in result.data)

+                              '/login">login</a>' in result.data)

  

      def test_hub_json(self):

          result = self.app.get('/ralph/json', follow_redirects=True)
@@ -134,7 +136,7 @@ 

                                     follow_redirects=True)

              self.assertEqual(result.status_code, 200)

              self.assertFalse('Not logged in.  Click to <a href="'

-                              '/login/fedora">login</a>' in result.data)

+                              '/login">login</a>' in result.data)

              self.assertTrue('<a href="#" class="dropdown-item">Full Name: '

                              'fullname: ralph</a>' in result.data)

  
@@ -204,7 +206,9 @@ 

          with tests.auth_set(app, user):

              result = self.app.get('/login', follow_redirects=False)

              self.assertEqual(result.status_code, 302)

-             self.assertEqual(urlparse(result.location).path, "/")

+             self.assertEqual(

+                 urlparse(result.location).path,

+                 "/openidc/Authorization")

  

      def test_hub_add_widget_get_no_args(self):

          result = self.app.get('/ralph/add', follow_redirects=False)

file modified
+9 -17
@@ -8,12 +8,9 @@ 

  

  class ModelTest(hubs.tests.APPTest):

      def test_delete_user(self):

-         self.session = hubs.models.init(fedmsg_config['hubs.sqlalchemy.uri'])

- 

          # verify user exists

          username = 'ralph'

-         openid = '%s.id.fedoraproject.org' % username

-         user = hubs.models.User.get(self.session, openid)

+         user = hubs.models.User.get(self.session, username)

          self.assertIsNotNone(user)

  

          # check if association exists
@@ -23,7 +20,7 @@ 

  

          # delete the user

          self.session.delete(user)

-         user = hubs.models.User.get(self.session, openid)

+         user = hubs.models.User.get(self.session, username)

          self.assertIsNone(user)

  

          # checking to see if the hub is still intact
@@ -33,11 +30,9 @@ 

  

          # check if widgets still are intact

          widgets = hubs.models.Widget.by_hub_id_all(self.session, hub.name)

-         self.assertEqual(8, len(widgets))

+         self.assertEqual(11, len(widgets))

  

      def test_delete_hubs(self):

-         self.session = hubs.models.init(fedmsg_config['hubs.sqlalchemy.uri'])

- 

          # verify the hub exists

          hub_name = 'ralph'

          hub = hubs.models.Hub.get(self.session, hub_name)
@@ -45,14 +40,13 @@ 

  

          # check if association exists

          username = 'ralph'

-         openid = '%s.id.fedoraproject.org' % username

-         user = hubs.models.User.get(self.session, openid)

+         user = hubs.models.User.get(self.session, username)

          assoc = hubs.models.Association.get(self.session, hub, user, 'owner')

          self.assertIsNotNone(assoc)

  

          # check if widgets exist

          widgets = hubs.models.Widget.by_hub_id_all(self.session, hub.name)

-         self.assertEqual(8, len(widgets))

+         self.assertEqual(11, len(widgets))

did we add more widgets?

Not quite sure tbh, it is odd indeed

Just verified that all 11 widgets belong to the user ralph. This is fine.

00 = {Widget} <Widget feed /ralph/31>

01 = {Widget} <Widget meetings /ralph/32>

02 = {Widget} <Widget fedmsgstats /ralph/33>

03 = {Widget} <Widget workflow.updates2stable /ralph/34>

04 = {Widget} <Widget workflow.pendingacls /ralph/35>

05 = {Widget} <Widget subscriptions /ralph/36>

06 = {Widget} <Widget badges /ralph/37>

07 = {Widget} <Widget pagure_pr /ralph/38>

08 = {Widget} <Widget github_pr /ralph/39>

09 = {Widget} <Widget bugzilla /ralph/40>

10 = {Widget} <Widget about /ralph/51>

  

          # delete the hub

          self.session.delete(hub)
@@ -70,18 +64,16 @@ 

          self.assertIsNone(hub)

  

          # check if user is still intact

-         user = hubs.models.User.get(self.session, openid)

+         user = hubs.models.User.get(self.session, username)

          self.assertIsNotNone(user)

-         self.assertIn('ralph', user.openid)

+         self.assertEqual('ralph', user.username)

  

      def test_delete_user_then_hubs(self):

-         self.session = hubs.models.init(fedmsg_config['hubs.sqlalchemy.uri'])

          username = 'ralph'

-         openid = '%s.id.fedoraproject.org' % username

-         user = hubs.models.User.get(self.session, openid)

+         user = hubs.models.User.get(self.session, username)

          self.assertIsNotNone(user)

          self.session.delete(user)

-         user = hubs.models.User.get(self.session, openid)

+         user = hubs.models.User.get(self.session, username)

          self.assertIsNone(user)

  

          # checking to see if the hub is still intact

@@ -36,10 +36,9 @@ 

  

      def populate(self):

          for user in ['devyani7', 'dhrish', 'shalini', 'ralph', 'decause']:

-             openid = '%s.id.fedoraproject.org' % user

              fullname = user.title()

              hubs.models.User.get_or_create(

-                 hubs.app.session, openid=openid, fullname=fullname)

+                 hubs.app.session, username=user, fullname=fullname)

  

          hubs.app.session.flush()

  

file modified
+1 -2
@@ -21,8 +21,7 @@ 

  

  

  def username(session, value):

-     openid = 'http://%s.id.fedoraproject.org/' % value

-     return not hubs.models.User.by_openid(session, openid) is None

+     return hubs.models.User.by_username(session, value) is not None

  

  

  def github_organization(session, value):

file added
+9
@@ -0,0 +1,9 @@ 

+ #!/usr/bin/env python

+ # EASY-INSTALL-ENTRY-SCRIPT: 'nose==0.10.4','console_scripts','nosetests'

+ __requires__ = ['nose>=0.10.4', 'SQLAlchemy >= 0.7', 'jinja2 >= 2.4']

+ import sys

+ from pkg_resources import load_entry_point

+ 

+ sys.exit(

+    load_entry_point('nose>=0.10.4', 'console_scripts', 'nosetests')()

+ )

file modified
+6 -6
@@ -38,19 +38,19 @@ 

      session = hubs.models.init(

          fedmsg_config['hubs.sqlalchemy.uri'], True, True)

      print "Querying FAS for the %r users.. hang on." % letter

-     request = fasclient.send_request('/user/list',

-                                      req_params={'search': '%s*' % letter},

-                                      auth=True,

-                                      timeout=500)

+     request = fasclient.send_request(

+         '/user/list',

+         req_params={'search': '%s*' % letter, 'status': 'active'},

+         auth=True,

+         timeout=500)

      users = request['people']

  

      for user in users:

          username = user['username']

          fullname = user['human_name']

-         openid = '%s.id.fedoraproject.org' % username

          print "Creating account for %r" % openid

          hubs_user = hubs.models.User.get_or_create(

-             session, openid=openid, fullname=fullname)

+             session, username=username, fullname=fullname)

  

      session.commit()

  

file modified
+28 -29
@@ -15,10 +15,9 @@ 

           'pravins', 'keekri', 'linuxmodder', 'bee2502', 'jflory7']

  for username in users:

      fullname = 'Full Name Goes Here'

-     openid = '%s.id.fedoraproject.org' % username

-     print("Creating account for %r" % openid)

+     print("Creating account for %r" % username)

      hubs.models.User.get_or_create(

-         session, openid=openid, fullname=fullname)

+         session, username=username, fullname=fullname)

  

  session.commit()

  
@@ -59,9 +58,9 @@ 

  

  

  # Set up some memberships

- hub.subscribe(session, hubs.models.User.by_openid(session, 'pravins.id.fedoraproject.org'), 'owner')

- hub.subscribe(session, hubs.models.User.by_openid(session, 'decause.id.fedoraproject.org'), 'member')

- hub.subscribe(session, hubs.models.User.by_openid(session, 'ralph.id.fedoraproject.org'), 'subscriber')

+ hub.subscribe(session, hubs.models.User.by_username(session, 'pravins'), 'owner')

+ hub.subscribe(session, hubs.models.User.by_username(session, 'decause'), 'member')

+ hub.subscribe(session, hubs.models.User.by_username(session, 'ralph'), 'subscriber')

  

  

  session.commit()
@@ -102,11 +101,11 @@ 

  

  

  # Set up some memberships

- hub.subscribe(session, hubs.models.User.by_openid(session, 'decause.id.fedoraproject.org'), 'owner')

- hub.subscribe(session, hubs.models.User.by_openid(session, 'jflory7.id.fedoraproject.org'), 'member')

- hub.subscribe(session, hubs.models.User.by_openid(session, 'bee2502.id.fedoraproject.org'), 'member')

- hub.subscribe(session, hubs.models.User.by_openid(session, 'keekri.id.fedoraproject.org'), 'member')

- hub.subscribe(session, hubs.models.User.by_openid(session, 'linuxmodder.id.fedoraproject.org'), 'member')

+ hub.subscribe(session, hubs.models.User.by_username(session, 'decause'), 'owner')

+ hub.subscribe(session, hubs.models.User.by_username(session, 'jflory7'), 'member')

+ hub.subscribe(session, hubs.models.User.by_username(session, 'bee2502'), 'member')

+ hub.subscribe(session, hubs.models.User.by_username(session, 'keekri'), 'member')

+ hub.subscribe(session, hubs.models.User.by_username(session, 'linuxmodder'), 'member')

  

  

  session.commit()
@@ -147,12 +146,12 @@ 

  

  

  # Set up some memberships

- hub.subscribe(session, hubs.models.User.by_openid(session, 'croberts.id.fedoraproject.org'), 'owner')

- hub.subscribe(session, hubs.models.User.by_openid(session, 'ryanlerch.id.fedoraproject.org'), 'owner')

- hub.subscribe(session, hubs.models.User.by_openid(session, 'mrichard.id.fedoraproject.org'), 'member')

- hub.subscribe(session, hubs.models.User.by_openid(session, 'mattdm.id.fedoraproject.org'), 'member')

- hub.subscribe(session, hubs.models.User.by_openid(session, 'decause.id.fedoraproject.org'), 'member')

- hub.subscribe(session, hubs.models.User.by_openid(session, 'ralph.id.fedoraproject.org'), 'subscriber')

+ hub.subscribe(session, hubs.models.User.by_username(session, 'croberts'), 'owner')

+ hub.subscribe(session, hubs.models.User.by_username(session, 'ryanlerch'), 'owner')

+ hub.subscribe(session, hubs.models.User.by_username(session, 'mrichard'), 'member')

+ hub.subscribe(session, hubs.models.User.by_username(session, 'mattdm'), 'member')

+ hub.subscribe(session, hubs.models.User.by_username(session, 'decause'), 'member')

+ hub.subscribe(session, hubs.models.User.by_username(session, 'ralph'), 'subscriber')

  

  

  session.commit()
@@ -193,13 +192,13 @@ 

  

  

  # Set up some memberships

- hub.subscribe(session, hubs.models.User.by_openid(session, 'duffy.id.fedoraproject.org'), 'owner')

- hub.subscribe(session, hubs.models.User.by_openid(session, 'ryanlerch.id.fedoraproject.org'), 'owner')

- hub.subscribe(session, hubs.models.User.by_openid(session, 'gnokii.id.fedoraproject.org'), 'owner')

- hub.subscribe(session, hubs.models.User.by_openid(session, 'mrichard.id.fedoraproject.org'), 'owner')

- hub.subscribe(session, hubs.models.User.by_openid(session, 'nask0.id.fedoraproject.org'), 'member')

- hub.subscribe(session, hubs.models.User.by_openid(session, 'decause.id.fedoraproject.org'), 'member')

- hub.subscribe(session, hubs.models.User.by_openid(session, 'ralph.id.fedoraproject.org'), 'subscriber')

+ hub.subscribe(session, hubs.models.User.by_username(session, 'duffy'), 'owner')

+ hub.subscribe(session, hubs.models.User.by_username(session, 'ryanlerch'), 'owner')

+ hub.subscribe(session, hubs.models.User.by_username(session, 'gnokii'), 'owner')

+ hub.subscribe(session, hubs.models.User.by_username(session, 'mrichard'), 'owner')

+ hub.subscribe(session, hubs.models.User.by_username(session, 'nask0'), 'member')

+ hub.subscribe(session, hubs.models.User.by_username(session, 'decause'), 'member')

+ hub.subscribe(session, hubs.models.User.by_username(session, 'ralph'), 'subscriber')

  

  session.commit()

  
@@ -239,10 +238,10 @@ 

  hub.widgets.append(widget)

  

  

- hub.subscribe(session, hubs.models.User.by_openid(session, 'ralph.id.fedoraproject.org'), 'owner')

- hub.subscribe(session, hubs.models.User.by_openid(session, 'abompard.id.fedoraproject.org'), 'owner')

- hub.subscribe(session, hubs.models.User.by_openid(session, 'lmacken.id.fedoraproject.org'), 'owner')

- hub.subscribe(session, hubs.models.User.by_openid(session, 'nask0.id.fedoraproject.org'), 'member')

- hub.subscribe(session, hubs.models.User.by_openid(session, 'decause.id.fedoraproject.org'), 'subscriber')

+ hub.subscribe(session, hubs.models.User.by_username(session, 'ralph'), 'owner')

+ hub.subscribe(session, hubs.models.User.by_username(session, 'abompard'), 'owner')

+ hub.subscribe(session, hubs.models.User.by_username(session, 'lmacken'), 'owner')

+ hub.subscribe(session, hubs.models.User.by_username(session, 'nask0'), 'member')

+ hub.subscribe(session, hubs.models.User.by_username(session, 'decause'), 'subscriber')

  

  session.commit()

file modified
+1 -1
@@ -4,7 +4,7 @@ 

  fedmsg

  fedmsg_meta_fedora_infrastructure

  flask

- flask-openid

+ flask-oidc

  fmn.lib

  fmn.rules

  gunicorn

file modified
+3 -2
@@ -10,13 +10,14 @@ 

      '--port', '-p', default=5000,

      help='Port for Hubs to run on.')

  parser.add_argument(

-     '--host', default="127.0.0.1",

+     '--host', default="localhost",

      help='Hostname to listen on. When set to 0.0.0.0 the server is available \

-           externally. Defaults to 127.0.0.1 making the it only visible on \

+           externally. Defaults to localhost making the it only visible on \

            localhost')

  

  args = parser.parse_args()

  

  from hubs.app import app

  

+ app.debug = True

Oooh, tried to sneak in another change! I see you! :-)

  app.run(debug=True, host=args.host, port=int(args.port))

file added
+3
@@ -0,0 +1,3 @@ 

+ #!/bin/bash

+ HUBS_CONFIG=`pwd`/hubs/tests/hubs_test.cfg PYTHONPATH=. ./nosetests \

+     --with-coverage --cover-erase --cover-package=hubs $*

no initial comment

This should probably be True, and then only mention in the documentation that for local development, you want this False.

Shouldn't this be by_username?

They should, thanks

4 new commits added

  • Adjust default value regarding HTTPS for cookie
  • Change the host to localhost since 127.0.0.1 won't work with oidc
  • Document how to registed the oidc server in the README
  • Replace the dependency on flask-openid by the one of flask-oidc
7 years ago

1 new commit added

  • Replace calls to by_openid to calls to by_username thanks @puiterwijk
7 years ago

test_url.scheme == ref_url.scheme?

Note that using flask.session means you should make really sure that session uses https-only cookies in any production context.

We could but that might give problems with proxies and all no?

Not if you use the flask proxy support: that should use x-forwarded-scheme to get https for it's flask.request.host_url.

Perhaps name this oidc = OpenIDConnect instead of FAS = ?

A permanent session? That sounds like a bad idea to me...

I am not sure the gain is worth the potential pain

I thought about this, maybe more openidc to avoid conflict with the module

Sure. My point was mostly "not FAS", since it's not tied to FAS (in general..)

1 new commit added

  • Rename FAS to OIDC and fix the test to flask.session.auth
7 years ago

Perhaps fall back to "sub" if nickname is None?
Since there is no guarantee in general that OIDC gives you a (stable) nickname, and this would otherwise break horribly with other (mapped) IdP's.

This is just a workaround for a bug in flask where it doesn't run before_request.

(Which is going to break your setting of flask.g.auth as well).

I still consider this more a bug in flask-oidc than flask and even if it's not the OS we target will likely not be updated to a newer flask any time soon

You're checking for flask.session.fas_user, and then setting flask.session.auth?

Well, it's Flask that's not executing the before_request...

And flask-oidc assuming having something in g while it may not be true :)

Maybe fall back to using sub if there's no nickname field?
There is no guarantee in the general case with IdPs mapped otherwise that the nickname field is there or stable.

Perhaps instead just have runserver.py set this to False?
Since runserver itself is meant for development setups.

It is inserting it on every request before_request. The flask documentation says: "Registers a function to run before each request.". Mark the "Each request" part.

I'd say "return hubs.models.... is not None" is more readable.

Is there a ticket upstream I could follow?

I'm more thinking on improving the runserver to allow specifying a configuration file to use for dev (and which would allow setting this configuration key to False while keeping the default correct). Sounds good?

What would be the correct one?

A permanent session? That sounds like a bad idea to me...

It's needed to be able to specify the duration of the session (like we do in
pkgdb, pagure, fedocal, election... :))

So.... this is because you actively clear all before_requests in tests/init.py: APP.before_request_funcs[None] = []

I'm fine with this but could this ever happen in our setup?

Not in our setup (unless someone, probably me, messed up a config change), but if someone deploys this code outside of our infrastructure, or if someone copies the auth code from here, they can't depend on it.

Ok, I'll adjust although I doubt hubs ever get deployed elsewhere than our infra and having this assumption may actually bring some odd logic in our code for something that's unlikely to ever happen

4 new commits added

  • Fall back to sub if there is no nickname provided by the auth system
  • Fix typo found by @puiterwijk
  • Make the username validator's code a little easier to read
  • Fix issue with Feed elements not rendering
7 years ago

1 new commit added

  • Make the app in debug mode earlier since it's not an argument of run()
7 years ago

1 new commit added

  • Add a runtest.sh script to run the tests properly with the right config
7 years ago

1 new commit added

  • Adjust the instructions on how to run the tests
7 years ago

Not quite sure tbh, it is odd indeed

Just verified that all 11 widgets belong to the user ralph. This is fine.

00 = {Widget} <Widget feed /ralph/31>

01 = {Widget} <Widget meetings /ralph/32>

02 = {Widget} <Widget fedmsgstats /ralph/33>

03 = {Widget} <Widget workflow.updates2stable /ralph/34>

04 = {Widget} <Widget workflow.pendingacls /ralph/35>

05 = {Widget} <Widget subscriptions /ralph/36>

06 = {Widget} <Widget badges /ralph/37>

07 = {Widget} <Widget pagure_pr /ralph/38>

08 = {Widget} <Widget github_pr /ralph/39>

09 = {Widget} <Widget bugzilla /ralph/40>

10 = {Widget} <Widget about /ralph/51>

Oooh, tried to sneak in another change! I see you! :-)

Thanks for the review, let's merge :)

Pull-Request has been merged by pingou

7 years ago

So... despite the confusion, :thumbsup: :-)