-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add JWT-based Auth option for API requests #900
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
p-kaczynski
requested review from
donaldgray,
tomcrane and
christinemacdonald
September 2, 2024 16:25
donaldgray
requested changes
Sep 4, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of the c#12 stuff (primary constructor and object-initializer) will need to be old-skooled until we update the docker images.
donaldgray
approved these changes
Sep 4, 2024
Default defaults to latest major, which is like 13 or 14rn xD
p-kaczynski
force-pushed
the
feature/ICS-31/jwt_customer_auth
branch
from
September 5, 2024 10:13
389de46
to
080bf9c
Compare
donaldgray
approved these changes
Sep 5, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I tried to balance the non-invasive, limited change with doing it at least semi-properly.
I think it should be fine, I ran it locally and it behaved as expected.
Note: this by default will be disabled, config needs to be provided for it to work:
don't worry, this key is made up for this PR note only
of course key can be any Base64 encoded data, e.g.:
I opted NOT to include default because it'd surely end up being the actual key in prod deployments at some point down the line ;)
The
DLCS:JwtValidIssuers:#
should beurn:caspian:site:default
, which is the default value used by Portal, though the final term (heredefault
) can be whatever, configurable per env (so even if key reuse happens we still can limit (or allow) cross-env calls.This will split off the existing path into its own method, do JWT things and if it's indeed a
DLCS Customer Id
claim, correctly signed, will load the customer from repo by that id and use the first key from the list for this customer, before returning to the usual flow.I split the code into respective flows, but I really didn't want to throw exceptions for flow control in sth that's ran on every request, exposed to the world, as it can create spurious overheads. So to keep it hopefully light it just returns a wrapped string (
FailedCaller
) in case of auth failure, which gets turned intoAuthenticationResult
.The code could be nicer I'm sure, but I'd prob have to refactor much, much more, and I really, really don't want to break stuff, so make your judgement ;)