PR#234: don't verify users' bugzilla accounts - fedora-qa/blockerbugs

fedora-qa / blockerbugs

#234 don't verify users' bugzilla accounts

Merged 2 years ago by kparal. Opened 2 years ago by kparal.

feature/link-bugzilla into develop

don't verify users' bugzilla accounts

Kamil Páral • 2 years ago

c7b4486

blockerbugs/controllers/forms.py

file modified

+4 -9

		`@@ -21,8 +21,8 @@`

		`from flask_wtf import FlaskForm`
		`from wtforms import StringField, SelectField, BooleanField, IntegerField`
		`- from wtforms import TextAreaField, PasswordField, ValidationError`
		`- from wtforms.validators import DataRequired, Email`
		`+ from wtforms import TextAreaField, ValidationError`
		`+ from wtforms.validators import DataRequired`


		`def one_proposal(form, field):`
		`@@ -31,14 +31,9 @@`


		`class BugProposeForm(FlaskForm):`
		`- bugid = IntegerField(u'Bug ID', [DataRequired()])`
		`- bz_user = StringField(u'Bugzilla Login', [DataRequired(), Email()])`
		`+ bugid = IntegerField(u'Bug number', [DataRequired()])`
		`+ fas_login = StringField(u'Fedora account', [DataRequired()])`
		`milestone = SelectField(u'Milestone', [DataRequired()], coerce=int)`
		`blocker = BooleanField(u'Blocker', [one_proposal])`
		`freeze_exception = BooleanField(u'Freeze Exception', [one_proposal])`
		`justification = TextAreaField(u'Justification', [DataRequired()])`
		`-`
		`-`
		`- class FasBugzillaForm(FlaskForm):`
		`- bz_user = StringField(u'Login', [DataRequired(), Email()])`
		`- bz_pass = PasswordField(u'Password', [DataRequired()])`

blockerbugs/controllers/main.py

file modified

+15 -53

		`@@ -19,22 +19,20 @@`

		`"""Interface for the main page"""`

		`- from flask import Blueprint, render_template, redirect, url_for, abort, g, flash, make_response, request`
		`+ from flask import Blueprint, render_template, redirect, url_for, abort, g, make_response, request`
		`import datetime`
		`- from sqlalchemy import func, desc, or_`
		`- import bugzilla`
		`+ 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, __version__`
		`from blockerbugs.util import bz_interface, pagure_bot`
		`from blockerbugs.models.bug import Bug`
		`from blockerbugs.models.milestone import Milestone`
		`from blockerbugs.models.update import Update`
		`from blockerbugs.models.release import Release`
		`- from blockerbugs.models.userinfo import UserInfo`
		`- from blockerbugs.controllers.forms import BugProposeForm, FasBugzillaForm`
		`+ from blockerbugs.controllers.forms import BugProposeForm`

		`main = Blueprint('main', __name__)`

		`@@ -414,46 +412,9 @@`
		`return bz_interface.create_bugzilla()`


		`- @main.route('/fas_bugzilla', methods=['GET', 'POST'])`
		`- @fas_login_required`
		`- def fas_bugzilla():`
		`- fas_bz_form = FasBugzillaForm()`
		`-`
		`- if fas_bz_form.validate_on_submit():`
		`- try:`
		`- # try logging into bz if user/pass are valid`
		`- bz = bugzilla.RHBugzilla4(url=app.config['BUGZILLA_URL'],`
		`- user=fas_bz_form.bz_user.data,`
		`- password=fas_bz_form.bz_pass.data,`
		`- use_creds=False,`
		`- force_xmlrpc=True)`
		`- bz.disconnect()`
		`-`
		`- #create new user_info if user with given FAS-login does not exist`
		`- #if user_with given FAS-login exists just update user_info.bz_user`
		`- user_info = UserInfo.query.filter_by(fas_login=g.fas_user.username).first()`
		`- if user_info:`
		`- user_info.bz_user = fas_bz_form.bz_user.data`
		`- else:`
		`- user_info = UserInfo(g.fas_user.username, fas_bz_form.bz_user.data)`
		`- db.session.add(user_info)`
		`- db.session.commit()`
		`- return redirect(url_for('.propose_bug'))`
		`- except bugzilla.BugzillaError as e:`
		`- app.logger.info(e)`
		`- flash(e)`
		`-`
		`- return render_template('fas_bugzilla.html', title='Link your FAS and Red Hat Bugzilla Accounts',`
		`- fas_bz_form=fas_bz_form)`
		`-`
		`-`
		`@main.route('/propose_bug', methods=['GET', 'POST'])`
		`@fas_login_required`
		`def propose_bug():`
		`- user_info = UserInfo.query.filter_by(fas_login=g.fas_user.username).first()`
		`- if not user_info or not user_info.bz_user:`
		`- return redirect(url_for('.fas_bugzilla'))`
		`-`
		`current_milestone = get_current_milestone()`
		`bugform = BugProposeForm()`
		`bugform.milestone.choices = [(m.id, m.name) for m in Milestone.query.filter_by(active=True).order_by(Milestone.id).all()]`
		`@@ -461,11 +422,11 @@`

		`# 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, bz_user = user_info.bz_user)`
		`+ bugform.process(request.form, fas_login=g.fas_user.username)`

		`if bugform.validate_on_submit():`
		`app.logger.debug('bugid: %i' % bugform.bugid.data)`
		`- app.logger.debug('bz_user: %s' % user_info.bz_user)`
		`+ app.logger.debug('fas_login: %s' % bugform.fas_login.data)`
		`app.logger.debug('milestone: %i' % bugform.milestone.data)`
		`app.logger.debug('blocker: %s' % bugform.blocker.data)`
		`app.logger.debug('freeze_exception: %s' % bugform.freeze_exception.data)`
		`@@ -482,34 +443,35 @@`
		`# The form does basic datatype validation but we need to check for valid`
		`# data`
		`bz = get_bugzilla()`
		`- proposer = bz_interface.BlockerProposal(bz, bugid, trackers, bugform.blocker.data,`
		`- bugform.freeze_exception.data, user_info.bz_user)`
		`+ proposal = bz_interface.BlockerProposal(bz, bugid, trackers, bugform.blocker.data,`
		`+ bugform.freeze_exception.data)`

		`# check bug to make sure it exists and that it isn't closed`
		`try:`
		`- proposer.check_proposed_bug()`
		`+ proposal.check_proposed_bug()`
		`except bz_interface.BZInterfaceError as e:`
		`bugform.bugid.errors = [e.msg]`

		`# check to make sure that any proposals aren't being re-done`
		`if bugform.blocker.data:`
		`app.logger.debug('checking for valid blocker proposal')`
		`- if not proposer.check_blocker_proposal():`
		`+ if not proposal.check_blocker_proposal():`
		`bugform.blocker.errors = ['Bug %i is already proposed as a blocker' % bugid]`
		`if bugform.freeze_exception.data:`
		`app.logger.debug('checking for valid freeze exception proposal')`
		`- if not proposer.check_fe_proposal():`
		`+ if not proposal.check_fe_proposal():`
		`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`
		`app.logger.info('%s proposing %i as %s for %s' % (`
		`- user, bugid, proposer.get_tracker_type(), selected_milestone.name))`
		`+ user, bugid, proposal.get_tracker_type(), selected_milestone.name))`
		`try:`
		`- proposer.propose_bugs(user, selected_milestone.name, bugform.justification.data)`
		`+ proposal.propose_bugs(user, selected_milestone.name, bugform.justification.data)`
		`return render_template('thanks.html', bugid=bugid,`
		`isblocker=bugform.blocker.data,`
		`- isfe=bugform.freeze_exception.data)`
		`+ isfe=bugform.freeze_exception.data,`
		`+ bz_url=app.config['BUGZILLA_URL'])`
		`except bz_interface.BZInterfaceError as e:`
		`bugform.bugid.errors = [e.msg]`
		`app.logger.info('bug proposal form errors: %s' % e.msg)`

blockerbugs/models/userinfo.py

file modified

		`@@ -25,6 +25,9 @@`

		`from blockerbugs import db, BaseModel`

		`+ # FIXME: This is not needed anymore after https://pagure.io/fedora-qa/blockerbugs/issue/230`
		`+ # It also supports storing and checking API keys, but we don't use it anywhere.`
		`+ # Get rid of this table?`

		`class UserInfo(BaseModel):`
		`id = db.Column(db.Integer, primary_key=True)`

blockerbugs/templates/fas_bugzilla.html

file removed

-80

		`@@ -1,80 +0,0 @@`
		`- {% extends "layout.html" %}`
		`-`
		`- {% block jsheader %}`
		`- {% endblock %}`
		`-`
		`- {% block info %}`
		`- {% endblock %}`
		`-`
		`- {% block navigation %}`
		`- {% endblock %}`
		`-`
		`- {% block body %}`
		`- <div class="row">`
		`- <div class="col-auto">`
		`- <h2>Link your FAS and Red Hat Bugzilla Accounts</h2>`
		`- </div>`
		`- </div>`
		`- <div class="row justify-content-center">`
		`- <div class="col-auto" style="padding: 10px;"id="login">`
		`- <form method="post" action="{{ url_for('main.fas_bugzilla') }}">`
		`- {{ fas_bz_form.csrf_token }}`
		`- <fieldset id="bugzilla-credentials">`
		`- <legend>Bugzilla Credentials</legend>`
		`- <table>`
		`- <tr>`
		`- <th scope="col"> {{ fas_bz_form.bz_user.label }}</th>`
		`- <td> {{ fas_bz_form.bz_user(size=20) }}</td>`
		`- </tr>`
		`- {% if fas_bz_form.bz_user.errors %}`
		`- <tr>`
		`- <td></td>`
		`- <td>`
		`- {% if fas_bz_form.bz_user.errors %}`
		`- <div class="alert-danger alert" role="alert">`
		`- <ul>`
		`- {% for error in fas_bz_form.bz_user.errors %}`
		`- <li>{{ error }}</li>`
		`- {% endfor %}`
		`- </ul>`
		`- </div>`
		`- {% endif %}`
		`- </td>`
		`- </tr>`
		`- {% endif %}`
		`- <tr>`
		`- <th scope="col">{{ fas_bz_form.bz_pass.label }}</th>`
		`- <td>{{ fas_bz_form.bz_pass(size=20) }}</td>`
		`- </tr>`
		`- {% if fas_bz_form.bz_pass.errors %}`
		`- <tr>`
		`- <td></td>`
		`- <td>`
		`- {% if fas_bz_form.bz_pass.errors %}`
		`- <div class="alert-danger alert" role="alert">`
		`- <ul>`
		`- {% for error in fas_bz_form.bz_pass.errors %}`
		`- <li>{{ error }}</li>`
		`- {% endfor %}`
		`- </ul>`
		`- </div>`
		`- {% endif %}`
		`- </td>`
		`- </tr>`
		`- {% endif %}`
		`- <tr>`
		`- <td>`
		`- <input type="submit" value="Submit"/>`
		`- </td>`
		`- </tr>`
		`- </table>`
		`- </fieldset>`
		`- </form>`
		`- </div>`
		`- </div>`
		`- <div class="row">`
		`- <div class="col-md-12">`
		`- Your Red Hat bugzilla password is used only to verify that you control the account which will be linked to your FAS account. Your password will <b>NOT</b> be stored anywhere in this application.`
		`- </div>`
		`- </div>`
		`- {% endblock %}`

blockerbugs/templates/propose_bug.html

file modified

+11 -4

		`@@ -27,6 +27,17 @@`

		`<form method="POST" action="{{ url_for('main.propose_bug') }}">`
		`{{ bugform.csrf_token }}`
		`+`
		`+ <fieldset>`
		`+ <legend>Your identification</legend>`
		`+ <div class="row">`
		`+ <div class="form-group col-md-3">`
		`+ <label for="fas_login">{{ bugform.fas_login.label.text }}</label>`
		`+ {{ bugform.fas_login(readonly=True, class_="form-control") }} {{ form_error(bugform.fas_login) }}`
		`+ </div>`
		`+ </div>`
		`+ </fieldset>`
		`+`
		`<fieldset>`
		`<legend>Blocker Information</legend>`
		`<div class="row">`
		`@@ -34,10 +45,6 @@`
		`<label for="bugid">{{ bugform.bugid.label.text }}</label>`
		`{{ bugform.bugid(class_="form-control") }} {{ form_error(bugform.bugid) }}`
		`</div>`
		`- <div class="form-group col-md-3">`
		`- <label for="bz_user">{{ bugform.bz_user.label.text }}</label>`
		`- {{ bugform.bz_user(readonly=True, class_="form-control") }} {{ form_error(bugform.bz_user) }}`
		`- </div>`
		`<div class="form-group col-md-2">`
		`<label for="milestone">{{ bugform.milestone.label.text }}</label>`
		`<br /> {{ bugform.milestone(class_="form-control") }} {{ form_error(bugform.milestone) }}`

blockerbugs/templates/thanks.html

file modified

+24 -14

		`@@ -1,23 +1,19 @@`
		`{% extends "base_nav.html" %}`

		`{% macro blockertype(blocker, fe) -%}`
		`- {%- if blocker -%}`
		`- blocker`
		`- {%- endif %}`
		`- {%- if fe %}`
		`- {%- if blocker %}`
		`- and freeze exception`
		`- {%- else -%}`
		`- freeze exception`
		`- {%- endif %}`
		`- {%- endif %}`
		`+ {%- if blocker and fe -%}`
		`+ blocker and a freeze exception`
		`+ {%- elif blocker -%}`
		`+ blocker`
		`+ {%- elif fe -%}`
		`+ freeze exception`
		`+ {%- endif %}`
		`{%- endmacro %}`

		`{% block jsheader %}`
		`{% endblock %}`

		`{% block info %}`
		`-`
		`{% endblock %}`

		`{% block body %}`
		`@@ -25,11 +21,25 @@`
		`<div class="row">`
		`<div class="col-md-12" id="index">`
		`<h2>Proposal Complete</h2>`
		`+ <p></p>`
		`<p>`
		`- Thank you for proposing {{ bugid }} as a {{ blockertype(isblocker, isfe) }}.`
		`- Bug {{ bugid }} has been updated but the changes may not show up on`
		`- the bug list until the next sync operation (every 30 minutes).`
		`+ <b>Thank you for proposing`
		`+ <a href="{{ bz_url }}/show_bug.cgi?id={{ bugid }}" target="_blank" rel="noopener noreferrer">bug {{ bugid }}</a>`
		`+ as a {{ blockertype(isblocker, isfe) }}.</b> This bug has been updated in Bugzilla, but the`
		`+ changes may not show up here in the BlockerBugs application until the next sync operation (every`
		`+ 30 minutes). Once the bug is visible here, there will be also a link to a proposal review`
		`+ discussion, where you can participate.`
		`</p>`
		`+ <div class="alert-info alert">`
		`+ <b>IMPORTANT:</b> Please subscribe to that bug (if you're not already) to receive updates on the`
		`+ problem and also on the proposal decision.<br/>`
		`+ <button id="howto-toggle" type="button" class="btn btn-link" data-toggle="collapse" data-target="#howto">How to do that?</button>`
		`+ <div id="howto" class="collapse">`
		`+ Open <a href="{{ bz_url }}/show_bug.cgi?id={{ bugid }}" target="_blank" rel="noopener noreferrer">bug {{ bugid }}</a>,`
		`+ log in, and make sure you're listed in the <i>CC List</i> field. If you're not, click <i>Add me to`
		`+ CC list</i>, click <i>This is a minor update (do not send email)</i> and click <i>Save Changes</i>.`
		`+ </div>`
		`+ </div>`
		`</div>`
		`</div>`

blockerbugs/util/bz_interface.py

file modified

+6 -7

		`@@ -198,13 +198,12 @@`


		`class BlockerProposal():`
		`- def __init__(self, bz, bugid, trackers, is_blocker=False, is_fe=False, bz_user=''):`
		`+ def __init__(self, bz, bugid, trackers, is_blocker=False, is_fe=False):`
		`self.bz = bz`
		`self.bugid = bugid`
		`self.trackers = trackers`
		`self.is_blocker = is_blocker`
		`self.is_fe = is_fe`
		`- self.bz_user = bz_user`
		`self.bugid_ok = False`
		`self.proposal_ok = False`
		`self.bugdata = None`
		`@@ -232,9 +231,9 @@`
		`if self.is_prioritized:`
		`return 'PrioritizedBug'`

		`- def propose_bugs(self, bz_user, milestone_name, justification):`
		`+ def propose_bugs(self, proposer, milestone_name, justification, cc_add=None):`
		`comment = ['Proposed as a', self.get_tracker_type(), 'for', milestone_name, 'by',`
		`- bz_user, 'using the blocker tracking app because:\n\n',`
		`+ proposer, 'using the blocker tracking app because:\n\n',`
		`justification]`
		`tracker_bugs = []`
		`if self.is_blocker:`
		`@@ -243,7 +242,7 @@`
		`tracker_bugs.append(self.trackers['fe'])`
		`self.log.info('comment: %s' % comment)`
		`try:`
		`- self._do_proposal(tracker_bugs, self.bugid, ' '.join(comment), self.bz_user)`
		`+ self._do_proposal(tracker_bugs, self.bugid, ' '.join(comment), cc_add)`
		`except Fault as e:`
		`if e.faultCode == 51:`
		`# bugzilla account does not exist, this should happen very rarely, if ever`
		`@@ -253,9 +252,9 @@`
		`else:`
		`raise BZInterfaceError(e.faultString)`

		`- def _do_proposal(self, tracker, proposed_bugid, comment, bz_user):`
		`+ def _do_proposal(self, tracker, proposed_bugid, comment, cc_add):`
		`bug_update = self.bz.build_update(blocks_add=tracker,`
		`- cc_add=[bz_user],`
		`+ cc_add=cc_add or [],`
		`comment=comment)`
		`self.bz.update_bugs(proposed_bugid, bug_update)`

testing/test_bugchange.py

file modified

+4 -4

		`@@ -53,9 +53,9 @@`
		`stubbz.build_update.return_value = bz_success`

		`test_bz = BlockerProposal(stubbz, self.ref_proposed, self.ref_trackers,`
		`- is_blocker=True, bz_user=self.ref_cc)`
		`+ is_blocker=True)`
		`test_bz.propose_bugs(self.ref_user, self.ref_milestone,`
		`- self.ref_justification)`
		`+ self.ref_justification, cc_add=self.ref_cc)`

		`test_cc = stubbz.build_update.mock_calls[0][2]['cc_add']`

		`@@ -101,11 +101,11 @@`
		`stubbz.update_bugs = mock.Mock(side_effect=Fault(50, 'not 51 exception'))`

		`test_bz = BlockerProposal(stubbz, self.ref_proposed, self.ref_trackers,`
		`- is_blocker = True, bz_user = self.ref_cc)`
		`+ is_blocker=True)`

		`with pytest.raises(BZInterfaceError) as excinfo:`
		`test_bz.propose_bugs(self.ref_user, self.ref_milestone,`
		`- self.ref_justification)`
		`+ self.ref_justification, cc_add=self.ref_cc)`

		`assert excinfo.value.msg == 'not 51 exception'`

kparal commented 2 years ago

After Bugzilla auth changes [1] we can no longer test users' logins and passwords
for validity. Because we already have the user logged in through FAS, which
combats spam, the easiest solution is to simply drop the bugzilla login
verification process. The downside is that the user needs to manually CC himself
to that bug (if not CCed already).

This patch drops the verification process and adjusts the form templates so that
the user is advised to CC himself to the bug.

[1] https://listman.redhat.com/archives/bugzilla-announce-list/2022-February/msg00000.html