Add OIDC authentication #6534
Merged
Add OIDC authentication #6534
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
|
You can use keycloak. Put the following into a Then, you can import a client with this JSON into your master realm:
|
fm3
reviewed
Oct 24, 2022
There was a problem hiding this comment.
Thanks for fighting your way through this! I think this is already going in a great direction. I added a few comments, mostly about naming (let’s avoid the abbreviation oidc), and about accessing the config.
We might want to discuss later if we want to redirect the user directly or keep the API somewhat rest-ful, and return json uris and do the redirection in the front-end but that is a change we can build later on, when a frontend is added.
| /* | ||
| Build redirect URL to redirect to OIDC provider for auth request (https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest) | ||
| */ | ||
| def redirectUrl(openIdClient: OpenIdConnectConfig, redirectUrl: String): Fox[String] = |
There was a problem hiding this comment.
Suggested change
| def redirectUrl(openIdClient: OpenIdConnectConfig, redirectUrl: String): Fox[String] = | |
| def getRedirectUrl: Fox[String] = |
I’d add the get because redirect also reads a bit like a verb.
I’d say it is not necessary to pass the parameters in from the caller, but rather have lazy vals for the client itself, I thinkt it already has everything it needs to construct them (mostly the injected WkConf).
Maybe the OpenIdConnectConfig case class is not needed at all, but the fields can be read from the passed WkConf where needed?
There was a problem hiding this comment.
Redirect URL as stated above should not be known to the oidcclient IMO. The oidcconfig info can also be gathered at the client, true. However, the case class can be useful IMO because it allows for checking if the configuration is valid (and thus determine if the routes are activated)
| absoluteOidcCallbackURL, | ||
| request.queryString.get("code").flatMap(_.headOption).getOrElse("missing code"), | ||
| ) | ||
| oidc: OpenConnectId <- validateJsValue[OpenConnectId](code).toFox |
There was a problem hiding this comment.
Suggested change
| oidc: OpenConnectId <- validateJsValue[OpenConnectId](code).toFox | |
| openConnectId <- validateJsValue[OpenConnectId](code).toFox |
(if I understand correctly that OpenIdConnect uses OpenConnectIds? Seems confusing to me, but may be correct if that’s what the protocol states)
There was a problem hiding this comment.
This is not something that is directly part of the protocol. I renamed it to OpenIdConnectClaimSet since it describes it more accurately
|
Yes, it does not all have to be done in the backend. Typically the front end does some of the stuff in a OIDC flow. Also including refreshing authorization. I just did this so I could test it. |
Yes, I was a bit confused. We are using code flow authorization with openid scope (https://darutk.medium.com/diagrams-of-all-the-openid-connect-flows-6968e3990660). I was a bit confused because we do not use a client secret atm, which is often associated with implicit flow, but it works because keycloak is configured to not use authorization. |
Did that in the backend, frontend needs to be updated, also e2e tests will fail again, but I couldn't build them new after |
|
Thanks!
did that work before? |
I get ,also for yarn install I get: |
|
Now why exactly yarn install fails, I’m uncertain. Which node version do you have? @philippotto do you have any insights here? Could it be specific to the linux distribution? In any case, I can refresh the snapshots here to avoid blocking this PR by the install issue. But we should figure that out. |
|
Nvm, with the new command the e2e tests run 🤷 |
|
@frcroth Could you have a look at the CI? See https://app.circleci.com/pipelines/github/scalableminds/webknossos/9133/workflows/463f4087-80d1-43fc-9a94-a4fb2b9cb8cc/jobs/20501 |
@philippotto Fixed now |
|
@philippotto I think front end needs to be adapted again because a name changed.
Btw the |
Great 👍
Done! For such small things, such as renaming a var, feel free to do a search&replace in the frontend folder next time :) |
fm3
reviewed
Dec 1, 2022
fm3
approved these changes
Dec 1, 2022
Caused by empty result
|
Will be merged next week. |
hotzenklotz
added a commit
that referenced
this pull request
Dec 6, 2022
…cing * 'master' of github.com:scalableminds/webknossos: (23 commits) Guard against invalid-mag bucket volume save actions (#6660) Add OIDC authentication (#6534) Add second (non-admin) default user to "initial data" trigger (#6666) Fix and improve miscellaneous things in Folders tab (#6674) Improves mag and voxelSize inferral for remote datasets (#6670) Squashed commit of the following: Fix import of N5 datasets by adding n5 schema to frontend validation (#6668) Virtual Folder Structure for Datasets (#6591) Ability to recover from webGL context loss (#6663) Fix assertion that referenced teams cannot be deleted (#6664) fix task summary with pending jobs (#6662) Only allow taskTypeId as parameter in task creation (#6640) Release 22.12.0 (#6661) Create Annotation From View Mode: Keep Activated Mapping (#6647) Workaround to avoid false-positive version warning (#6656) Fix WK-lib download snippet (#6605) Use LZ4 with WASM for volume saving/undo/redo (#6652) Fix version number extraction when building docker image (#6655) Pass mapping name to precompute meshes (#6651) Deduplicate bboxes when importing NML (#6648) ...
philippotto
added a commit
that referenced
this pull request
Dec 13, 2022
* Add second (non-admin) default user to "initial data" trigger (#6666) * add second (non-admin) user to initial data trigger * rename second user and DRY insertion method * format * name default organization team "Default" when inserting initial data (analoguous to general organization creation) * Add OIDC authentication (#6534) Co-authored-by: Florian M <[email protected]> Co-authored-by: Philipp Otto <[email protected]> * Guard against invalid-mag bucket volume save actions (#6660) * [WIP] guard against invalid-mag bucket volume save actions * avoid unnecessary creation of objects in ResolutionInfo instances * use layer's resolution info when in wkstore adapter * rename bucketPositionToGlobalAddress to bucketPositionToGlobalAddressOld * get rid of bucketPositionToGlobalAddressOld * rename bucketPositionToGlobalAddressNew to bucketPositionToGlobalAddress * simplify getBucketExtent signature * fix tests * fix quick-select tool for scenarios where the color layer is available in the current mag but the volume layer isn't * Update frontend/javascripts/test/model/binary/layers/wkstore_adapter.spec.ts * avoid losing error chain during conversion Co-authored-by: Philipp Otto <[email protected]> Co-authored-by: Philipp Otto <[email protected]> * Provide valid JSON schema (#6642) extra `{}` cause JSON decode error in python standard JSON library and webknossos instance Co-authored-by: Philipp Otto <[email protected]> * Improve layout of dashboard * Swagger annotation for shortLinkByKey (#6682) * Bump qs from 6.5.2 to 6.5.3 Bumps [qs](https://github.com/ljharb/qs) from 6.5.2 to 6.5.3. - [Release notes](https://github.com/ljharb/qs/releases) - [Changelog](https://github.com/ljharb/qs/blob/main/CHANGELOG.md) - [Commits](ljharb/qs@v6.5.2...v6.5.3) --- updated-dependencies: - dependency-name: qs dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: Philipp Otto <[email protected]> Co-authored-by: frcroth <[email protected]> Co-authored-by: Florian M <[email protected]> Co-authored-by: Florian M <[email protected]> Co-authored-by: Philipp Otto <[email protected]> Co-authored-by: erjel <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
This was referenced Aug 14, 2023
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.
URL of deployed dev instance (used for testing):
Steps to test:
providerURL = "http://localhost:8080/realms/master/")oidcEnabledto true in the application.conffeaturesblockOpen questions:
Issues:
(Please delete unneeded items, merge only when none are left open)