Add OIDC login by ajkyffin · Pull Request #154 · ral-facilities/scigateway-auth

ajkyffin · 2025-10-08T11:12:01Z

This PR will close #135

Description

Adds endpoints for OIDC authentication

Testing Instructions

louise-davies · 2025-10-09T09:15:36Z

When trying to run this branch in Docker (either via the image the CI built and uploaded to Harbor or building locally) I get an import error:

ImportError: cannot import name 'OIDC_ICATAuthenticator' from
'scigateway_auth.src.authentication'
(/app/scigateway_auth/src/authentication.py)

EDIT:
64a4b9a - this refactor wasn't completed in the imports, so I just added a commit to fix this

ajkyffin · 2025-10-09T09:33:52Z

Grrrr. Yes I broke it but I made the linter happy.

codecov · 2025-10-10T12:41:59Z

Codecov Report

❌ Patch coverage is 17.42424% with 109 lines in your changes missing coverage. Please review.
✅ Project coverage is 47.91%. Comparing base (bef86ae) to head (efb2f74).
⚠️ Report is 53 commits behind head on main.

Files with missing lines	Patch %	Lines
scigateway_auth/src/oidc.py	0.00%	57 Missing ⚠️
scigateway_auth/routers/authentication.py	0.00%	40 Missing ⚠️
scigateway_auth/src/authentication.py	27.27%	8 Missing ⚠️
scigateway_auth/common/config.py	82.60%	4 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main     #154       +/-   ##
===========================================
- Coverage   65.55%   47.91%   -17.64%     
===========================================
  Files           9       12        +3     
  Lines         360      480      +120     
  Branches       13        0       -13     
===========================================
- Hits          236      230        -6     
- Misses        124      250      +126

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Asked Alan and he said it was originally an error to set to False and thought it had already been fixed

…s everything

patrick-austin

Looks OK, the upload to codecov seems to be failing (but I don't have the permissions to dif to deeply so not sure?) but coverage is still 60% for me locally, and the new lines not covered are in a file that wasn't covered before either (routers/authentication.py).

There were a couple of places where we might be able use / benefit from SecretStr, but functionally this shouldn't change anything. Given it's last working day @ajkyffin I'm happy to approve this as is without bogging things down. Extending the use of SecretStr (if it's desirable) can be done as a follow up issue/PR.

patrick-austin · 2025-12-09T15:29:01Z

scigateway_auth/common/config.py

+from typing import List, Self

-from pydantic import BaseModel, ConfigDict
+from pydantic import BaseModel, ConfigDict, model_validator


Suggested change

from pydantic import BaseModel, ConfigDict, model_validator

from pydantic import BaseModel, ConfigDict, model_validator, SecretStr

To be used for secrets.

patrick-austin · 2025-12-09T15:30:24Z

scigateway_auth/common/config.py

+    display_name: str
+    configuration_url: str
+    client_id: str
+    client_secret: str = None


Suggested change

client_secret: str = None

client_secret: SecretStr = None

SecretStr prevents secret values from appearing in logs, so might be useful here.

patrick-austin · 2025-12-09T15:31:04Z

scigateway_auth/common/config.py

+    oidc_providers: dict[str, OidcProviderConfig] = {}
+    oidc_redirect_uri: str = None
+    oidc_icat_authenticator: str = None
+    oidc_icat_authenticator_token: str = None


Suggested change

oidc_icat_authenticator_token: str = None

oidc_icat_authenticator_token: SecretStr = None

Would this also benefit from being a SecretStr?

patrick-austin · 2025-12-09T15:32:51Z

scigateway_auth/routers/authentication.py

+        providers[provider_id] = {
+            "display_name": provider_config.display_name,
+            "configuration_url": provider_config.configuration_url,
+            "client_id": provider_config.client_id,
+            "pkce": False if provider_config.client_secret else True,
+            "scope": provider_config.scope,
+        }


Could probably do this with a combination of exclude and serialization aliases, but if it works as is then that's fine too.

ajkyffin force-pushed the oidc branch from bc2e40c to efb2f74 Compare October 10, 2025 12:40

ajkyffin marked this pull request as ready for review November 24, 2025 16:05

ajkyffin requested review from VKTB, louise-davies and patrick-austin December 4, 2025 10:00

ajkyffin and others added 22 commits December 4, 2025 10:26

Add OIDC login

e7f388c

Fix typo in oidc_login method

cc03143

Set verify_exp: True in decode options for OidcHandler

0cd2ef5

Asked Alan and he said it was originally an error to set to False and thought it had already been fixed

Rework that adds support for client_secret and caching and also break…

79c49de

…s everything

oidc_redirect_url changed to oidc_redirect_uri

447aff0

Use str type hint instead of HttpUrl

efaf442

Fix issuer check

e8f5998

Add leeway to oidc payload check

f51d8ec

Remove extra second of leeway which was based on incorrect logic

df6f743

Fix linting errors

4656d08

Fix linting error

dbaf6a1

Fix import error caused by incomplete OIDCICATAuthenticator class rename

35d716c

Missing parentheses calling r.raise_for_status

529514a

Add tests for OIDC backend

9a8768f

Add oidc.get_provider_config function

eeaf1aa

Use jwt.PyJWKSet to read keys

13cf647

Add check for JWK use

0b4d50a

Add check for calling the token endpoint without a client_secret

4f961a4

Refactor ICATAuthenticator so that OIDCICATAuthenticator is not needed

f5ee06c

Move non-icat calls out of except ICATAuthenticationError block

7d0c8e7

Add docstrings

b32a6f0

Add OIDC config documentation to README.md

f79eb57

ajkyffin force-pushed the oidc branch from 6f1d115 to f79eb57 Compare December 4, 2025 10:30

patrick-austin approved these changes Dec 9, 2025

View reviewed changes

ajkyffin mentioned this pull request Dec 9, 2025

Use SecretStr for auth token and client secret #162

Open

ajkyffin merged commit ed7d888 into main Dec 9, 2025
10 checks passed

ajkyffin deleted the oidc branch December 9, 2025 19:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add OIDC login#154

Add OIDC login#154
ajkyffin merged 22 commits intomainfrom
oidc

ajkyffin commented Oct 8, 2025 •

edited by patrick-austin

Loading

Uh oh!

louise-davies commented Oct 9, 2025 •

edited

Loading

Uh oh!

ajkyffin commented Oct 9, 2025

Uh oh!

codecov bot commented Oct 10, 2025

Uh oh!

patrick-austin left a comment

Uh oh!

patrick-austin Dec 9, 2025

Uh oh!

patrick-austin Dec 9, 2025

Uh oh!

patrick-austin Dec 9, 2025

Uh oh!

patrick-austin Dec 9, 2025

Uh oh!

ajkyffin Dec 9, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	from pydantic import BaseModel, ConfigDict, model_validator
	from pydantic import BaseModel, ConfigDict, model_validator, SecretStr

	oidc_icat_authenticator_token: str = None
	oidc_icat_authenticator_token: SecretStr = None

Conversation

ajkyffin commented Oct 8, 2025 • edited by patrick-austin Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing Instructions

Uh oh!

louise-davies commented Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ajkyffin commented Oct 9, 2025

Uh oh!

codecov bot commented Oct 10, 2025

Codecov Report

Uh oh!

patrick-austin left a comment

Choose a reason for hiding this comment

Uh oh!

patrick-austin Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

patrick-austin Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

patrick-austin Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

patrick-austin Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

ajkyffin Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ajkyffin commented Oct 8, 2025 •

edited by patrick-austin

Loading

louise-davies commented Oct 9, 2025 •

edited

Loading