Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(keycloak): Add keycloak theme, capture university, add ORCid integration #971

Merged
merged 50 commits into from
Feb 29, 2024
Merged

feat(keycloak): Add keycloak theme, capture university, add ORCid integration #971

merged 50 commits into from
Feb 29, 2024

Conversation

theosanderson
Copy link
Member

@theosanderson theosanderson commented Feb 9, 2024 •
edited
Loading

Resolves #743, resolves #745, resolves #546

Preview: https://keycloakify.loculus.org/

image

image

We build a custom keycloak theme with keycloakify (which is react based), which gets packaged (like most Keycloak themes) into a .jar file. We then put that jar file into a Docker image which serves just as a directory to store the image, and copies it out into a volume that is also mounted on keycloak in the right place for keycloak to get the theme. Keycloak does indeed gets the theme and uses it as the login theme.

We also enable the declarative_user_profile feature of Keycloak, and use it to capture the user's University / Organisation.

We also add ORCid keycloak integration and capture the ORCid where possible

Outstanding issues:

#974 - put in real terms of use
#973 - this is branched from a starter template. there is more we can clean up but I don't feel ready to yet, because these are useful docs about how to e.g. add new pages, for now
#1172 - clean up ORCid secrets

@theosanderson theosanderson marked this pull request as draft February 9, 2024 19:29
@theosanderson theosanderson added the preview Triggers a deployment to argocd label Feb 9, 2024
@theosanderson theosanderson force-pushed the keycloakify branch 2 times, most recently from 6694361 to dd0db42 Compare February 9, 2024 20:36
@theosanderson theosanderson changed the title wip: Keycloakify Add keycloak theme and hopefully capture university Feb 9, 2024
@theosanderson theosanderson force-pushed the keycloakify branch 2 times, most recently from 86dcdf5 to 81de8ed Compare February 28, 2024 18:37
@theosanderson theosanderson marked this pull request as ready for review February 28, 2024 19:21
@theosanderson theosanderson changed the title Add keycloak theme and hopefully capture university Add keycloak theme, capture university, add ORCid integration Feb 28, 2024
@corneliusroemer
Use caching and retagging for keycloakify action (#1174)
aa0b53a 
* Use caching and retagging for keycloakify action

* Fix: need to login befor querying docker registry
@corneliusroemer corneliusroemer changed the title Add keycloak theme, capture university, add ORCid integration feat(keycloak): Add keycloak theme, capture university, add ORCid integration Feb 28, 2024
corneliusroemer
corneliusroemer reviewed Feb 28, 2024
View reviewed changes
Copy link
Contributor

@corneliusroemer corneliusroemer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool stuff! Haven't tested yet just looked through code. Should we enable dependabot for this subrepo or rather not - because all that you changed with respect to the source repo is the templates folder?

Will the workflows run despite not being in the root of the repo?

Are some of the files in here unnecessary and could be removed?

@theosanderson
Copy link
Member Author

Cool stuff! Haven't tested yet just looked through code. Should we enable dependabot for this subrepo or rather not - because all that you changed with respect to the source repo is the templates folder?

Will the workflows run despite not being in the root of the repo?

Are some of the files in here unnecessary and could be removed?

No, I think dependabot could be very painful.
The workflow is just a remnant of the starter. I've deleted it now.

Yes, some files may be unnecessary, but form part of the documentation of this starter: #973

@corneliusroemer
Copy link
Contributor

Here's another small PR targeting keycloakify: #1175

@corneliusroemer
fix(ci): trigger e2e on keycloakify changes; don't wait for each imag…
2254a5b 
…e instead use timeout (#1175)

* fix(ci): add `keycloak` to paths that trigger E2E

* No need to wait for particular images, kubernetes will retry pulling images and backoff if not found

Given we build many docker images, we would otherwise wait for each - not just the 2 here.

Instead, use timeout to fail if the images never make it.
@corneliusroemer
Copy link
Contributor

Sorry @theosanderson I merged #1175 into this branch here by accident - feel free to revert if you have objections. I thought I'd enable auto-squash - but it merged directly without requiring review as it's not targeting main.

@theosanderson
Copy link
Member Author

Np looks good but made one comment

@corneliusroemer
Copy link
Contributor

I'm getting an HTTP 500 error when trying to login with testuser:
image

Could be that this is because something is not ready on the deployment side?

@theosanderson
Copy link
Member Author

Hmm that was fine when I last checked. Will have a look in a bit

@theosanderson
Copy link
Member Author

I deleted the app and it seems fine now, but worth keeping an eye on

@corneliusroemer
Copy link
Contributor

corneliusroemer commented Feb 28, 2024 •
edited
Loading

Next time something like this happens I'll try to find the logs - which reminds me we should send our logs to a log aggregator, will make an issue: #1176

@theosanderson
Copy link
Member Author

The keycloak logs were about timeouts waiting for acks

corneliusroemer
corneliusroemer approved these changes Feb 28, 2024
View reviewed changes
Copy link
Contributor

@corneliusroemer corneliusroemer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basic registration/login flow works in my tests.

So does ORCID flow, cool stuff!

Small papercuts here and there, like logout page having button outside of containing box, but that's not relevant at this stage

@corneliusroemer corneliusroemer added javascript auth authentication, authorization, incl. keycloak and keycloakify related issues labels Feb 28, 2024
@theosanderson theosanderson merged commit 68afa9c into main Feb 29, 2024
10 checks passed
@theosanderson theosanderson deleted the keycloakify branch February 29, 2024 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@corneliusroemer corneliusroemer corneliusroemer approved these changes

Assignees
No one assigned
Labels
auth authentication, authorization, incl. keycloak and keycloakify related issues preview Triggers a deployment to argocd
Projects
None yet
Milestone
No milestone
2 participants
@theosanderson @corneliusroemer