Adding oauthn #690

ZohebShaikh · 2024-10-25T15:37:08Z

Alternative to the authentication were investigated:-

Authomatic
Python Social Auth is an OAuth and OAuth2 client for a multitude of services.
Authlib has built-in OAuth 2 client for Flask and Django.
Requests-OAuthlib has OAuth library support for Python Requests.
mozilla-django-oidc is a lightweight Django authentication and access management library for integration with OpenID Connect enabled authentication services.

In the end I decided to not use them as we just need to make 2 requests and there is not that much error handling required as well ... We can look into integrating on of the above mentioned alternatives for the OAuth device flow integration.

codecov · 2024-10-25T15:52:44Z

Codecov Report

Attention: Patch coverage is 98.38057% with 4 lines in your changes missing coverage. Please review.

Project coverage is 93.10%. Comparing base (33908fd) to head (07c9686).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/blueapi/service/main.py	87.50%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #690      +/-   ##
==========================================
+ Coverage   92.17%   93.10%   +0.92%     
==========================================
  Files          35       36       +1     
  Lines        1803     2029     +226     
==========================================
+ Hits         1662     1889     +227     
+ Misses        141      140       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

tpoliaw

A couple of biggish design level things:

If we're assuming the access tokens are short-lived, single use things, would it make more sense (and probably simplify the code) to request a new token for every request? When a user logs in, only store the refresh token, then for every subsequent request, request a new access token and use that. It means we don't need to do any expiry checks and the rest client doesn't have to aware of any refresh requests.

Secondly, should the client request the authentication details from the server instead of having to provide its own config? It's a pain for a user to have to pass configuration when presumably it doesn't make sense for it to ever be different from the configuration the server is going to use to validate it? We could have a login endpoint that returns the configuration to use. Presumably any future web client would need something similar to initiate to login process.
For the token file location, it could be an optional flag to the CLI defaulting to somewhere like $XDG_CACHE_DIR/blueapi_token.

src/blueapi/client/rest.py

src/blueapi/service/authentication.py

tpoliaw · 2024-11-21T16:58:11Z

tests/unit_tests/client/test_rest.py

+    cached_valid_token: Path,
+):
+    plan = Plan(name="my-plan", model=MyModel)
+    mock_authn_server.stop()  # Cannot use multiple RequestsMock context manager


Can't this be handled by the fixture instead of needing it in every test?

src/blueapi/config.py

src/blueapi/cli/cli.py

src/blueapi/service/authentication.py

tpoliaw · 2024-11-21T18:12:39Z

src/blueapi/service/main.py

+async def on_token_error_401(_: Request, __: Exception):
+    return JSONResponse(
+        status_code=status.HTTP_401_UNAUTHORIZED,
+        content={},


The spec wants this to return a WWW-Authenticate header (see docs). Something like WWW-Authenticate=Bearer realm="blueapi"? I'm not sure what the realm/scopes should be

This specification outlines the expected response from Keycloak when it encounters an invalid token, Not for what a fast-api/any other server how it should handle invalid requests

Pretty sure it refers to services like this

If the protected resource request does not include authentication
credentials or does not contain an access token that enables access
to the protected resource, the resource server MUST include the HTTP
"WWW-Authenticate" response header field

Or from the HTTP status code spec

The 401 (Unauthorized) status code indicates that the request has not been applied because it lacks valid authentication credentials for the target resource. The server generating a 401 response MUST send a WWW-Authenticate header field

All challenges defined by this specification MUST use the auth-scheme
value "Bearer". This scheme MUST be followed by one or more
auth-param values. The auth-param attributes used or defined by this
specification are as follows. Other auth-param attributes MAY be
used as well.

If the protected resource request included an access token and failed
authentication, the resource server SHOULD include the "error"
attribute to provide the client with the reason why the access
request was declined.

3.1. Error Codes

When a request fails, the resource server responds using the appropriate HTTP status code (typically, 400, 401, 403, or 405) and includes one of the following error codes in the response: invalid_request The request is missing a required parameter, includes an unsupported parameter or parameter value, repeats the same parameter, uses more than one method for including an access token, or is otherwise malformed. The resource server SHOULD respond with the HTTP 400 (Bad Request) status code. invalid_token The access token provided is expired, revoked, malformed, or invalid for other reasons. The resource SHOULD respond with the HTTP 401 (Unauthorized) status code. The client MAY request a new access token and retry the protected resource request. insufficient_scope The request requires higher privileges than provided by the access token. The resource server SHOULD respond with the HTTP 403 (Forbidden) status code and MAY include the "scope" attribute with the scope necessary to access the protected resource. If the request lacks any authentication information (e.g., the client was unaware that authentication is necessary or attempted using an unsupported authentication method), the resource server SHOULD NOT include an error code or other error information.

Quotes end:
return JsonResponse(status_code=status.HTTP_401_UNAUTHORIZED, headers={"WWW-Authenticate": "Bearer realm=blueapi"})```

seems a reasonable first attempt. According to the spec we SHOULD check what type of JWTException we get and set the error appropriately too.

ZohebShaikh · 2024-11-22T12:06:54Z

A couple of biggish design level things:

If we're assuming the access tokens are short-lived, single use things, would it make more sense (and probably simplify the code) to request a new token for every request? When a user logs in, only store the refresh token, then for every subsequent request, request a new access token and use that. It means we don't need to do any expiry checks and the rest client doesn't have to aware of any refresh requests.

Secondly, should the client request the authentication details from the server instead of having to provide its own config? It's a pain for a user to have to pass configuration when presumably it doesn't make sense for it to ever be different from the configuration the server is going to use to validate it? We could have a login endpoint that returns the configuration to use. Presumably any future web client would need something similar to initiate to login process. For the token file location, it could be an optional flag to the CLI defaulting to somewhere like $XDG_CACHE_DIR/blueapi_token.

The first suggestion makes sense and seems straightforward to implement.

Regarding point 2, I wanted to add something: the configuration currently includes three pieces of information, all of which are optional. The only critical detail is the token_file_path, which could be moved to the cached directory by default.

Now, when the server has authentication enabled, how should the login feature behave? For example, if a user runs a command like blueapi -c config controller plans and the client doesn’t have an access token, should it prompt the user to log in automatically? Or should it let the request fail with a 404 error and leave it up to the user to realize they need to log in?

tpoliaw · 2024-11-22T12:24:50Z

If the server has auth enabled and the user doesn't pass a valid token, I think returning 401 and letting the user figure it out (via a 'Authentication required' message to make it obvious?) is the easiest option although automatically trying to login could work too as long as there's a way to prevent it so commands can be scripted.

For the first point, I think the auth endpoint would need to be cached as well as the refresh token so that the login details would not need to be queried for every request.

src/blueapi/cli/cli.py

DiamondJoseph · 2024-11-28T10:57:48Z

src/blueapi/cli/cli.py

+        access_token = auth.get_valid_access_token()
+        if not access_token:
+            print("Problem with cached token, starting new session")


Should: this print should be in the except block for the get_valid_access_token, as this path also has the case of there not being a cached token?

then delete_cache also in there

DiamondJoseph · 2024-11-28T11:01:01Z

src/blueapi/cli/cli.py

+        client = BlueapiClient.from_config(config)
+        oidc_config = client.get_oidc_config()
+        auth = SessionManager(
+            oidc_config, cache_manager=SessionCacheManager(config.auth_token_path)
+        )
+        print("Logging in")
+        auth.start_device_flow()


This path is when SessionManager.from_cache(config.auth_token_path) == None: how is its behaviour different? Are we not in a fatal misconfiguration for logging in at this point?

src/blueapi/cli/cli.py

DiamondJoseph · 2024-11-28T11:11:52Z

src/blueapi/service/authentication.py

+
+    def save_cache(self, cache: Cache) -> None:
+        cache_json: str = cache.model_dump_json()
+        cache_base64: bytes = base64.b64encode(cache_json.encode("utf-8"))


Did we decide we weren't going to base64 encode this before saving? It'd make it slightly simpler

I think we though we will keep it

DiamondJoseph · 2024-11-28T11:35:37Z

src/blueapi/service/authentication.py

+        cache_dir = os.environ.get("XDG_CACHE_HOME")
+        if not cache_dir:
+            cache_dir = os.path.expanduser(BLUEAPI_CACHE_LOCATION)
+            if cache_dir.startswith("~/"):  # Expansion failed.


Should: What are we actually testing for here? Do we need an absolute path, in which case we should make a Path and check that it is absolute: and that the directory exists. Currently we allow a relative path if it's not in the home directory, and allow paths that may not exist.

This test checks whether the ~/ relative expansion has failed. If the path is absolute, it will remain unchanged. Using relative paths is simply more convenient than using absolute ones.

DiamondJoseph · 2024-11-28T12:15:15Z

src/blueapi/service/authentication.py

+        except Exception:
+            return None


If you remove these lines, the Exception will propagate up out of this method and you don't need to account for the | None case until wherever you want to catch the Exception

DiamondJoseph · 2024-11-28T12:16:13Z

src/blueapi/service/authentication.py

+                return ""
+        return ""


As above, if you remove the path where cache is None, you don't have to catch this empty string elsewhere.

DiamondJoseph · 2024-11-28T12:22:59Z

src/blueapi/service/main.py

+async def on_token_error_401(_: Request, __: Exception):
+    return JSONResponse(
+        status_code=status.HTTP_401_UNAUTHORIZED,
+        content={},


All challenges defined by this specification MUST use the auth-scheme
value "Bearer". This scheme MUST be followed by one or more
auth-param values. The auth-param attributes used or defined by this
specification are as follows. Other auth-param attributes MAY be
used as well.

If the protected resource request included an access token and failed
authentication, the resource server SHOULD include the "error"
attribute to provide the client with the reason why the access
request was declined.

3.1. Error Codes

When a request fails, the resource server responds using the appropriate HTTP status code (typically, 400, 401, 403, or 405) and includes one of the following error codes in the response: invalid_request The request is missing a required parameter, includes an unsupported parameter or parameter value, repeats the same parameter, uses more than one method for including an access token, or is otherwise malformed. The resource server SHOULD respond with the HTTP 400 (Bad Request) status code. invalid_token The access token provided is expired, revoked, malformed, or invalid for other reasons. The resource SHOULD respond with the HTTP 401 (Unauthorized) status code. The client MAY request a new access token and retry the protected resource request. insufficient_scope The request requires higher privileges than provided by the access token. The resource server SHOULD respond with the HTTP 403 (Forbidden) status code and MAY include the "scope" attribute with the scope necessary to access the protected resource. If the request lacks any authentication information (e.g., the client was unaware that authentication is necessary or attempted using an unsupported authentication method), the resource server SHOULD NOT include an error code or other error information.

Quotes end:
return JsonResponse(status_code=status.HTTP_401_UNAUTHORIZED, headers={"WWW-Authenticate": "Bearer realm=blueapi"})```

seems a reasonable first attempt. According to the spec we SHOULD check what type of JWTException we get and set the error appropriately too.

ZohebShaikh force-pushed the adding-oauthn branch from 90fdb41 to f38e0c3 Compare October 25, 2024 15:54

ZohebShaikh requested review from callumforrester and keithralphs October 25, 2024 16:03

DiamondJoseph force-pushed the adding-oauthn branch 5 times, most recently from bf18693 to 7c9779b Compare November 8, 2024 12:32

ZohebShaikh and others added 14 commits November 11, 2024 14:32

added oauth

e810563

Test refactoring

983ea61

Linting

5ded18a

Move RMQ launch into docker-compose

e6b396c

More changes

0d20a46

Refactor tests to not require mocking jwt_decode

365b59e

Refactor dependencies

f23a828

Consistency in names

a7e0515

Test changes for mocks

c99aa5c

Revert unrelated changes

5f5b355

Remove vestigial decoupling

82f1ddd

Revert unrelated changes

a6185a5

Re-use fixtures

0b270d4

Refresh dev-requirements

3b06ea0

DiamondJoseph force-pushed the adding-oauthn branch from cf632d0 to 3b06ea0 Compare November 11, 2024 14:38

DiamondJoseph added 6 commits November 11, 2024 14:51

Check if access_token is jwt first

2c8e614

Revert accidental change

69af289

lint

13ca9b5

Last test

1f2b340

Linting

def1ccc

whitespace changes

355b055

ZohebShaikh added 3 commits November 21, 2024 11:33

added test for 401 unauthenticated response

9d116b1

added tests for main server auth

2e365e0

made changes to the conftest

66eee00

ZohebShaikh deployed to release November 21, 2024 13:46 — with GitHub Actions Active

ZohebShaikh deployed to prod November 21, 2024 13:48 — with GitHub Actions Active

tpoliaw reviewed Nov 21, 2024

View reviewed changes

ZohebShaikh added 14 commits November 25, 2024 22:23

added design changes

c015582

saving access token

721fa22

updated flow

cd69900

simplified getting valid access token

638a0cc

updated working tests

f961e51

added unit tests for authentication

4904ee2

removed pytest skip for tests

8166c4d

added tests for oauth

ea471d8

simplified tests

27c12ce

added tests for coverage

d7e32b2

Merge branch 'main' into adding-oauthn

fa30faa

added detail for 401

4178ccd

updated system test comment

c3899b3

updated test

889730b

DiamondJoseph reviewed Nov 28, 2024

View reviewed changes

ZohebShaikh added 6 commits November 28, 2024 13:21

added code review changes

841adcb

added browser to open the login page

9037ce2

changed name to blueapi

45bf6c2

deleted expand user check

07c9686

added code review changes

bb52bdb

updated test

6fc6b73

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding oauthn #690

Adding oauthn #690

ZohebShaikh commented Oct 25, 2024 •

edited

Loading

codecov bot commented Oct 25, 2024 •

edited

Loading

tpoliaw left a comment

tpoliaw Nov 21, 2024

tpoliaw Nov 21, 2024

ZohebShaikh Nov 22, 2024

tpoliaw Nov 22, 2024

DiamondJoseph Nov 28, 2024

ZohebShaikh commented Nov 22, 2024

tpoliaw commented Nov 22, 2024

DiamondJoseph Nov 28, 2024

DiamondJoseph Nov 28, 2024

DiamondJoseph Nov 28, 2024

DiamondJoseph Nov 28, 2024

ZohebShaikh Nov 28, 2024

DiamondJoseph Nov 28, 2024

ZohebShaikh Nov 28, 2024

DiamondJoseph Nov 28, 2024

DiamondJoseph Nov 28, 2024

DiamondJoseph Nov 28, 2024

Adding oauthn #690

Are you sure you want to change the base?

Adding oauthn #690

Conversation

ZohebShaikh commented Oct 25, 2024 • edited Loading

Alternative to the authentication were investigated:-

codecov bot commented Oct 25, 2024 • edited Loading

Codecov Report

tpoliaw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZohebShaikh commented Nov 22, 2024

tpoliaw commented Nov 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZohebShaikh commented Oct 25, 2024 •

edited

Loading

codecov bot commented Oct 25, 2024 •

edited

Loading