files: treat messages about 'File too big' as sentences and add a dot.
Make it more easy to read: File is too big to display Show full annotation anyway by treating them as two sentences: File is too big to display. Show full annotation anyway.
Similar messages about 'Changeset too big to display' in the changeset and pullrequest code are wrapped in an <h4> tag. Follow the same style when showing files or annotations.
files: support annotation on files larger than cut_off_limit
When requesting the annotation for a file larger than the cut_off_limit configured in the ini file, the only current option is to click the useless 'show as raw' (which is not an annotation).
Replace it with a link 'Show full annotation anyway' instead.
auth: note that we never emit authuser "cookies" for the default user
The only place where we set "authuser" in the session is in log_in_user, which is called only by the internal auth system and by auth plugins. The internal auth system cannot log a user in as the default user, because the default user doesn't have a password (and cannot have a password assigned). Auth plugins cannot log a user in as the default user, because the user doesn't have the right extern_type. As such, it's a bug if log_in_user is ever called with the default user (which this commit documents with an assert).
This realization makes the is_authenticated field of the authuser cookie redundant, as it's always True. It also emphasizes that is_default_user and is_authenticated are mutually exclusive.
auth: avoid setting AuthUser.is_authenticated for unauthenticated users
AuthUser.is_authenticated could be True for three reasons: because the user "was" the default user, because the user was authenticated by session cookie, or because the user was just authenticated by an auth module (including the internal auth module). In the last case, a session cookie is emitted (even when using container auth), so the last two cases are closely related.
This commit do that unauthenticated users (the first case) only get the is_default_user attribute set, and that the is_authenticated attribute only is set for authenticated users (for the second and third case).
This complicates some expressions, but allows others to be simplified. More importantly, it makes the code more explicit, and makes the "is_authenticated" name mean what it says.
(This will temporarily make the is_authenticated session value look even more weird than before.)
This makes makes a number of checks more readable.
The username of the default user is presently hardcoded to "default" (in db.User.DEFAULT_USER); this is currently what defines the default user, and this commit doesn't change that. (Even if the check that defines is_default_user is a comparison between user IDs and not usernames, the anonymous_user object used in the comparison is loaded by looking up the user named "default".)
All redirect does is to log "Generating 302 redirect" with logging the actual location and raise a WebOb HTTPFound exception, and the logging is redundant, as WebOb exceptions and their status codes are already logged.
Instead, just raise the exception directly, which is both explicit and simpler (and finally, gets rid of "return redirect" which never really returns).
All abort does is to look up the matching WebOb exception and raising that; so just raise it directly. WebOb exception names are also more readable than HTTP error codes. (And finally, don't "return abort", since abort never returns.)
notifications: mark notifications to self "pre-read"
When a user e.g. comments on its own pull request, that user receives a notification about its own comment. This is slightly dubious behavior, but at least brings a level of continuity to the notification history.
However, at the very least, the notification should not show as unread.
cache: when invalidating a cache, always just delete all 'live cache' records instead of marking them inactive
Keep it simple. Adding the record again might be slightly more expensive than just updating the active flag but instead we get get a simpler model and automatic cleanup without using the cache-keys paster command.
It turns out the user.is_authenticated check is redundant, since it's True for both anonymous users and logged in users, and API key users are handled prior to the check.
security: apply CSRF check to all non-GET requests
The automatic CSRF protection was broken for POST requests with no request payload parameters (but possibly containing request URI parameters); a security hole was narrowly avoided because the code base quite consistently checks the request method in the same way, and because of browser protection against PUT/DELETE CSRF attacks.
Since explicit is better than implicit, the better way of checking the HTTP request method is to simply check request.method, instead of checking if request.POST is non-empty, which is subtly different (it doesn't catch POST requests if all parameters are in the query string) and non-obvious (because it also applies to PUT requests).
The commit also fixes some tests which relied on the CSRF protection being broken. It does not fix all the controllers that still does the misleading request.POST check, but since the CSRF check has now been tightened, those are no longer a potential security issue.
pull requests: remove immediate invocation of the function defined in pullrequest_data.html
pullrequest_data.html behaved differently when included and when used as namespace. Keep it simple and just let it define the pullrequest_overview function.
pull requests: don't filter on repo when finding comments for a PR
Filtering on repo while finding comments for a PR would miss some. Fix that.
The existing data model is inconsistent; PRs already have two repos and programming errors could easily make the one on a comment wrong. (Revision comments do however need the repo reference.)
auth: use HMAC-SHA1 to calculate password reset token
The use of standard cryptographic primitives is always preferable, and in this case allows us not to worry about length extension attacks and possibly any number of issues that I'm not presently aware of.
This is a better implementation of password reset function, which doesn't involve sending a new password to the user's email address in clear text, and at the same time is stateless.
The old implementation generated a new password and sent it in clear text to whatever email assigned to the user currently, so that any user, possibly unauthenticated, could request a reset for any username or email. Apart from potential insecurity, this made it possible for anyone to disrupt users' workflow by repeatedly resetting their passwords.
The idea behind this implementation is to generate an authentication token which is dependent on the user state at the time before the password change takes place, so the token is one-time and can't be reused, and also to bind the token to the browser session.
The token is calculated as SHA1 hash of the following:
* user's identifier (number, not a name) * timestamp * hashed user's password * session identifier * per-application secret
We use numeric user's identifier, as it's fixed and doesn't change, so renaming users doesn't affect the mechanism. Timestamp is added to make it possible to limit the token's validness (currently hard coded to 24h), and we don't want users to be able to fake that field easily. Hashed user's password is needed to prevent using the token again once the password has been changed. Session identifier is an additional security measure to ensure someone else stealing the token can't use it. Finally, per-application secret is just another way to make it harder for an attacker to guess all values in an attempt to generate a valid token.
When the token is generated, an anonymous user is directed to a confirmation page where the timestamp and the usernames are already preloaded, so the user needs to specify the token. User can either click the link in the email if it's really them reading it, or to type the token manually.
Using the right token in the same session as it was requested directs the user to a password change form, where the user is supposed to specify a new password (twice, of course). Upon completing the form (which is POSTed) the password change happens and a notification mail is sent.
The test is updated to test the basic functionality with a bad and a good token, but it doesn't (yet) cover all code paths.
The original work from Andrew has been thorougly reviewed and heavily modified by Søren Løvborg.
email: send comment and pullrequest mails with the author's name in 'From'
When emails are sent for comments and pullrequest invitations, set the From header to: Author's Name (no-reply) <generic email address>
Using the name of the person that causes the email, makes the emails more useful and interpretable for the recipient of the emails. To avoid replies directly to the author, triggering an 'offline' email discussion that is not visible in the Kallithea interface, a generic 'no-reply' email address is used instead of the author's email address. This approach is assumed to be accepted by spam filters, as several other web services are using the same approach.
The sender used for other email types, e.g. password reset mails, is untouched and remains the value configured in app_email_from.
The sender used for the SMTP envelope is untouched as well.
Show an extra checkbox next to the first selected checkbox to be able to specify single revision ranges (which is different from the start of an open range).
When two revisions are selected, hide all other checkboxes to make it impossible to select more.
auth: avoid random auth_internal failures - add explicit import of auth_internal to user admin
auth_internal would often have been loaded by the custom auth module loader and available as auth_modules.auth_internal ... but sometimes it wasn't and navigating to Add User would fail with:
File '.../kallithea/controllers/admin/users.py', line 155 in new c.default_extern_type = auth_modules.auth_internal.KallitheaAuthPlugin.name AttributeError: 'module' object has no attribute 'auth_internal'
users: fix missing c.readonly in UsersController.update error rendering
The c.readonly attribute, while correctly set in UsersControllers.edit, was not assigned in UsersControllers.update, causing a template error if the form validation failed. This commit unifies template rendering in separate method to fix this and avoid future problems.
pullrequests: remove reviewer list during PR creation
There is not much use for it before the actual diff is shown ... and removing it also removes a bit of duplicated code that otherwise should be maintained in two places.