-
Notifications
You must be signed in to change notification settings - Fork 86
Redirect login, logout, account linking requests from WIT to AUTH #1650
Conversation
@alexeykazakov snapshot fabric8-wit image is available for testing. |
[test] |
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.
Code looks fine, a lot of code gone too - already feels light.
will test it locally now.
controller/login.go
Outdated
|
||
brokerEndpoint, err := c.configuration.GetKeycloakEndpointBroker(ctx.Request) | ||
func RedirectLocation(params url.Values, location string) (string, error) { |
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 like this is being used in the controller
package only , we needn't have it as an exported method then.
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.
Agree.
Done.
@@ -600,6 +625,104 @@ func (c *UsersController) Update(ctx *app.UpdateUsersContext) error { | |||
return returnResponse | |||
} | |||
|
|||
func (c *UsersController) createAuthUpdateClient(ctx *app.UpdateUsersContext) (*authservice.Client, error) { |
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.
fine for now, but we should consolidate the client initialization methods somewhere in future, for re-use.
return client, nil | ||
} | ||
|
||
func (c *UsersController) updateInAuth(ctx *app.UpdateUsersContext) (*app.User, error) { |
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 too should be consolidated and maintained as part of a separate interface spec , something like RemoteAuthService
- but definitely fine for now.
@alexeykazakov snapshot fabric8-wit image is available for testing. |
@alexeykazakov snapshot fabric8-wit image is available for testing. |
Tested out , looks fine -
Found a couple of issues one during login where I was getting a Both the fixes are part of fabric8-services/fabric8-auth#112 |
[test] |
[test] |
Codecov Report
@@ Coverage Diff @@
## master #1650 +/- ##
==========================================
+ Coverage 54.21% 54.36% +0.15%
==========================================
Files 130 129 -1
Lines 16720 16304 -416
==========================================
- Hits 9065 8864 -201
+ Misses 6953 6753 -200
+ Partials 702 687 -15
Continue to review full report at Codecov.
|
Part of fabric8-services/fabric8-auth#79