Summary

Add support for custom authentication forms for hosting services.

Review Request #8149 — Created May 6, 2016 and submitted May 9, 2016, 3:36 p.m.

Information

Owner

chipx86

Repository

Review Board

Branch

release-2.0.x

Bugs

Depends On

Blocks

8143

Reviewers

Groups

reviewboard

People

Description

Up until now, we made the assumption that every hosting service just
needed a username and a password (which was sometimes an API key). This
isn't always sufficient, though, and we've worked around this limitation
in the past by taking per-repository fields for additional
credential-related information, which really should have been part of
the account details.

Now, hosting services can define an authentication form. This manages
fields for the hosting URL (if self-hosted), username/password, and
two-factor auth token. Subclasses can override the help text and labels
for these, override the fields completely, or create new fields.

The form is responsible for all validation for authentication
information, the transformation of the fields into a set of credentials
for authorization, and the authorization flow itself. It's also
responsible for state like which of the fields are required or presented
to the user (allowing us to remove some of the hacks in JavaScript).

The main repository configuration form now instantiates one of these
authentication forms for every hosting service. This means that for
every hosting service, the authentication fields are independent,
preventing errors from showing up in an auth form when moving between
hosting services, and preventing other such weird glitches we had with
sharing the auth form.

There's one UI change to how accounts for self-hosted services work.
Previously, the hosting URL field would appear before the accounts
field. You'd type the URL to the service, and the list of accounts
would update to show only those from that service. Now, we show all
accounts across all URLs, each showing the hosting URL its mapped to as
part of the name). The hosting URL is then shown below that (only when
linking an account). This helps make it clear what accounts you have.

The other major change is that re-authenticating now allows you to
change the username and hosting URL, if needed. Previously, this was not
allowed.

All-in-all, this change helps move us to a model where RepositoryForm
doesn't have to do all the work, and we're able to better split up
responsibilities. Similar changes will follow for repository and bug
tracker configuration in the future.

Testing Done

Unit tests pass.

Manually tested the following conditions:

Creating a new repository and account with no errors.
Creating a new repository and account with auth field errors, and then
fixing them.
Creating a new repository and account with two-factor auth code required,
and then entering it.
Creating a new repository and account with self-hosted service.
Creating a new repository with existing account.
Creating a new repository with existing account requiring re-auth, and
providing new credentials (new username, new password, new hosting URL,
new two-factor auth code).
Creating a new repository without a hosting service.
Creating a new repository with a hosting service using a custom auth
form (repeating the above tests).
Creating a new repository with bug tracker configurations (both with
new information and with using the selected hosting service's information).
Editing a repository (with all the above tests)

Issues

Description	From	Last Updated
Col: 80 E501 line too long (82 > 79 characters)	reviewbot	May 6, 2016, 6:45 p.m.
local variable 'e' is assigned to but never used	reviewbot	May 6, 2016, 6:46 p.m.
'forms' imported but unused	reviewbot	May 6, 2016, 6:46 p.m.
redefinition of unused 'test_get_file_exists_with_mercurial' from line 1074	reviewbot	May 6, 2016, 6:46 p.m.
local variable 'account' is assigned to but never used	reviewbot	May 6, 2016, 6:46 p.m.
Typo: password.	brennie	May 6, 2016, 11 p.m.
Can we call it help_text?	brennie	May 6, 2016, 7:59 p.m.
Probably should have import ugettext_lazy as _ in the code example so people know what _ is.	brennie	May 6, 2016, 11:01 p.m.
If this is required, we really should have it not default to None. That way, it doesn't appear to be …	brennie	May 6, 2016, 8 p.m.
Col: 80 E501 line too long (82 > 79 characters)	reviewbot	May 6, 2016, 6:49 p.m.
The comma here is unnecessary.	brennie	May 6, 2016, 8:01 p.m.
Can we use something else as id is a builtin?	brennie	May 9, 2016, 2:03 p.m.
redefinition of unused 'test_get_file_exists_with_mercurial' from line 1073	reviewbot	May 6, 2016, 6:49 p.m.
Out of curiousity, why do we override full_clean instead of just clean?	brennie	May 6, 2016, 8:02 p.m.
Col: 80 E501 line too long (82 > 79 characters)	reviewbot	May 9, 2016, 2:03 p.m.
These don't need to be in the try	brennie	May 9, 2016, 2:52 p.m.
Col: 80 E501 line too long (82 > 79 characters)	reviewbot	May 9, 2016, 3:31 p.m.

Tool: Pyflakes
Processed Files:
    reviewboard/hostingsvcs/models.py
    reviewboard/scmtools/tests.py
    reviewboard/scmtools/admin.py
    reviewboard/scmtools/forms.py
    reviewboard/testing/hosting_services.py
    reviewboard/hostingsvcs/forms.py
    reviewboard/hostingsvcs/service.py
    reviewboard/hostingsvcs/tests.py

Ignored Files:
    reviewboard/static/rb/js/admin/repositoryform.js
    reviewboard/templates/admin/scmtools/repository/change_form.html



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/hostingsvcs/models.py
    reviewboard/scmtools/tests.py
    reviewboard/scmtools/admin.py
    reviewboard/scmtools/forms.py
    reviewboard/testing/hosting_services.py
    reviewboard/hostingsvcs/forms.py
    reviewboard/hostingsvcs/service.py
    reviewboard/hostingsvcs/tests.py

Ignored Files:
    reviewboard/static/rb/js/admin/repositoryform.js
    reviewboard/templates/admin/scmtools/repository/change_form.html

reviewboard/hostingsvcs/forms.py (Diff revision 1)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (82 > 79 characters)
```
reviewboard/hostingsvcs/forms.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 local variable 'e' is assigned to but never used
```
reviewboard/hostingsvcs/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 'forms' imported but unused
```
reviewboard/hostingsvcs/tests.py (Diff revision 1)
The issue has been dropped. Show all issues
```
 redefinition of unused 'test_get_file_exists_with_mercurial' from line 1074
```
reviewboard/scmtools/forms.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 local variable 'account' is assigned to but never used
```

Change Summary:


Fixed some unused variables/imports.
Added some code I forgot to git add.

Diff:

Revision 2 (+1400 -343)

Show changes

	reviewboard/hostingsvcs/forms.py
	reviewboard/hostingsvcs/models.py
	reviewboard/hostingsvcs/service.py
	reviewboard/hostingsvcs/tests.py
	reviewboard/scmtools/admin.py
	reviewboard/scmtools/forms.py
	reviewboard/scmtools/tests.py
	reviewboard/static/rb/js/admin/repositoryform.js
	2 more

Tool: Pyflakes
Processed Files:
    reviewboard/hostingsvcs/models.py
    reviewboard/scmtools/tests.py
    reviewboard/scmtools/admin.py
    reviewboard/scmtools/forms.py
    reviewboard/testing/hosting_services.py
    reviewboard/hostingsvcs/forms.py
    reviewboard/hostingsvcs/service.py
    reviewboard/hostingsvcs/tests.py

Ignored Files:
    reviewboard/static/rb/js/admin/repositoryform.js
    reviewboard/templates/admin/scmtools/repository/change_form.html



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/hostingsvcs/models.py
    reviewboard/scmtools/tests.py
    reviewboard/scmtools/admin.py
    reviewboard/scmtools/forms.py
    reviewboard/testing/hosting_services.py
    reviewboard/hostingsvcs/forms.py
    reviewboard/hostingsvcs/service.py
    reviewboard/hostingsvcs/tests.py

Ignored Files:
    reviewboard/static/rb/js/admin/repositoryform.js
    reviewboard/templates/admin/scmtools/repository/change_form.html

reviewboard/hostingsvcs/forms.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (82 > 79 characters)
```
reviewboard/hostingsvcs/tests.py (Diff revision 2)
The issue has been dropped. Show all issues
```
 redefinition of unused 'test_get_file_exists_with_mercurial' from line 1073
```

reviewboard/hostingsvcs/forms.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Typo: password.
```

reviewboard/hostingsvcs/forms.py (Diff revision 2)

The issue has been dropped. Show all issues

Can we call it help_text?

chipx86 May 6, 2016, 8:03 p.m.

ModelForm supports help_texts, which is the same thing as this. We're just not using ModelForm here. The naming is for consistency.

reviewboard/hostingsvcs/forms.py (Diff revision 2)

The issue has been resolved. Show all issues

Probably should have import ugettext_lazy as _ in the code example so people know what _ is.

reviewboard/hostingsvcs/forms.py (Diff revision 2)

The issue has been dropped. Show all issues

If this is required, we really should have it not default to None. That way, it doesn't appear to be optional at first glance.

chipx86 May 6, 2016, 8:03 p.m.

data must come first as the form parameter, and it's supposed to default to None. That means anything after it must also default to None. It's not great, but it's what we've got :/

reviewboard/hostingsvcs/forms.py (Diff revision 2)

Does this link correctly in docs?

chipx86 May 6, 2016, 8:03 p.m.
```
Yep. Except it should be :meth:.
```

reviewboard/hostingsvcs/forms.py (Diff revision 2)

The issue has been dropped. Show all issues

The comma here is unnecessary.

chipx86 May 6, 2016, 8:03 p.m.

"A string containing the hosting URL or None" means the string may contain either one of those, which isn't correct. This is either returning a string containing the hosting URL, or it's returning None.

reviewboard/hostingsvcs/service.py (Diff revision 2)

The issue has been resolved. Show all issues

Can we use something else as id is a builtin?

chipx86 May 6, 2016, 8:03 p.m.

Yeah. We kind of do a bit of both.. Models have id, for instance. It's just kinda lengthy for hosting service stuff, but I'll switch it over.

reviewboard/scmtools/forms.py (Diff revision 2)

The issue has been dropped. Show all issues

Out of curiousity, why do we override full_clean instead of just clean?

chipx86 May 6, 2016, 8:03 p.m.

There's things we need to do before Django's full_clean runs, like mess with the required values. If we tried to do that in clean, it'd be too late. Same with manipulation of some of the errors and cleaned_data, the way we're doing it.

Change Summary:


Renamed the HostingService.id variable to HostingService.hosting_service_id.
Fixed a typo in some docs.
Fleshed out some example code to include imports.

Diff:

Revision 3 (+1405 -343)

Show changes

	reviewboard/hostingsvcs/forms.py
	reviewboard/hostingsvcs/models.py
	reviewboard/hostingsvcs/service.py
	reviewboard/hostingsvcs/tests.py
	reviewboard/scmtools/admin.py
	reviewboard/scmtools/forms.py
	reviewboard/scmtools/tests.py
	reviewboard/static/rb/js/admin/repositoryform.js
	2 more

Tool: Pyflakes
Processed Files:
    reviewboard/hostingsvcs/models.py
    reviewboard/scmtools/tests.py
    reviewboard/scmtools/admin.py
    reviewboard/scmtools/forms.py
    reviewboard/testing/hosting_services.py
    reviewboard/hostingsvcs/forms.py
    reviewboard/hostingsvcs/service.py
    reviewboard/hostingsvcs/tests.py

Ignored Files:
    reviewboard/static/rb/js/admin/repositoryform.js
    reviewboard/templates/admin/scmtools/repository/change_form.html



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/hostingsvcs/models.py
    reviewboard/scmtools/tests.py
    reviewboard/scmtools/admin.py
    reviewboard/scmtools/forms.py
    reviewboard/testing/hosting_services.py
    reviewboard/hostingsvcs/forms.py
    reviewboard/hostingsvcs/service.py
    reviewboard/hostingsvcs/tests.py

Ignored Files:
    reviewboard/static/rb/js/admin/repositoryform.js
    reviewboard/templates/admin/scmtools/repository/change_form.html

reviewboard/hostingsvcs/forms.py (Diff revision 3)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (82 > 79 characters)
```

Ship it!

reviewboard/hostingsvcs/forms.py (Diff revision 3)
The issue has been resolved. Show all issues
```
These don't need to be in the try
```

Typo in description: "beforet"

Aside from what Barret saw, nothing jumps out at me, but it's a big change so it's hard to see how the pieces fit together.

chipx86 May 9, 2016, 2:59 p.m.

Yeah, there's kind of a lot going on. It's largely because RepositoryForm is such a giant, and I'm hoping to break that down into more manageable pieces with the new repo work.. Are there parts I can try to go over in more detail?

Description:: Up until now, we made the assumption that every hosting service just
needed a username and a password (which was sometimes an API key). This
isn't always sufficient, though, and we've worked around this limitation
in the past by taking per-repository fields for additional
credential-related information, which really should have been part of
the account details.

Now, hosting services can define an authentication form. This manages
fields for the hosting URL (if self-hosted), username/password, and
two-factor auth token. Subclasses can override the help text and labels
for these, override the fields completely, or create new fields.

The form is responsible for all validation for authentication
information, the transformation of the fields into a set of credentials
for authorization, and the authorization flow itself. It's also
responsible for state like which of the fields are required or presented
to the user (allowing us to remove some of the hacks in JavaScript).

The main repository configuration form now instantiates one of these
authentication forms for every hosting service. This means that for
every hosting service, the authentication fields are independent,
preventing errors from showing up in an auth form when moving between
hosting services, and preventing other such weird glitches we had with
sharing the auth form.

There's one UI change to how accounts for self-hosted services work.
~ Previously, the hosting URL field would appear beforet the accounts
~ Previously, the hosting URL field would appear before the accounts
field. You'd type the URL to the service, and the list of accounts
would update to show only those from that service. Now, we show all
accounts across all URLs, each showing the hosting URL its mapped to as
part of the name). The hosting URL is then shown below that (only when
linking an account). This helps make it clear what accounts you have.

The other major change is that re-authenticating now allows you to
change the username and hosting URL, if needed. Previously, this was not
allowed.

All-in-all, this change helps move us to a model where RepositoryForm
doesn't have to do all the work, and we're able to better split up
responsibilities. Similar changes will follow for repository and bug
tracker configuration in the future.

Change Summary:

Moved some statements outside of a try.

Diff:

Revision 4 (+1405 -343)

Show changes

	reviewboard/hostingsvcs/forms.py
	reviewboard/hostingsvcs/models.py
	reviewboard/hostingsvcs/service.py
	reviewboard/hostingsvcs/tests.py
	reviewboard/scmtools/admin.py
	reviewboard/scmtools/forms.py
	reviewboard/scmtools/tests.py
	reviewboard/static/rb/js/admin/repositoryform.js
	2 more

Tool: Pyflakes
Processed Files:
    reviewboard/hostingsvcs/models.py
    reviewboard/scmtools/tests.py
    reviewboard/scmtools/admin.py
    reviewboard/scmtools/forms.py
    reviewboard/testing/hosting_services.py
    reviewboard/hostingsvcs/forms.py
    reviewboard/hostingsvcs/service.py
    reviewboard/hostingsvcs/tests.py

Ignored Files:
    reviewboard/static/rb/js/admin/repositoryform.js
    reviewboard/templates/admin/scmtools/repository/change_form.html



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/hostingsvcs/models.py
    reviewboard/scmtools/tests.py
    reviewboard/scmtools/admin.py
    reviewboard/scmtools/forms.py
    reviewboard/testing/hosting_services.py
    reviewboard/hostingsvcs/forms.py
    reviewboard/hostingsvcs/service.py
    reviewboard/hostingsvcs/tests.py

Ignored Files:
    reviewboard/static/rb/js/admin/repositoryform.js
    reviewboard/templates/admin/scmtools/repository/change_form.html

reviewboard/hostingsvcs/forms.py (Diff revision 4)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (82 > 79 characters)
```

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-2.0.x (51a14da)

		Up until now, we made the assumption that every hosting service just
		needed a username and a password (which was sometimes an API key). This
		isn't always sufficient, though, and we've worked around this limitation
		in the past by taking per-repository fields for additional
		credential-related information, which really should have been part of
		the account details.

		Now, hosting services can define an authentication form. This manages
		fields for the hosting URL (if self-hosted), username/password, and
		two-factor auth token. Subclasses can override the help text and labels
		for these, override the fields completely, or create new fields.

		The form is responsible for all validation for authentication
		information, the transformation of the fields into a set of credentials
		for authorization, and the authorization flow itself. It's also
		responsible for state like which of the fields are required or presented
		to the user (allowing us to remove some of the hacks in JavaScript).

		The main repository configuration form now instantiates one of these
		authentication forms for every hosting service. This means that for
		every hosting service, the authentication fields are independent,
		preventing errors from showing up in an auth form when moving between
		hosting services, and preventing other such weird glitches we had with
		sharing the auth form.

		There's one UI change to how accounts for self-hosted services work.
~		Previously, the hosting URL field would appear beforet the accounts
	~	Previously, the hosting URL field would appear before the accounts
		field. You'd type the URL to the service, and the list of accounts
		would update to show only those from that service. Now, we show all
		accounts across all URLs, each showing the hosting URL its mapped to as
		part of the name). The hosting URL is then shown below that (only when
		linking an account). This helps make it clear what accounts you have.

		The other major change is that re-authenticating now allows you to
		change the username and hosting URL, if needed. Previously, this was not
		allowed.

		All-in-all, this change helps move us to a model where RepositoryForm
		doesn't have to do all the work, and we're able to better split up
		responsibilities. Similar changes will follow for repository and bug
		tracker configuration in the future.