Enh/add auth

[MBueschelberger]

Description:

This PR shall represent a draft for adding a potential authentication layer based on fastapi.security to the FastApi-app.

For this procedure, the OTEAPI_AUTH_DEPS-env can be enabled, which underlies a similar syntax like the OTEAPI_PLUGIN_PACKAGES-variable. The major difference is that the single modules are referencing to the individual class of the Python-package-regex: my_package.my_module.my_class.

Of course, one has to be careful hiding the OTEAPI_AUTH_DEPS-variable to the outside world, since it may lead to similar security concerns such as the OTEAPI_PLUGIN_PACKAGES-env.

An example of the overall variable my look like that:
my_module.my_class | my_other_module.my_other_class

When this example variable will be parsed during start-up of the app, my_class and my_other_class will be imported and hence result in:

FastApi(dependencies=[Depends(my_class()), Depends(my_other_class))]

This gives the flexibility that a developer can come up with its own authentication scheme.

Moreover, my_class or my_class might be a childclass of the common classes from the fastapi.security-module such as HTTPBasic or HTTPBearer and can provide a callback-function by overriding the __call__-method:

MyHttpAuth(HTTPBearer):
        async def __call__(self, request: Request) -> str:
                auth = await super().__call__(request=request)
                my_callback_function(auth.credentials)
                return auth.credentials

The token from the auth.credentials may then be further used for authentication to OntoRec etc.

I am keen towards your feedback and suggestions for improvement!

Type of change:

• Bug fix.
• New feature.
• Documentation update.

Checklist for the reviewer:

This checklist should be used as a help for the reviewer.

• Is the change limited to one issue?
• Does this PR close the issue?
• Is the code easy to read and understand?
• Do all new feature have an accompanying new test?
• Has the documentation been updated as necessary?

[CasperWA]

Some suggested changes here, but I really like this idea of passing in the authentication classes/functions in this way.

[MBueschelberger]

@CasperWA :

I added a unit test for checking the retrieval of the token from the configs or the request headers and then through the cache again.

While running the pytests in docker_compose_dev.yml manually via cli in the container, all tests exit successfully. However, the SecretStr seems to be censored to "******" only during the docker-build. Is it maybe related that the DummyCache in the pytests are using different json_encoders for SecretStr in different environments?

We probably need to specify the json_encoders in the pydantic models for the strategies which are using SecretStr. This is releated to the fact, that the secret is censored once the config.json() is called in order to dump it into the cache (no matter if DummyCache or redis backend). Therefore, the secret-attribute will be useless, if it shall be used by any related strategy afterwards.

An example for the encoding might look like this:

# taken from https://docs.pydantic.dev/usage/types/#secret-types


class SimpleModelDumpable(BaseModel):
    password: SecretStr
    password_bytes: SecretBytes

    class Config:
        json_encoders = {
            SecretStr: lambda v: v.get_secret_value() if v else None,
            SecretBytes: lambda v: v.get_secret_value() if v else None,
        }

In order to prevent the revealing of secrets in the cache through the services, I made the exposure of the redis-cache endpoint optional, since I think this might not be desired in various production scenarios as explained in the AppSettings here.

[MBueschelberger]

@CasperWA, I think all threads are resolved. As soon as this PR is merged, we can update this line in the requirements.txt and the branch would be ready to be merged as well here.

[CasperWA]

@CasperWA, I think all threads are resolved. As soon as this PR is merged, we can update this line in the requirements.txt and the branch would be ready to be merged as well here.

It's a reality! You can update to minimum v0.3.0, i.e.: ~=0.3.0.

[CasperWA]

Hmm, I can't immediately penetrate the test error messages. Do you mind if I take a stab at it locally and commit a fix, if I find one? Otherwise I'll just let you know the issue and you could commit :)

Edit: I see the issue now. The tests simply needs to be updated to use the new SecretConfig gig you setup. I think that's easier for you to fix, no? :)

[MBueschelberger]

Hi @CasperWA,

I can reproduce locally. But I am puzzled what is the difference to the previous dev-version of the SecretConfig and the current one from oteapi-core==0.0.3, since the pytest did not fail before. I will try yo investigate

[MBueschelberger]

@CasperWA, I have figured out the issue.

The route in the redisadmin was doing a type check while returning the data from the DummyCache. In my examplem I use the redis-endpoint in order to test whether the token/SecretStr has been properly retrieved and serialized through the config or through the request-headers.

Nevertheless, the DummyCache should also return a str, because the sessions/configs are serialized via the json-method in the routers of each single strategy (see json.dumps in the test_data for the pytest). This leads us to the opportunity, that we can neglect the json.dumps in the get-method of the DummyCache and the conversion from JSON-str to a dict works properly via json.loads in all routers.

[codecov-commenter]

Codecov Report

Base: 68.62% // Head: 69.15% // Increases project coverage by +0.53% 🎉

Coverage data is based on head (cc619bd) compared to base (7d02f7c).
Patch coverage: 79.48% of modified lines in pull request are covered.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #189      +/-   ##
==========================================
+ Coverage   68.62%   69.15%   +0.53%     
==========================================
  Files          18       18              
  Lines         494      522      +28     
==========================================
+ Hits          339      361      +22     
- Misses        155      161       +6     

Flag	Coverage Δ
pytest	69.15% <79.48%> (+0.53%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
app/routers/dataresource.py	27.53% <25.00%> (-0.83%)	⬇️
app/routers/redisadmin.py	91.66% <80.00%> (+2.77%)	⬆️
app/main.py	78.18% <81.81%> (+0.40%)	⬆️
app/routers/function.py	81.25% <100.00%> (+0.81%)	⬆️
app/routers/transformation.py	69.11% <100.00%> (+0.93%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[CasperWA]

Should there be a similar test for dataresource as well?

Also, I cleaned up the conftest file according to my comments above. I hope this is fine?

[CasperWA]

Wait. So by default you don't want to include the Redis admin endpoint (for security reasons) but do want to expose secrets? :/

[MBueschelberger]

Well, the redis admin is not exposed because the secrets are exposed.

If the secret strings for the oteapi.models are not exposed, they cannot be read in the strategies anymore, since they are censored during serialization. This pretty much makes them useless if you want to use them for connecting to an external host.

If they are exposed and the redis admin is enabled, you may simply reveal all user's in a configuration through the redis endpoint.

For this reason, the chance, that you may remotely inspect the redis-cache from other's users sessions, is lowered since you only can access the redis-cache from the strategies, and not directly through the services anymore.

In other words, the motivation is that when you put sensitive/general information into the cache, you cannot reveal it by chance if you only got the config-id, but it still can be used by the strategies internally.

Since I did not find any straight-forward user management for redis through an oauth-scheme, I think this is one of the only ways how to deal with this scenario for the moment.

Does this make sense to you?

[CasperWA]

This makes sense. Essentially what we need is to hash/encrypt the secrets before storing in redis. That should solve it. The strategies can then get some interface to decrypting the secrets they may need. Or it's done from the service before invoking the strategies. Not very important, but it should circumvent this issue.

However that's something for another day and another PR.

[CasperWA]

@MBueschelberger Could you comment on the last part, Then this can be merged and I can look at the OTELib PR that depends on this.

[CasperWA]

@MBueschelberger merge at will :)