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.

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

Dynamically load the SSO IdP metadata at startup #279

Merged
merged 2 commits into from
Feb 27, 2023

Conversation

humphd
Copy link
Contributor

@humphd humphd commented Feb 24, 2023

This adjusts our SAML code a bit to dynamically load the IdP's metadata at startup vs. requiring us to pass it in. This also allows us to check that the IdP is reachable.

This data is not a secret (the metadata is always public), so we don't need to keep it in secrets. And storing the entire file seems like a waste, so this uses the URL. This change will make it easier to setup staging and production too.

One note about the way I'm doing this. This code intentionally re-uses a Promise, so I haven't forgotten the await in my fetch call. It's too bad Samlify doesn't support this use case directly.

I've put this in 0.5 since I missed code freeze for 0.4.

@humphd humphd added this to the Milestone 0.5 milestone Feb 24, 2023
@humphd humphd self-assigned this Feb 24, 2023
@humphd
Copy link
Contributor Author

humphd commented Feb 24, 2023

I had to update CI for the e2e tests to include the login container. I tried to run it as a service container like MySQL and Redis, but apparently you can't mount volumes when you do this, because your code isn't checked-out when the containers start. So I use docker compose to do it, and it seems fine.

Copy link
Contributor

@Eakam1007 Eakam1007 left a comment

Choose a reason for hiding this comment

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

Looks good. I am just curious about why we reuse the promise instead of waiting for it to resolve. Does the await take too long?

@humphd
Copy link
Contributor Author

humphd commented Feb 25, 2023

@Eakam1007 the way this code is written. We'd have to do an async init() or something.

@humphd humphd merged commit 79bebf5 into DevelopingSpace:main Feb 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants