|
|
Mads Kiilerich
|
5a148717d392
|
10 years ago
|
|
auth: let login helper function return exception to raise instead of raising it self
Make the execution flow more obvious by raising the exception where it matters.
Avoid redundant and potentially misleading return statement that tried to make it clear that execution wouldn't continue after the function call.
|
|
|
Søren Løvborg
|
23a86f1c33a1
|
10 years ago
|
|
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.
|
|
|
Søren Løvborg
|
c64c076b96c3
|
10 years ago
|
|
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.)
|
|
|
Søren Løvborg
|
95bc1801d480
|
10 years ago
|
|
auth: inline AuthUser.set_authenticated
This makes the following commits easier to follow, and makes it more explicit that something weird is going on, with more cleanup needed.
|
|
|
Søren Løvborg
|
ba30adf2fb8a
|
10 years ago
|
|
auth: introduce AuthUser.is_default_user attribute
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".)
|
|
|
Søren Løvborg
|
d9b78d8f1db3
|
10 years ago
|
|
cleanup: replace redirect with WebOb exceptions
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).
|
|
|
Søren Løvborg
|
1fc8d7e9f3ab
|
10 years ago
|
|
cleanup: replace abort with WebOb exceptions
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.)
|
|
|
Mads Kiilerich
|
ef392737c203
|
10 years ago
|
|
auth: validate that the token protecting from CSRF attacks never is leaked
This will partly give some protection if it should happen, partly make sure the leak doesn't go unnoticed but is found so it can be fixed.
|
|
|
Søren Løvborg
|
b537babcf966
|
10 years ago
|
|
login: include query parameters in came_from
The login controller uses the came_from query argument to determine the page to continue to after login.
Previously, came_from specified only the URL path (obtained using h.url.current), and any URL query parameters were passed along as separate (additional) URL query parameters; to obtain the final redirect target, h.url was used to combine came_from with the request.GET.
As of this changeset, came_from specifies both the URL path and query string (obtained using request.path_qs), which means that came_from can be used directly as the redirect target (as always, WebOb handles the task of expanding the server relative path to a fully qualified URL). The mangling of request.GET can also be removed.
The login code appended arbitrary, user-supplied query parameters to URLs by calling the Routes URLGenerator (h.url) with user-supplied keyword arguments. This construct is unfortunate, since url only appends _unknown_ keyword arguments as query parameters, and the parameter names could overlap with known keyword arguments, possibly affecting the generated URL in various ways. This changeset removes this usage from the login code, but other instances remain.
(In practice, the damage is apparently limited to causing an Internal Server Error when going to e.g. "/_admin/login?host=foo", since WebOb returns Unicode strings and URLGenerator only allows byte strings for these keyword arguments.)
|
|
|
Søren Løvborg
|
431689d7f37d
|
10 years ago
|
|
remove vestiges of Python 2.5 support
We only support Python 2.6 and 2.7; hence we do not need to import with-statement support from __future__.
|
|
|
Søren Løvborg
|
d402d1e4aed4
|
10 years ago
|
|
security: HTTP method sanity checks
This serves to document and verify some implicit constraints on the HTTP method.
|
|
|
Søren Løvborg
|
3598e2a4e051
|
10 years ago
|
|
auth: remove redundant is_authenticated check
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.
|
|
|
Søren Løvborg
|
a041321d2aa1
|
10 years ago
|
|
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.
|
|
|
Mads Kiilerich
|
cc157dcedba6
|
10 years ago
|
|
|
|
|
Mads Kiilerich
|
0210d0b769d4
|
10 years ago
|
|
|
|
|
Søren Løvborg
|
b580691553f5
|
10 years ago
|
|
auth: turn dead AuthUser code into assertion
The result of db.User.get_dict never contains the keys 'api_keys' or 'permissions'. The keys returned by get_dict are 1) all the User table columns, 2) the keys explicitly defined in User.__json__, and 3) the keys defined in User.get_api_data, none of which include the two blacklisted keys.
'api_keys' would be returned if __json__ called get_api_data with argument details=True; but currently that is not the case.
In case there's a reason why these two keys must never appear in an AuthUser object, the check has not been removed entirely; instead, it's been turned into an assertion. This way, it will be noticed if __json__ is later modified to request detailed API data, for instance.
|
|
|
Søren Løvborg
|
fd80edc4aa20
|
10 years ago
|
|
auth: move UserModel.fill_data to AuthUser
Because the method is only used by AuthUser, and also because it's an awful thing to do (copying a large but ill-defined set of attributes from one object to another), and we don't want usage to spread.
|
|
|
Søren Løvborg
|
7557da2252a3
|
10 years ago
|
|
auth: construct AuthUser from either user_id or db.User object
If the caller already has the database User object, there's no reason for AuthUser to look it up again.
The `api_key` lookup functionality is dropped, because 1) it's only used in one place, and 2) it's simple enough for the caller to do the lookup itself.
The `user_id` lookup functionality is kept, because 1) it's frequently used, and 2) far from a simple `User.get(id)` lookup, it has a complex interaction with UserModel. (That cleanup will have to wait for another day.)
All calls of the form `AuthUser(user_id=x.user_id)` can be replaced with `AuthUser(dbuser=x)`, assuming `x` is a db.User. However, verifying that assumption requires a manual audit of every call site, since `x` might also be another `AuthUser` object, for instance. Therefore, only the most obvious call sites have been fixed here.
|
|
|
Søren Løvborg
|
fd3e1ca9cce9
|
10 years ago
|
|
auth: fold AuthUser._propagate_data into constructor
This simplifies both the program and data flow.
|
|
|
Søren Løvborg
|
7e8d80882865
|
10 years ago
|
|
auth: refactor user lookup in AuthUser constructor for clarity
First, note that `fill_data` checks that the specified `db.User` is `active` before copying anything, and returns False if not.
Now, previously when calling e.g. `AuthUser(user_id=anonymous_user_id)`, `_propagate_data` would explicitly refuse to look up the anonymous user, but then fall back to the anonymous user anyway (if `active`), or use None values (if not `active`).
Given the same situation, the new code simply looks up the anonymous user like it would any other user, and copies data using `fill_data`. If the anonymous user is not `active`, we fall back to the existing code path and behave as before (that is, use None values).
|
|
|
Søren Løvborg
|
81d8affd08f4
|
10 years ago
|
|
auth: remove username from AuthUser session cookie
There's no reason to store the username when we store the user ID. We have load the user from database anyway under all circumstances, to verify e.g. that the user is (still) active.
This does not impact application code, but does impact a number of test cases which explicitly checks the username stored in the session.
|
|
|
Søren Løvborg
|
789c98a9306d
|
10 years ago
|
|
auth: remove username lookup support from AuthUser constructor
The previous changeset means that there are no more callers using the `username` argument to AuthUser.__init__; it can be removed.
|
|
|
Søren Løvborg
|
b92c2e4324b0
|
10 years ago
|
|
auth: remove redundant AuthUser constructor arguments
AuthUser looks up the user based on the first of `user_id`, `api_key` and `username` that is not None *and* does not refer to the default (anonymous) user. In practice, the arguments always refer to the same user, so there's no point in specifying more than one of them.
|
|
|
Søren Løvborg
|
837fc99ca140
|
10 years ago
|
|
auth: have fill_data take User object, not lookup key
Simplify the UserModel.fill_data interface by passing the actual db.User object instead of a key by which it can be looked up. This also saves a lookup in case the db.User is already loaded (which for now is only the case in one place, but that'll change in the following changesets).
Also enhance fill_data docstring.
|
|
|
Søren Løvborg
|
102bbfe8af43
|
10 years ago
|
|
auth: remove redundant hashlib imports
hashlib is already imported at the top of the module (regardless of OS).
|
|
|
Søren Løvborg
|
71c2b7054e55
|
10 years ago
|
|
auth: make internal AuthUser methods private
These methods are never invoked from outside AuthUser; make this explicit in the code.
Also remove '_instance' member, which is not referenced anywhere else in the code.
|
|
|
Søren Løvborg
|
49f656a0ccdd
|
10 years ago
|
|
|
|
|
Mads Kiilerich
|
87aebcc15dc2
|
10 years ago
|
|
|
|
|
Mads Kiilerich
|
f43dc1913984
|
10 years ago
|
|
|
|
|
Søren Løvborg
|
f103b1a2383b
|
10 years ago
|
|
BaseController: hide "Log out" link for external login sessions
If user is authorized by external means (API key or container auth), Kallithea is not actually able to log the user out and should not show the "Log out" link.
|
|
|
Søren Løvborg
|
8394211b1c32
|
10 years ago
|
|
AuthUser: refactor AuthUser cookie/session serialization
* All serialization/deserialization logic is now in one place.
* CookieStoreWrapper is gone; any login sessions established in 2012 or earlier will be terminated and users will have to log in again.
* The term "cookie store" has been dropped in favor of simply "cookie" (the value is not a cookie "store" or data repository, but just the actual cookie/session serialization of AuthUser).
Note: from_cookie_store was never used.
|
|
|
Søren Løvborg
|
ad89cd5a6e1a
|
10 years ago
|
|
|
|
|
Søren Løvborg
|
815bf70a88ce
|
10 years ago
|
|
AuthUser: simplify check_ip_allowed and drop is_ip_allowed
check_ip_allowed is always called with user_id and inherit_from_default arguments taken from the same User/AuthUser object, so just take that object instead. This simplifies the is_ip_allowed method to the point where it can be removed.
|
|
|
Søren Løvborg
|
a38e328db172
|
10 years ago
|
|
AuthUser: make get_perms method private
The get_perms method is used exactly once, by the permissions property.
A future, more thorough examination of the code may reveal if it should stay or go; for now, just make it private. Also, enhance docstring accuracy.
|
|
|
Mads Kiilerich
|
148360f533a4
|
10 years ago
|
|
|
|
|
Mads Kiilerich
|
9d87b8d5ba00
|
10 years ago
|
|
auth: ignore permissions from in-active user groups (Issue #138)
Tests by Thomas De Schampheleire.
Additionally, rename the unused and seemingly search-replace-massacred function revokehas_permrevoke_permgrant_perm_perm into revoke_perm.
|
|
|
Mads Kiilerich
|
3e81e6534cad
|
10 years ago
|
|
auth: make random password generator more random
Use the secure os.urandom instead of the pseudo-random 'random' module.
|
|
|
Andrew Shadura
|
7eb5bbbfb8dd
|
11 years ago
|
|
|
|
|
Søren Løvborg
|
4a2a66bf93c5
|
10 years ago
|
|
AuthUser: Drop ip_addr field
None of the AuthUser consumers actually need to get the IP address from the AuthUser object, so it's just redundant.
Also, AuthUser represents a user session, and should not be used as a generic user + IP address data structure.
|
|
|
Thomas De Schampheleire
|
4c5c59b96adc
|
10 years ago
|
|
login: preserve GET arguments throughout login redirection (issue #104)
When redirecting a user to the login page and while handling this login and redirecting to the original page, the GET arguments passed to the original URL are lost through the login redirection process.
For example, when creating a pull request for a specific revision from the repository changelog, there are rev_start and rev_end arguments passed in the URL. Through the login redirection, they are lost.
Fix the issue by passing along the GET arguments to the login page, in the login form action, and when redirecting back to the original page. Tests are added to cover these cases, including tests with unicode GET arguments (in URL encoding).
|
|
|
Andrew Shadura
|
45725b774525
|
11 years ago
|
|
|
|
|
Thomas De Schampheleire
|
12e6de3d7e29
|
11 years ago
|
|
|
|
|
Mads Kiilerich
|
9e36b8bf73b7
|
11 years ago
|
|
auth: avoid flash message with 'None' on login redirect Introduced in 4cad3a52e0ed.
|
|
|
Mads Kiilerich
|
e3aab61a9411
|
11 years ago
|
|
|
|
|
Thomas De Schampheleire
|
e04106e46d6f
|
11 years ago
|
|
auth: return early in LoginRequired on API key validation
Simplify the logic in the LoginRequired decorator when Kallithea is accessed using an API key. Either: - the key is valid and API access is allowed for the accessed method (continue), or - the key is invalid (redirect to login page), or - the accessed method does not allow API access (403 Forbidden)
In none of these cases does it make sense to continue checking for user authentication, so return early.
|
|
|
Thomas De Schampheleire
|
4cad3a52e0ed
|
11 years ago
|
|
auth: return early in LoginRequired on invalid IP
Simplify the code of the LoginRequired decorator by returning early when an unacceptable condition is met.
Note: the 'return' of redirect_to_login() is not strictly needed since we should not return from that function (redirection occurs). Adding it, however, is a security measure in case redirect_to_login does not do what it should do.
|
|
|
Mads Kiilerich
|
ae947de541d5
|
11 years ago
|
|
auth: check CSRF protection token when authenticating
Use pylons secure_form to get CSRF protection on all authenticated POSTs. This fixes CVE-2015-0276.
GETs should not have any side effects and do thus not need CSRF protection.
Reported by Paul van Empelen.
|
|
|
Thomas De Schampheleire
|
dabdc356393b
|
11 years ago
|
|
|
|
|
Thomas De Schampheleire
|
923037eb67d4
|
11 years ago
|
|
spelling: fix various typos
This commit fixes various typos or basic English grammar mistakes found by reviewing the kallithea.pot file.
Full correction of sentences that are not very well formulated, like missing articles, is out of scope for this commit. Likewise for inconsistent capitalization of strings like 'Repository group'/'Repository Group'.
|
|
|
Mads Kiilerich
|
cc1ab5ef6686
|
11 years ago
|
|
cleanup: avoid some 'except Exception' catching - catch specific exceptions or log it and show what happened
This has a risk of introducing regressions ... but we want to get rid of all exception muting and make the whole system less fragile and easier to debug.
|
|
|
Mads Kiilerich
|
d51a6f5e57d1
|
11 years ago
|
|
|
|
|
Mads Kiilerich
|
5376baf8cd6b
|
11 years ago
|
|
|
|
|
Bradley M. Kuhn
|
24c0d584ba86
|
11 years ago
|
|
|
|
|
Bradley M. Kuhn
|
1948ede028ef
|
11 years ago
|
|
|
|
|
Bradley M. Kuhn
|
ad38f9f93b3b
|
11 years ago
|
|
Correct licensing information in individual files.
The top-level license file is now LICENSE.md.
Also, in various places where there should have been joint copyright holders listed, a single copyright holder was listed. It does not appear easy to add a link to a large list of copyright holders in these places, so it simply refers to the fact that various authors hold copyright.
In future, if an easy method is discovered to link to a list from those places, we should do so.
Finally, text is added to LICENSE.md to point to where the full list of copyright holders is, and that Kallithea as a whole is GPLv3'd.
|
|
|
Bradley M. Kuhn
|
05fe69812197
|
11 years ago
|
|
|
|
|
Bradley M. Kuhn
|
d208416c84c6
|
11 years ago
|
|
|
|
|
Bradley M. Kuhn
|
d1addaf7a91e
|
11 years ago
|
|
Second step in two-part process to rename directories. This is the actual directory rename.
|