Authentication plugin #207
Conversation
|
Sorry I am experiencing some personal health issues these days, I think I'll come back to keep on with the work here this weekend. |
| @@ -0,0 +1,35 @@ | |||
| package basicauthenticator | |||
There was a problem hiding this comment.
can we name this auth/internal/authenticator.go? While it does use basic auth, other providers can use it too, for obtaining credentials.
There was a problem hiding this comment.
/internal/ in the import path would prevent other packages to import this. I am not quite sure on what you want to achieve here. Could you please explain further? Thanks
|
|
||
| // RequireAdmin returns a gin middleware which requires a client token or basic authentication header to be supplied | ||
| func (a *Auth) getUserInfoFromAuth(ctx *gin.Context) (user *model.User, authenticator string, err error) { | ||
| designatedAuthenticator := ctx.GetHeader("X-Gotify-Authenticator") |
There was a problem hiding this comment.
Is it now required to specify this header? This creates a security risk because a user that exists in our internal auth, could be authenticated via a different origin.
I thought we go through the authenticators one by one and try to authenticate the user.
- if wrong password -> abort request
- if unknown user -> goto next authenticator
There was a problem hiding this comment.
I think we are on the same track.
if wrong password -> abort request
Authenticators can return an error, this would abort the authentication process.
if unknown user -> goto next authenticator
On line 48, if designatedAuthenticator == "", all authenticator would be tried sequentially, otherwise, only authenticators with matching name will be used.
There was a problem hiding this comment.
Ahh, I missed the condition where user and err is nil, can we make this more explicit? Could be by returning a named error like ErrUserNotFound = errors.new("user not found"), or by adding another else branch
} else if err != nil {
return nil, providerName, err
} else {
// continue with next provider
}
There was a problem hiding this comment.
I think we can't let external plugins import our auth package as it would introduce an import loop. The way around, as I see, would be to introduce another method to get whether this is a fatal authentication error in our error interface like maybe Abort() bool and default this to false if this method is missing.
There was a problem hiding this comment.
The error constant could be inside the plugin api. But I'm okay with both ways.
There was a problem hiding this comment.
I think an error constant inside the plugin API is the better way because it's easier to extend the API afterwards without breaking existing code.
There was a problem hiding this comment.
I don't think it is necessary to introduce another common api package just for authenticators as we did in user plugins, as we don't have as many data types to take care of.
| Plugins []PluginConf | ||
| ID uint `gorm:"primary_key;unique_index;AUTO_INCREMENT"` | ||
| Name string `gorm:"type:varchar(180);unique_index"` | ||
| Authenticator string `gorm:"type:varchar(100);index"` |
There was a problem hiding this comment.
I'd prefer the name Origin. Also, is the index needed?
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if user.ID == 0 { |
There was a problem hiding this comment.
If the user does exist, we need to verify that user.Authenticator == authenticator.
|
|
||
| // Code implements AuthenticationError | ||
| func (e TokenRequiredError) Code() int { | ||
| return 400 |
There was a problem hiding this comment.
403 Forbidden because the user provided valid authentication, but is not authorized.
| @@ -33,14 +33,14 @@ func (s *UtilSuite) Test_getID() { | |||
|
|
|||
| func (s *UtilSuite) Test_getToken() { | |||
| ctx, _ := gin.CreateTestContext(httptest.NewRecorder()) | |||
| RegisterAuthentication(ctx, nil, 1, "asdasda") | |||
| RegisterAuthentication(ctx, nil, "asdasda") | |||
There was a problem hiding this comment.
nil, as user isn't allowed anymore, there are a lot occurrences of this in tests.
There was a problem hiding this comment.
Sorry could you please explain this, which should be nil?
|
Feel free to reopen this pr, if its ready for another review. |
|
@eternal-flame-AD can I help you on this? |
|
Yes! I'm having a tight schedule these days so I expect not being able to work extensively on this project for ~2-3 mo. I will be very grateful if u can fork this and make this happen! :) I'll be available for discussions and reviews tho. |
Closes #203.