- 
                Notifications
    You must be signed in to change notification settings 
- Fork 1
Feature/decouple keycloak #65
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
base: main
Are you sure you want to change the base?
Conversation
| KEYCLOAK_ALLOWED_ROLES=null | ||
| ADMIN_USERS=["[email protected]"] | ||
| OIDC_ISSUER=http://localhost:5556 | ||
| OIDC_AUDIENCE=caddy-client | 
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.
duplicate var
|  | ||
|  | ||
| run: | ||
| # will run with local client by default | 
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.
Can we remove this local client thing from docker runs now?
| - caddy-net | ||
| restart: unless-stopped | ||
| healthcheck: | ||
| test: ["CMD", "wget", "--quiet", "--tries=1", "--spider", "http://localhost:5556/healthz"] | 
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.
typo z in health endpoint url?
| @@ -0,0 +1,16 @@ | |||
| --- | |||
| // OAuth2 agnostic authentication redirect | |||
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.
Does this redirect to auth once you hit the app? If so, how does this work with something like keycloak that sits on the alb?
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.
yep. I have tested this in dev with keycloak, im not 100% sure about the details of the ALB, maybe one for @RyanWhite25 ?
| @@ -0,0 +1,29 @@ | |||
| --- | |||
| // Handle logout - clear session and redirect to OIDC provider logout | |||
| const dexIssuer = 'http://localhost:5556'; | |||
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.
This should probably be the OIDC_ISSUER var rather than hard coded to dex. Same with the client being OIDC_AUDIENCE instead
| return Astro.redirect(logoutUrl.toString()); | ||
| } else { | ||
| // Local: Just redirect to login (Dex doesn't have a standard logout endpoint) | 
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.
We don't have logout at the minute so I don't mind this bit being a WIP until it's sorted out by platform etc
| const { collection: collectionId, user: userId } = Astro.params; | ||
| const { collection, error } = await getCollection(collectionId || '', Astro.request.headers.get('x-amzn-oidc-accesstoken')); | ||
| const token = await Astro.session.get('accessToken'); | ||
| const { collection, error } = await getCollection(collectionId || '', token); | 
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.
This is outside the scope of this PR, but why would we even bother doing a GET without a collection id ?
| self.disable_auth_signature_verification = disable_auth_signature_verification | ||
| self.auth_provider_public_key = auth_provider_public_key | ||
| self.oidc_issuer = oidc_issuer | ||
| self.oidc_audience = oidc_audience or "account" | 
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.
why "account" default here?
| with open(file_path, "w") as f: | ||
| f.write(f"Test content for {filename}") | ||
|  | ||
| token = "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJhbGljZUBleGFtcGxlLmNvbSIsImVtYWlsIjoiYWxpY2VAZXhhbXBsZS5jb20ifQ.mock-signature" # pragma: allowlist secret | 
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.
Could we use the admin_user.token property here?
| description = "should auth signature be disabled" | ||
| } | ||
|  | ||
| variable "keycloak_allowed_roles" { | 
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.
Could also delete this but shooters choice
As an Engineer I want to be able to switch between OAuth2.0 providers