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	283407b
🔍 Latest deploy log	https://app.netlify.com/sites/opal-docs/deploys/6706200482ff1000087735d6

[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'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.