-
Notifications
You must be signed in to change notification settings - Fork 15
Add Console SSO authentication integration #176
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: develop
Are you sure you want to change the base?
Conversation
- Add AuthConfig with domain, apiDomain, audience, organizationId, clientId
- Implement auth middleware that validates tokens via Console authorization API
- Add http4s-client dependency for external API calls
- Create frontend auth service with Auth0 SPA SDK integration
- Add automatic login redirect
- Authorization calls: /api/msc/internal/authz/query/v1/{organizationId}/minis/list
🤖 Generated with [Claude Code](https://claude.ai/code)
Co-Authored-By: Claude <[email protected]>
|
@andreamarcolin could you review the auth logic? @benjben you will probably have comments on the Routing changes. @Brendan-McCauley the FE part is almost verbatim Claude-generated, does it look sensible to you? |
| private val destination = Opts.option[Uri]("destination", "HTTP(s) URL to send output data to (requires --output-json or --output-tsv)", "d").orNone | ||
| private val maxEvents = Opts.option[Int]("max-events", "Maximum number of events of each kind (good, bad) to keep in memory (setting this to 0 disables all /micro endpoints)", "m").orNone | ||
| private val yauaa = Opts.flag("yauaa", "Enable YAUAA user agent enrichment").orFalse | ||
| private val auth = Opts.option[Path]("auth", "Path to Auth0 HOCON configuration file (optional)", "a", "auth.conf").orNone |
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.
What about adding an auth section to the --collector-config file?
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.
That would not make sense because the Collector routes are the only thing not protected by auth :P
Also I like having a separate config file for this because it would be a very specific thing used in our deployments.
| case other => | ||
| resource(other.renderString) | ||
| private val authConfigRoute: HttpRoutes[IO] = HttpRoutes.of[IO] { | ||
| case GET -> Root / "micro" / "auth-config" => |
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.
In which scenario would someone query this path?
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 queried by the UI code (not by persons) so that it can correctly redirect to the auth screen. You can see that in the FE part below.
| } | ||
|
|
||
| val enabled: HttpRoutes[IO] = { | ||
| val apiRoutes = authConfig match { |
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.
It would have helped me to understand this code if baseRoutes was named apiRoutes and apiRoutes was named apiRoutesWithAuth or something like that
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.
| } | ||
| }) | ||
|
|
||
| val onFailure: AuthedRoutes[String, IO] = Kleisli(_ => OptionT.liftF(Forbidden())) |
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.
Doesn't the Forbidden() discard the error messages from above?
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.
It does, yes. The error message only goes to the logs. I don’t want to expose it to the user.
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.
I see! You can delete .as("Authentication denied") then I think
| // prevent eagerly requiring auth for everything | ||
| // we need to bypass auth and fall through into collector routes that will come later | ||
| if (req.pathInfo.startsWith(Root / "micro")) middleware(req) else OptionT.none |
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.
Are we fine with querying the auth endpoint for each single request made to micro?
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.
Not every single request, but only /micro requests, i.e. the UI, /micro/events, /micro/iglu and /micro/reset. (/micro/{all,good,bad} will be disabled when auth/hosted mode is enabled, as in #177).
This must be done this way because the jwt only has the primary org of the user, not all orgs. The user might belong to other orgs, and the only way to validate if the user is authorized for the org in question is to call this endpoint, per @andreamarcolin. He also insisted I don’t cache the response 😇
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't really comment on the UI/routing part, but authz looks fine. Not much more to review as there is no jwt validation (but appreciate the comment) 😆
🤖 Generated with Claude Code