Disable authorization when testing

BorjaEst commented 3 years ago

I have an application using Flaat with some resources are controlled by login_required and group_required.

All works perfect, however, it took me some time to undestand how to "Mock", "Patch" or "Pass" the authentication when testing.

After looking into the code, I see there is an ENV read for 'DISABLE_AUTHENTICATION_AND_ASSUME_AUTHENTICATED_USER' which can be used to skip the authentication.

If someone interested, currently I pass the authentication in pytest using:

@fixture(scope='function')
def skip_authorization(monkeypatch):
    """Patch ENV to skip the authorization step."""
    monkeypatch.setenv(
        "DISABLE_AUTHENTICATION_AND_ASSUME_AUTHENTICATED_USER",
        "YES"
    )

However, it might be better to be able to split between different authorization grants, such as:

@fixture(scope='function')
def grant_logged(monkeypatch):
    """Patch fixture to test function as logged user."""
    monkeypatch.setenv("ASSUME_LOGGED", True)

@fixture(scope='function')
def grant_admin(monkeypatch, grant_logged):
    """Patch fixture to test function as admin user."""
    monkeypatch.setenv("ASSUME_GROUPS, ["admin1", "admin2"])

Also it is not on the documentation, but I consider it a really important feature. Is it a feature inteded for usage?

Also note the idea is not to skip the authorization always, but control it at tearUp and tearDown. I still have some tests which return OK only if the reply was 401 UNAUTHORIZED.

marcvs commented 3 years ago

I'd change your request a bit in the sense that :

DISABLE_AUTHENTICATION_AND_ASSUME_AUTHENTICATED_USER=yes: Assumes a valid user
DISABLE_AUTHENTICATION_AND_ASSUME_VALID_GROUPS=yes: Assumes all group membership requriements to be true- - DISABLE_AUTHENTICATION_AND_ASSUME_GROUPS=<json list>: Assumes the user in question to be member of the groups specified.

Would that help?

marcvs commented 3 years ago

It's implemented in the "fix-28" branch, in case you want to take a look

BorjaEst commented 3 years ago

Does "DISABLE_AUTHENTICATION_AND_ASSUME_GROUPS" also assumes authenticated user? Maybe it was not too clear in my comment, but which one of those should be correct?

Case 1

From the documentation it is also noit clear if I should decorate my functions this way:

@group_required(group=["admin1"],claim='eduperson_assurance')
def some_function():
    ...

Then run my tests this way:

def some_function():
    .....
    # Only 1 env variable os enough?
    setenv(
        "DISABLE_AUTHENTICATION_AND_ASSUME_GROUPS", 
        ["admin1", "admin2"]
    )

This is how I have been doing it so far and is based on the "example-flask.py".

Case 2

However, from the documentation and names of the env variables one might think that "login_required" does any special action as well needed for the correct authentication.

@login_required()
@group_required(group=["admin1"],claim='eduperson_assurance')
def some_function():
     ...

Then run my tests this way:

def some_function():
    .....
    # Two env variables are needed?
    setenv(
        "DISABLE_AUTHENTICATION_AND_ASSUME_AUTHENTICATED_USER", 
        "Yes"
    )
    setenv(
        "DISABLE_AUTHENTICATION_AND_ASSUME_GROUPS", 
        ["admin1", "admin2"]
    )

Both cases might work, but the second might be redundant. Once on PyPI I can test it on my application and give you more feedback.

marcvs commented 3 years ago

Does "DISABLE_AUTHENTICATION_AND_ASSUME_GROUPS" also assumes authenticated user? No. It does not.

Maybe it was not too clear in my comment, but which one of those should be correct?

Case 1

From the documentation it is also noit clear if I should decorate my functions this way:
@group_required(group=["admin1"],claim='eduperson_assurance')
def some_function():
    ...
Then run my tests this way:
def some_function():
    .....
    # Only 1 env variable os enough?
    setenv(
        "DISABLE_AUTHENTICATION_AND_ASSUME_GROUPS", 
        ["admin1", "admin2"]
    )
Yes that should workl

This is how I have been doing it so far and is based on the "example-flask.py".

Case 2

However, from the documentation and names of the env variables one might think that "login_required" does any special action as well needed for the correct authentication.
@login_required()
@group_required(group=["admin1"],claim='eduperson_assurance')
def some_function():
     ...
Then run my tests this way:
def some_function():
    .....
    # Two env variables are needed?
    setenv(
        "DISABLE_AUTHENTICATION_AND_ASSUME_AUTHENTICATED_USER", 
        "Yes"
    )
    setenv(
        "DISABLE_AUTHENTICATION_AND_ASSUME_GROUPS", 
        ["admin1", "admin2"]
    )
Both cases might work, but the second might be redundant. Once on PyPI I can test it on my application and give you more feedback.

Yes, also the 2nd test makes sense. This allows to skip / test three different scenarios;

Override (only) the valid user check: DISABLE_AUTHENTICATION_AND_ASSUME_AUTHENTICATED_USER
Override (only) the group membership check: DISABLE_AUTHENTICATION_AND_ASSUME_VALID_GROUPS
Override (only) the group membership check, but assume give list of groups: DISABLE_AUTHENTICATION_AND_ASSUME_VALID_GROUPS

If you have a better IDEA for naming, I'll consider. But: I want to keep it very BULKY, so people see and know that they are deactivating something they should not do.

marcvs commented 3 years ago

It's on pypi as version 0.13.0

alvarolopez commented 3 years ago

I am sorry to enter the conversation here (I am not even a FLAAT developer), but I think it is not a good practice to modify the main code of the application to introduce changes just for testing purposes. Why cannot _get_entitlements_from_claim be mocked for the tests that do not want or need the authentication?

Also, if the original request from @BorjaEst was to simply disable authentication for the unit testing, this should be done at the original code (i.e. mocking the FLAAT decorator?). What would happen if FLAAT changes its internal API and the mocking is not working anymore?

BorjaEst commented 3 years ago

Thank you for the comment @alvarolopez, indeed, I tried to mock the FLAAT decorators @login_required, @groups_required, that is the best approach.

However to mock a decorator is complex and very relevant the moment when you run it, as it is applied at function declaration (not running). Maybe I am wrong, but it would have to be mocked before the "import" which goes against pep8 and other recommendations....

Instead, suggestions in forums are to mock the function the decorator uses to check the authorization. In this case would be:

login_required:
- _find_request_based_on_web_framework
- _get_all_info_from_request
group_required:
- _find_request_based_on_web_framework
- _get_all_info_from_request
- ensure_is_list
- _determine_number_of_required_matches
- _get_entitlements_from_claim

Those are a lot of patches to enable testing. The source code would have to be edited a bit more.

Maybe changing the scopes of def decorated inside the @wraps to the module and call them inside might do the trick.

@marcvs I could give it a try if you and @alvarolopez agree.

BorjaEst commented 3 years ago

It's on pypi as version 0.13.0

Meanwhile, FYI this is working in my application. Although according to @alvarolopez we might have to change it.

marcvs commented 3 years ago

@alvarolopez We had this mechanism in place for quite some time already. The idea was to allow testing by adding two more, and hence changing the first one. The use case came not only from unit-testing, but also from people that wanted to test their shell code wrapped around the actual tool using flaat.

I'd see it as a feature of flaat to allow mocking.

What should be wrong with it?

alvarolopez commented 3 years ago

@BorjaEst you can unwrap a decorated function so that you test only your code (inspect.unwrap) so something like the following might work, but this depends on what you actually want to test.

import inspect                                                                     
import unittest                                                                    

from flaat import Flaat                                                            
import flask                                                                       

app = flask.Flask(__name__)                                                        

flaat = Flaat()                                                                    
flaat.set_web_framework('flask')                                                   

@app.route('/')                                                                    
@flaat.login_required()                                                            
def hello_world():                                                                 
    return 'Hello, World!'                                                         

class Test(unittest.TestCase):                                                     
    def setUp(self):                                                               
        self.app = app.test_client()                                               

def tearDown(self):                                                            
        pass                                                                       

    def test_hello_world(self):                                                    
        response = self.app.get('/', follow_redirects=True)                        
        self.assertEqual(response.status_code, 401)                                

    def test_hello_world_undecorated(self):                                        
        fn = inspect.unwrap(hello_world)                                           
        self.assertEqual("Hello, World!", fn())                                    

if __name__ == "__main__":                                                         
    unittest.main()

alvarolopez commented 3 years ago

@marcvs yes, I see that this is not new. My point is that, modifying the main code of the application just for testing is a bad practice, IMO.

A much more elegant solution is that, depending on the framework, you provide a mocked request object, with the required assertions just for testing. This way, somebody could do:

def do_some_test():
    req = flaat.testing.get_fake_request()
    (... do whatever testing you want using that fake object....)

indigo-dc / flaat