-
Notifications
You must be signed in to change notification settings - Fork 177
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
feat: Added Custom Auth #1139
base: main
Are you sure you want to change the base?
feat: Added Custom Auth #1139
Conversation
@kaanrkaan is attempting to deploy a commit to the Hatchet Team on Vercel. A member of the Team first needs to authorize it. |
@abelanger5, I would greatly appreciate any feedback or review you may have on this. Thank you! |
Hey @kaanrkaan, thanks for the PR! I'll be taking a close look at this in the next couple of days and will keep you updated. |
any update on this PR? |
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.
looks good to me
@abelanger5 any updates on the PR? |
…y IDPs. For example authentik
a3388d5
to
35449df
Compare
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.
Hey @kaanrkaan, thanks again for this PR! We were very low on bandwidth during the holiday season, but I'd love to get this merged in for the next release. Overall the PR looks good, I left some requested changes. Note that I don't think the improvements to the UI/usability are strictly necessary, so feel free to resolve that comment if you'd prefer not to tackle it.
Additionally, as a heads up, because this PR regenerates our swagger spec, we can't merge it directly into main without verifying the spec manually (although any diff would be caught by our CI tests, we would like to err on the side of safety).
After all requested changes have been resolved, we will do one last pass with at least 2 team members before merge, and we'll merge it into a temporary branch to verify before merging into main.
@@ -21,6 +21,10 @@ func (u *MetadataService) MetadataGet(ctx echo.Context, request gen.MetadataGetR | |||
authTypes = append(authTypes, "github") | |||
} | |||
|
|||
if u.config.Auth.ConfigFile.Custom.Enabled { | |||
authTypes = append(authTypes, "custom") |
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.
For a small UI improvement, it would be great to be able to provide a name and an icon URL which can be sent to the UI in login/index.tsx
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.
Corresponding changes will also need to be made in ./register/index.tsx
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.
Please remove proto files from this PR -- we can update the proto version in a different PR, as this will break our CI tests.
return nil, nil, fmt.Errorf("custom client secret is required") | ||
} | ||
|
||
if cf.Auth.Custom.AuthorizationURL == "" { |
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.
Some validation via url.Parse
on the AuthorizationURL
and AccessTokenURL
would be useful here
|
||
AuthorizationURL string `mapstructure:"authorizationURL" json:"authorizationURL,omitempty"` | ||
AccessTokenURL string `mapstructure:"accessTokenURL" json:"accessTokenURL,omitempty"` | ||
ResourceURL string `mapstructure:"resourceURL" json:"resourceURL,omitempty"` |
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.
Is this referring to a userinfo URL? If so, it might be helpful to rename it to UserInfoURL
AccessTokenURL string `mapstructure:"accessTokenURL" json:"accessTokenURL,omitempty"` | ||
ResourceURL string `mapstructure:"resourceURL" json:"resourceURL,omitempty"` | ||
RedirectURL string `mapstructure:"redirectURL" json:"redirectURL,omitempty"` | ||
UserIdentifier string `mapstructure:"userIdentifier" json:"userIdentifier,omitempty"` |
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 field looks unused
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.
Please remove this file from the PR -- we are working on fixing this migration in a separate PR.
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.
Same as above, please remove from the PR
"NGShXCFbKbmuNeyy7/THzLqNU0od0O8wrgoAgZhkPIWwM4bEm0LflJg4F/xbrkgJMlgwAcmbpR1R4G2U", | ||
"b6TNMtJmGVlDlpFGolnIBmzxqlU4ya3E8m+88Q6ZYP4OcnnNUk5s6pKqYCvvtkoFzElxURVw3odsBEEC", | ||
"k8yHrKP1KoPJo5QHaRK4J677evf6/wIAAP//MiYyXyABAgA=", | ||
"H4sIAAAAAAAC/+y9e2/jOLIo/lUE/X7A3QWcZ3fPmRPg/OFO3N3eTidZO5lg7yAIaIm2OZEljUjlcRr5", |
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.
As a heads up, we will need to verify this spec before merging into main
-- we will likely change the base branch to a custom branch and rerun all generators before merging.
Description
Motivation: The current self-hosted version supports authentication through Google, GitHub, and local accounts. However, to provide a more flexible and comprehensive authentication solution, we propose integrating a Generic OAuth2. This integration enhances the versatility and scalability of the self-hosted version, making it more adaptable to diverse environments and user needs.
This pull request introduces support for a new custom OAuth flow, including backend and frontend changes. The most important changes include adding new API endpoints, updating the OAuth configuration, and modifying the frontend login page to support the custom OAuth provider.
Backend Changes:
openapi.yaml
anduser.yaml
. ([[1]](https://github.com/hatchet-dev/hatchet/pull/1139/files#diff-97b387589242cdf48a7b66f4daa27066a206a9dcd8659d8587740c904aaa1ce7R43-R46)
,[[2]](https://github.com/hatchet-dev/hatchet/pull/1139/files#diff-c9b0ba958712a3995a7c08989693bf7ddc047f2ada30416b359c158d444ad0f9R180-R209)
)custom_oauth_callback.go
. ([api/v1/server/handlers/users/custom_oauth_callback.goR1-R171](https://github.com/hatchet-dev/hatchet/pull/1139/files#diff-efb7cb724507a0ef56ab3cc46321b34c90f4092cf4e0b7f7e27a6f4317915ed5R1-R171)
)custom_oauth_start.go
. ([api/v1/server/handlers/users/custom_oauth_start.goR1-R30](https://github.com/hatchet-dev/hatchet/pull/1139/files#diff-dd6f0cfcf68493068af113851e0eb93db747cc84ec34f24b0c926da594a1eac2R1-R30)
)configs.go
. ([[1]](https://github.com/hatchet-dev/hatchet/pull/1139/files#diff-0a89e2dc1668b45a0c13a724b8370395c6ba7858fc2e056b4dbe3c2294609922R14-R18)
,[[2]](https://github.com/hatchet-dev/hatchet/pull/1139/files#diff-0a89e2dc1668b45a0c13a724b8370395c6ba7858fc2e056b4dbe3c2294609922R66-R78)
)Frontend Changes:
index.tsx
. ([[1]](https://github.com/hatchet-dev/hatchet/pull/1139/files#diff-58fcbd495ec97f2dcd23d93ae89964a861312c6ad7e57e77581aabb6ecbbd561R26-R33)
,[[2]](https://github.com/hatchet-dev/hatchet/pull/1139/files#diff-58fcbd495ec97f2dcd23d93ae89964a861312c6ad7e57e77581aabb6ecbbd561R45)
,[[3]](https://github.com/hatchet-dev/hatchet/pull/1139/files#diff-58fcbd495ec97f2dcd23d93ae89964a861312c6ad7e57e77581aabb6ecbbd561R161-R170)
)Other Changes:
[[1]](https://github.com/hatchet-dev/hatchet/pull/1139/files#diff-5dae58817d67249465ccd577680efbe209af6efd05e69414732d6e1e5f273b34L4-R4)
,[[2]](https://github.com/hatchet-dev/hatchet/pull/1139/files#diff-a7b46e898d2bf451ca3ddef8aec670eaf3819e194f660c19dac9da4f2548edc5L4-R4)
,[[3]](https://github.com/hatchet-dev/hatchet/pull/1139/files#diff-4aad7ea9705bab7520e2aebcf18514f535b747ca161334b11d5a3c0f7e8fe46eL4-R4)
,[[4]](https://github.com/hatchet-dev/hatchet/pull/1139/files#diff-6b1d5f75ca8e6540cb6248394678ed3e41631a394681e9b69bb14fd356a96b9cL4-R4)
,[[5]](https://github.com/hatchet-dev/hatchet/pull/1139/files#diff-ee2f134689e37199fade5382ed922d587113363950401bf11f36a6b377644e65L4-R4)
,[[6]](https://github.com/hatchet-dev/hatchet/pull/1139/files#diff-379298bd2027b3e847425ab728ebd7d083ea09afcae68991e1a4db672c075d07L4-R4)
)gen.go
. ([[1]](https://github.com/hatchet-dev/hatchet/pull/1139/files#diff-984209ef99f3799ba42a3c327662ca8344b5a2e5cda37abc31040e0aff2c9a58R2129-R2134)
,[[2]](https://github.com/hatchet-dev/hatchet/pull/1139/files#diff-984209ef99f3799ba42a3c327662ca8344b5a2e5cda37abc31040e0aff2c9a58R3277-R3300)
,[[3]](https://github.com/hatchet-dev/hatchet/pull/1139/files#diff-984209ef99f3799ba42a3c327662ca8344b5a2e5cda37abc31040e0aff2c9a58R7531-R7584)
,[[4]](https://github.com/hatchet-dev/hatchet/pull/1139/files#diff-984209ef99f3799ba42a3c327662ca8344b5a2e5cda37abc31040e0aff2c9a58R8751-R8756)
,[[5]](https://github.com/hatchet-dev/hatchet/pull/1139/files#diff-984209ef99f3799ba42a3c327662ca8344b5a2e5cda37abc31040e0aff2c9a58R10533-R10574)
,[[6]](https://github.com/hatchet-dev/hatchet/pull/1139/files#diff-984209ef99f3799ba42a3c327662ca8344b5a2e5cda37abc31040e0aff2c9a58R11894-R11911)
)Added following env values:
Type of change