Enable OAuth2 authentication.

[ojecborec]

I've created the new branch to enable reviewing changes for #602.

[netlify]

✅ Deploy Preview for opal-docs canceled.

Name	Link
🔨 Latest commit	a7a9404
🔍 Latest deploy log	https://app.netlify.com/sites/opal-docs/deploys/674de438aa9d4a00095ebad5

[roekatz]

Hi @ojecborec,

Thanks again for the help & contribution.

1. Can you explain what is the use case / use cases that this PR aims to enable? Why have you found it needed? (Is it having opal-server authenticate opal-client using a third party identity provider?).
2. Please fix the pre-commit checks and the failing tests.
3. This new feature requires additional tests (probably for DataUpdater/PolicyUpdater).

[ojecborec]

Following PR is enabling OAuth2 third party authentication for both server and client. Client would send access token generated by Client Credentials grant and server would validate this token by either calling introspect endpoint or reading JWT signature. I'll have a look at pre-commit checks and failing tests.

[ojecborec]

If I understand correctly some pre-commit checks are failing on code like this

-        callbacks_router = init_callbacks_api(self.authenticator, self._callbacks_register)
+        callbacks_router = init_callbacks_api(
+            self.authenticator, self._callbacks_register
+        )

Why is callbacks_router = init_callbacks_api(self.authenticator, self._callbacks_register) being rejected? I see a lot of code like this all over the place.

[ojecborec]

Can you help me understand how is this test failure related to code I'm submitting (I'm not a Python developer) ?

    @pytest.mark.asyncio
    async def test_data_updater_with_report_callback(server):
        ...
    
        try:
            ...
    
            proc2 = multiprocessing.Process(target=trigger_update_patch, daemon=True)
            proc2.start()
            ...
    
        # cleanup
        finally:
            await updater.stop()
            proc.terminate()
>           proc2.terminate()
E           UnboundLocalError: local variable 'proc2' referenced before assignment

[roekatz]

@ojecborec I'll help with Python you'll help with OAuth2 :) (As I'm not an expert, although willing to make myself more familiar of course).

I still don't understand what's the missing use case for OPAL (from user's high level perspective) - Do you want opal-client to authenticate to opal-server using a specific Google account? (OAuth2 is usually used for authenticating web applications on behalf of actual people, isn't it?).

Regarding pre-commit - it enforces the "Black" formatting. You can install pre-commit locally (brew install pre-commit if you're on mac) and run it within the repo with pre-commit run --all-files. It would fix the formatting for you (not only show errors). You can also install/enable Black extension in your IDE so files are formatted on save / on some key combo.

Regarding the tests - Yeah seems like the stack trace is misleading. I believe the test raised an exception before proc2 was defined, finally is always executed (exception or not) for clean up but then proc2 isn't defined. I guess the exception has to do with the changes in DataUpdater because it actually tests data updates. Maybe fixing the proc2 issue (define it at the function's head and check if it's not None before calling .terminate()) would reveal the actual exception.
You can run tests locally using:

pip install -r requirements.txt 
pytest

[roekatz]

@ojecborec Apologies - I see now similar failures on other PRs, so those probably don't have to do with you changes.
I'll take care of fixing them in master

[roekatz]

@ojecborec - You can rebase onto master as tests should be fixed there now

[ojecborec]

I still don't understand what's the missing use case for OPAL (from user's high level perspective) - Do you want opal-client to authenticate to opal-server using a specific Google account? (OAuth2 is usually used for authenticating web applications on behalf of actual people, isn't it?).

Company software usually depends on some kind of authentication. You cannot utilize resources without providing username/password or some kind of token. OPAL server generates JWTs which do the job but often you already have dedicated OAuth2/OpenID service that your existing applications integrate with. This patch allows both OPAL server and client leverage this dedicated OAuth2/OpenID service for either token generation or validation.

[roekatz]

Hi @ojecborec - Here's a few comments on a first look mainly regarding the Authenticator interface.
Should still dive further into reviewing the core implementation

[roekatz]

That's not a great way to achieve polymorphism.
What you probably want is to have an Authenticator interface/abstract class.
Have authenticate & enabled simply raise NotImplementedError() (or use @abc.abstractmethod, although we don't really use it in OPAL's code base).

Than have different classes inherit Authenticator and have those directly implement those methods. (No need to access them through delegation.

[ojecborec]

I've refactored Authenticator as an interface.

[roekatz]

Instead of having this class "proxy" the actual implementation according to the configured AUTH_TYPE - you probably want to use the factory design pattern.
Have both CachedOAuth2Authenticator & _JWTAuthenticator inherit from Authenticator and implement its declared methods.

Then have another class / function (e.g AuthenticatorFactory.create) with return type Authenticator instantiate the right subclass according to configured auth type (or other relevant arguments).

Basically what you're doing here is having a class "deciding what it is" within its constructor, which again doesn't make use of Python's native polymorphism / OOP support.

[ojecborec]

Modified code to use AuthenticatorFactory.

[roekatz]

Methods in this class should have a specified return type, also their arguments should be typed

[ojecborec]

Fixed.

[roekatz]

No need to have it again if it's the same as in the _OAuth2Authenticator super class

[ojecborec]

Fixed.

[roekatz]

List available types in description

[ojecborec]

Amended description.

[roekatz]

Same comments as in _ClientAuthenticator

[ojecborec]

Refactored.

[ojecborec]

Seems like 2 security/snyk (permit) tests have failed but I don't have access to read details.

[ojecborec]

I've updated DataUpdater by making it interface and created DataUpdaterFactory that returns DataUpdater. There are 2 implementations.

The DefaultDataUpdater is backwards compatible however I've extended it to

1. Send Accept: application/json header as we are expecting JSON in return (response.json()) .
2. Changed error_details = await response.json() to error_details = await response.text() as the error response itself might not always be JSON.

The OAuth2DataUpdater where

1. Instead of ClientSession handling 307 redirects let us do it manually as it seems like neither Authorization nor Accept header are being forwarded to redirected URL which results in 401 response. Should we do the same for DefaultDataUpdater updater to forward Accept header when redirecting?
2. Remove token query parameter from redirected URL and send the Authorization header instead.

Branch oauth2-v2 has been rebased to master.

[ojecborec]

@roekatz
Can you look at the changes I've made, please?
What is the reason for security/snyk (permit) — 2 tests have failed? I don't have access.

[ojecborec]

@roekatz Is there anything I can do to get this PR merged?

[ojecborec]

@roekatz have your comments been resolved? Can you let me know why security tests are failing. Thank you.

[ojecborec]

@Abiji-2020 Would you mind checking this PR?

[danyi1212]

Hey @ojecborec, I see this PR has been open for quite some time, and I want to try helping you to close it ASAP.

There are a lot of commits and changes in this PR - some of them are probably unrelated.
Can you please rebase this PR from master, so that only the relevant commits will be included here?

[ojecborec]

Hi @danyi1212. Sorry for not replying earlier. This has been now git rebase master. Let me know if you need anything else. Thank you.

[ojecborec]

Hi @danyi1212. Have you made any progress in reviewing this PR?

[ojecborec]

Hi @danyi1212. Have you had any spare time to review this PR?