Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUG token authentication is accepted on endpoints which only allow 'session' as authentication-method #791

Closed
N247S opened this issue May 3, 2023 · 5 comments · Fixed by #813
Assignees
Labels

Comments

@N247S
Copy link
Contributor

N247S commented May 3, 2023

I am messing around a bit with configurations to figure out how things work, and what to be aware of when working with flask-security.

One thing I noticed was when one authenticates using a token, they can access endpoints which are protected with auth_required('session') (i.e. session-authentication only), which might not be a big problem, but goes against specs if I read it correctly.

Testcase:

def test_token_only_auth(app, client):
	app.post('/token')
	def token_login_mock():
		if request.is_json:
			d = request.get_json()

			if not d['email']:
				abort(HTTPStatus.BAD_REQUEST)
			user = lookup_identity(d['email'])
			if user and not user.active:
				abort(HTTPStatus.NOT_FOUND)

			if user and user.verify_and_update_password(d['password']):
				token = user.get_auth_token()
				return {'token': token}
			else:
				abort(HTTPStatus.UNAUTHORIZED)
	
	app.get('/protected')
	@auth_required('session')
	def protected():
		return 'secret'

	response = client.post('/token', data=auth_data, content_type='application/json')
	token = response.json['token']
	response = client.get('/protected', headers={'Authentication-Token': token})
	assert response.status_code == 401 # fails as session is created before auth-mechanism check

From what I could figure out the loginManager.request_loader is called prior to the auth_required. Upon a request it tries to create a session and populates it during which process the current_user proxy is called wich calls _get_user() which in turn calls current_app.login_manager._load_user() to get the user, which in turn calls _load_user_from_request() which is able to obtain the user from a token. This is pushed to the global-variables (g) and is used to populate the current_user.

After this chain of events is done, the auth_required decorator is called which for session-authentication checks current_user.is_authenticated, which is true because that user was found using the token already.
This means the decorator basically allows token authentication to be used for session authentication.

Again not sure if this is a big problem, but I think it goes against specs.

The version I tested this on is:
Flask 2.2.3
Flask-security-too 5.1.2 (fresh install as of 1 week ago, so I assume latest dependency versions)

If more information is needed, feel free to ask.

@jwag956 jwag956 added the bug label May 3, 2023
@jwag956
Copy link
Member

jwag956 commented May 3, 2023

Thanks for the detailed issue - seems like a bug and I can see a reasonable use case to have some endpoints be session/web only (quite a few sites require web/session access to say retrieve access tokens).

@N247S
Copy link
Contributor Author

N247S commented May 3, 2023

Hadn't thought that far into usecases yet, but seems reasonable.

One quickfix I could think of is by checking the fs_authn_via flag inside the auth_mechanism function for 'session authentication' as I dont think that flag is set when a user is restored from session data. But it feels sketchy at best.

With the growing authentication solutions it might be preferable to fix this with a big rework. But the above could serve as a quick/temp fix.

@jwag956 jwag956 self-assigned this Jul 8, 2023
@jwag956
Copy link
Member

jwag956 commented Jul 8, 2023

Thanks for your work - I have taken your PR and am working with it - hopefully get something up in the next few days.
Right now I am leaning towards the fs_authn_via solution....

@N247S
Copy link
Contributor Author

N247S commented Jul 8, 2023

Thank you for picking it up.
Just to make sure, PR#794 should be a fix for this.
Might save you some time.

@jwag956
Copy link
Member

jwag956 commented Jul 8, 2023

Yup - started with that - a big help.

jwag956 added a commit that referenced this issue Jul 8, 2023
If an endpoint was decorated with "session" only - a properly submitted token would also be accepted.
Fix that by checking as part of the auth_required() decorator and the user is authenticated AND was authenticated using the _user_loader (which is what flask-login calls for session based authenticated).

close #791
jwag956 added a commit that referenced this issue Jul 8, 2023
If an endpoint was decorated with "session" only - a properly submitted token would also be accepted.
Fix that by checking as part of the auth_required() decorator and the user is authenticated AND was authenticated using the _user_loader (which is what flask-login calls for session based authenticated).

close #791
jwag956 added a commit that referenced this issue Jul 9, 2023
* Update test_common.py

Added testcase for failing toke-authentication on session-only endpoint

* Update conftest.py

Added session-only authenticated route to test-fixture

* Update decorators.py

Added the `_check_session` function to specifically check session data to be used as authentication_method in the `auth_required`

* Update decorators.py

* Update decorators.py

* fixed decorator and added tests

* Fix session-only authentication.

If an endpoint was decorated with "session" only - a properly submitted token would also be accepted.
Fix that by checking as part of the auth_required() decorator and the user is authenticated AND was authenticated using the _user_loader (which is what flask-login calls for session based authenticated).

close #791

---------

Co-authored-by: N247S <fictiefverzonnen@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants