-
Notifications
You must be signed in to change notification settings - Fork 135
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
Add support for local auth configuration #3632
Conversation
✅ Hey kreinecke! The commit authors and yourself have already signed the CLA. |
Codecov Report
@@ Coverage Diff @@
## v2-master #3632 +/- ##
=============================================
+ Coverage 51.42% 51.43% +<.01%
=============================================
Files 725 726 +1
Lines 20567 20569 +2
Branches 3682 3682
=============================================
+ Hits 10577 10579 +2
Misses 9990 9990 |
d6f056c
to
ad10abb
Compare
NOTE |
INVITE_USER_CLIENT_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.
This file should be excluded from merge to disable this functionality until front-end changes are complete.
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.
Do you want to comment out the values before we merge?
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.
After other changes have been implemented and gates passed, will remove default config file from this PR entirely as this feature currently has no corresponding front end changes. Will update default config in the pending frontend change - feature will then be complete and config options fully usable.
Signed-off-by: Kate E. Reinecke <50168367+kreinecke@users.noreply.github.com>
Signed-off-by: Kate E. Reinecke <50168367+kreinecke@users.noreply.github.com>
…isterMigration call. Signed-off-by: Kate E. Reinecke <50168367+kreinecke@users.noreply.github.com>
Signed-off-by: Kate E. Reinecke <50168367+kreinecke@users.noreply.github.com>
Signed-off-by: Kate E. Reinecke <50168367+kreinecke@users.noreply.github.com>
Signed-off-by: Kate E. Reinecke <50168367+kreinecke@users.noreply.github.com>
Signed-off-by: Kate E. Reinecke <50168367+kreinecke@users.noreply.github.com>
…set in the config file Signed-off-by: Kate E. Reinecke <50168367+kreinecke@users.noreply.github.com>
Signed-off-by: Kate E. Reinecke <50168367+kreinecke@users.noreply.github.com>
Signed-off-by: Kate E. Reinecke <50168367+kreinecke@users.noreply.github.com>
Signed-off-by: Kate E. Reinecke <50168367+kreinecke@users.noreply.github.com>
Signed-off-by: Kate E. Reinecke <50168367+kreinecke@users.noreply.github.com>
…enting a new auth endpoint type env var to select local or remote auth type. Modify local users table schema to make GUI and username unique, Signed-off-by: Kate E. Reinecke <50168367+kreinecke@users.noreply.github.com>
…r. Add code to update last login time on login. Test for last login time update in progress. Signed-off-by: Kate E. Reinecke <50168367+kreinecke@users.noreply.github.com>
…remove info comments Signed-off-by: Kate E. Reinecke <50168367+kreinecke@users.noreply.github.com>
Signed-off-by: Kate E. Reinecke <50168367+kreinecke@users.noreply.github.com>
… jetstream startup appropriately depending on whether local or remote auth is configured. Signed-off-by: Kate E. Reinecke <50168367+kreinecke@users.noreply.github.com>
…de Climate gate. Signed-off-by: Kate E. Reinecke <50168367+kreinecke@users.noreply.github.com>
Signed-off-by: Kate E. Reinecke <50168367+kreinecke@users.noreply.github.com>
Signed-off-by: Kate E. Reinecke <50168367+kreinecke@users.noreply.github.com>
…YPE not defined. Add auth_endpoint_type to console config DB table Signed-off-by: Kate E. Reinecke <50168367+kreinecke@users.noreply.github.com>
Signed-off-by: Kate E. Reinecke <50168367+kreinecke@users.noreply.github.com>
84fd2bc
to
a2d1606
Compare
Signed-off-by: Kate E. Reinecke <50168367+kreinecke@users.noreply.github.com>
Signed-off-by: Kate E. Reinecke <50168367+kreinecke@users.noreply.github.com>
Signed-off-by: Kate E. Reinecke <50168367+kreinecke@users.noreply.github.com>
54a7c7d
to
7c0fda1
Compare
Signed-off-by: Kate E. Reinecke <50168367+kreinecke@users.noreply.github.com>
7c0fda1
to
892fb95
Compare
Signed-off-by: Kate E. Reinecke <50168367+kreinecke@users.noreply.github.com>
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.
Just a few comments.
createLocalUsers += "user_scope VARCHAR(36), " | ||
createLocalUsers += "last_login TIMESTAMP, " | ||
createLocalUsers += "last_updated TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP);" | ||
createLocalUsers += "CREATE TRIGGER update_last_updated AFTER UPDATE ON local_users BEGIN UPDATE local_users SET last_updated = DATETIME('now') WHERE _rowid_ = new._rowid_; END;" |
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 add a primary key to the table.
Also, does this CREATE TRIGGER syntax work with Postgres? Seems okay on SQLite and MySQL from what I can tell.
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.
Thanks - it is indeed not valid for postgres - I will add alternative trigger definition for that case.
INVITE_USER_CLIENT_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.
Do you want to comment out the values before we merge?
@@ -773,6 +784,10 @@ func (p *portalProxy) registerRoutes(e *echo.Echo, addSetupMiddleware *setupMidd | |||
pp.GET("/v1/auth/sso_login", p.initSSOlogin) | |||
pp.GET("/v1/auth/sso_logout", p.ssoLogoutOfUAA) | |||
|
|||
// Local User login/logout | |||
pp.POST("/v1/auth/local_login", p.localLogin) | |||
pp.POST("/v1/auth/local_logout", p.logout) |
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.
Do you think the local routes and remote routes should check if the corresponding auth type matches and response with 404 ?
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.
Yes. Will add.
@@ -10,6 +10,7 @@ import ( | |||
func init() { | |||
RegisterMigration(20170818162837, "SetupSchema", func(txn *sql.Tx, conf *goose.DBConf) error { | |||
consoleConfigTable := "CREATE TABLE IF NOT EXISTS console_config (" | |||
consoleConfigTable += " auth_endpoint_type VARCHAR(255) NOT NULL, " |
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 can't change a migration that has already been published - you'll need to move this to an ALTER statement in your new migration 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.
Thanks. Done. Will push with remaining changes.
Signed-off-by: Kate E. Reinecke <50168367+kreinecke@users.noreply.github.com>
Signed-off-by: Kate E. Reinecke <50168367+kreinecke@users.noreply.github.com>
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.
Changes LGTM - will try it out
I will make a start on documenting the feature and config on a separate branch. That can be merged when the front end PR is complete. In the meantime if it's not clear when you try it out, let me know. |
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.
LGTM
Add an option in jetstream config to set up a user for local authentication as an alternative to remote auth. Will eventually remove the mandatory provisioning of a UAA.