WIP - generic auth port #9
Conversation
|
|
||
| import ( | ||
| "flamingo.me/dingo" | ||
| interfaces2 "flamingo.me/flamingo/v3/core/auth/interfaces" |
There was a problem hiding this comment.
AFAIS there's no need to use the alias.
| interfaces2 "flamingo.me/flamingo/v3/core/auth/interfaces" | |
| "flamingo.me/flamingo/v3/core/auth/interfaces" |
|
|
||
| type ( | ||
| // Idendity information | ||
| Idendity interface { |
| Roles() []securityDomain.Role | ||
| } | ||
|
|
||
| //SimpleUser - implements User and can be used |
There was a problem hiding this comment.
As we will use godoc please format the comments like
// SimpleUser is a default implementation of the User interface...
and not
//SimpleUser - implements User ...
|
|
||
| //SimpleUser - implements User and can be used | ||
| SimpleUser struct { | ||
| SubjectVal string |
|
|
||
|
|
||
| // CustomField - get a customfield by key | ||
| func (u *SimpleUser) CustomField(key string) (string,bool) { |
There was a problem hiding this comment.
Simplify method by doing
val, ok := u.CustomFieldVal[key]
return val, ok|
|
||
| //AuthAction - default auth action - starting the authorsation with the registered Authservice | ||
| func (c *Authcontroller) AuthAction(ctx context.Context, r *web.Request) web.Result { | ||
| redirecturl, ok := r.Params["redirecturl"] |
There was a problem hiding this comment.
You can safely omit the ok variable here, the default value for string is ""
|
|
||
| // Inject for Authservice | ||
| func (o *Authservice) Inject(responder *web.Responder, config *struct { | ||
| Users config.Slice `inject:"config:basicauth.users,optional"` |
There was a problem hiding this comment.
Maybe users should be provided via something like a UserProvider? So the credentials do not have to be in the config?
|
|
||
| func (a *Authservice) IsAuthenticated(ctx context.Context, r *web.Request) bool { | ||
| _, _, ok := r.Request().BasicAuth() | ||
| //TODO check user pw |
| EmailVal *string | ||
| NameVal *string | ||
| CustomFieldVal map[string]string | ||
| DefaultRole string |
There was a problem hiding this comment.
What is the DefaultRole?
There was a problem hiding this comment.
Couple of remainders:
- AuthStarted/AuthStart and AuthDestroyed/SessionDestroyed events.
- New RoleProvider in auth module to add read roles from User and returns them.
- Unit testing - in general, we introduced interfaces everywhere, so it should not be a problem.
- Fake adapters for different AuthServices and maybe generic FakeUserRepository (UserProvider) in auth module which can provide functionality for reading user from csv/yaml, so other fake adapters can use it to provide user data depending on credentials.
| Groups []string | ||
| User interface { | ||
| Subject() string | ||
| Email() (string,bool) |
There was a problem hiding this comment.
I would go with single return argument, with default value as empty string. In general it would be easier for further usage and easy to check for empty string if it's needed.
| type ( | ||
| //Authservice - generic Authservice interface that should be used | ||
| Authservice interface { | ||
| Authenticate(ctx context.Context, returnURL *url.URL) (web.Result, error) |
There was a problem hiding this comment.
As remainder to add web.Request in a list of arguments. We can omit to pass returnUrl - making of returnUrl is provided in security middleware, and it can be from header referrer or defined path in config (but, it doesn't hurt if we keep it).
| ) | ||
|
|
||
| type ( | ||
| Authcontroller struct { |
There was a problem hiding this comment.
Camel case: "AuthController"
| type ( | ||
| Authcontroller struct { | ||
| responder *web.Responder | ||
| authservice Authservice |
There was a problem hiding this comment.
Camel cases and map of services: authServices map[string]AuthService
| redirecturl = r.Request().Referer() | ||
| } | ||
|
|
||
| if refURL, err := url.Parse(redirecturl); err != nil || refURL.Host != r.Request().Host { |
There was a problem hiding this comment.
I'm not sure should we have this logic here: there is same functionality in security middleware, so we can either move that here completely or we just read "redirecturl" Param and pass url or nil as argument.
| return c.responder.ServerError(errors.New("wrong redirect url given")) | ||
| } | ||
| return result | ||
| } |
There was a problem hiding this comment.
Maybe we can add also Logout action?
| @@ -0,0 +1,74 @@ | |||
| package interfaces | |||
There was a problem hiding this comment.
Reminder: to have basicauth as module in separate repository.
|
Superseded by #50 |
# Please enter the commit message for your changes. Lines starting # with '#' will be kept; you may remove them yourself if you want to. # An empty message aborts the commit. # # Date: Tue Jan 2 10:05:29 2018 +0100 # # On branch 9-circuit-breaker # Changes to be committed: # new file: core/circuitbreaker/circuitbreaker.go #
Resolve "framework/core?: circuitbreaker support" Closes #9 See merge request shared/flamingo/flamingo!30
No description provided.