Skip to content
This repository has been archived by the owner on Jan 24, 2023. It is now read-only.

Add support for local auth configuration #3632

Merged
merged 32 commits into from
Jul 6, 2019
Merged

Conversation

kreinecke
Copy link
Contributor

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.

@cfdreddbot
Copy link

✅ Hey kreinecke! The commit authors and yourself have already signed the CLA.

@codecov
Copy link

codecov bot commented Jun 5, 2019

Codecov Report

Merging #3632 into v2-master will increase coverage by <.01%.
The diff coverage is 28.57%.

@@              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

@kreinecke kreinecke force-pushed the local-admin-option branch 4 times, most recently from d6f056c to ad10abb Compare June 7, 2019 08:21
@kreinecke
Copy link
Contributor Author

NOTE default.config.properties should be excluded from merge until the corresponding front-end changes have been completed.

INVITE_USER_CLIENT_SECRET=
Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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.

@kreinecke kreinecke requested review from nwmac and removed request for nwmac June 7, 2019 13:00
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>
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>
@nwmac nwmac removed the conflicts Merge conflicts on PR label Jun 17, 2019
Copy link
Contributor

@nwmac nwmac left a 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;"
Copy link
Contributor

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.

Copy link
Contributor Author

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=
Copy link
Contributor

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)
Copy link
Contributor

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 ?

Copy link
Contributor Author

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, "
Copy link
Contributor

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.

Copy link
Contributor Author

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>
Copy link
Contributor

@nwmac nwmac left a 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

@kreinecke
Copy link
Contributor Author

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.

Copy link
Contributor

@nwmac nwmac left a comment

Choose a reason for hiding this comment

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

LGTM

@nwmac nwmac merged commit 26c3b78 into v2-master Jul 6, 2019
@nwmac nwmac deleted the local-admin-option branch July 6, 2019 09:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants