-
Notifications
You must be signed in to change notification settings - Fork 91
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
feat: [#478] The guard driver of Auth support custom driver #959
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #959 +/- ##
==========================================
+ Coverage 69.15% 69.31% +0.16%
==========================================
Files 157 160 +3
Lines 10526 10713 +187
==========================================
+ Hits 7279 7426 +147
- Misses 2913 2951 +38
- Partials 334 336 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Thanks, checking |
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.
Great PR 👍 Could you add some usages in the description? It's better to understand how to use the latest logic.
Thanks for your feedback. I'll work on these changes requested. The idea is to create multiple auth drivers and user providers and decouple them from each other. So we could use it any combination. |
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.
Great improvement 👍
The existing func (a *JwtGuard) Refresh() (token string, err error) {
auth, ok := a.ctx.Value(ctxKey).(Guards)
if !ok || auth[a.guard] == nil {
return "", errors.AuthParseTokenFirst
}
guard, ok := auth[a.guard]
if !ok {
return "", errors.AuthParseTokenFirst
}
if guard.Claims == nil {
return "", errors.AuthParseTokenFirst
}
nowTime := carbon.Now()
refreshTtl := a.config.GetInt("jwt.refresh_ttl")
if refreshTtl == 0 {
// 100 years
refreshTtl = 60 * 24 * 365 * 100
}
expireTime := carbon.FromStdTime(guard.Claims.ExpiresAt.Time).AddMinutes(refreshTtl)
if nowTime.Gt(expireTime) {
return "", errors.AuthRefreshTimeExceeded
}
token, err = a.LoginUsingID(guard.Claims.Key)
if err != nil {
return "", err
}
return
} This is the new approach with code reuse in mind func (r *JwtGuard) Refresh() (token string, err error) {
guard, err := r.GetAuthToken()
if err != nil {
return "", err
}
nowTime := carbon.Now()
refreshTtl := r.config.GetInt("jwt.refresh_ttl")
if refreshTtl == 0 {
// 100 years
refreshTtl = 60 * 24 * 365 * 100
}
expireTime := carbon.FromStdTime(guard.Claims.ExpiresAt.Time).AddMinutes(refreshTtl)
if nowTime.Gt(expireTime) {
return "", errors.AuthRefreshTimeExceeded
}
token, err = r.LoginUsingID(guard.Claims.Key)
if err != nil {
return "", err
}
return
}
func (r *JwtGuard) GetAuthToken() (*AuthToken, error) {
guards, ok := r.ctx.Value(ctxKey).(Guards)
if !ok {
return nil, ErrorParseTokenFirst
}
return r.authToken(guards)
}
func (r *JwtGuard) authToken(guards Guards) (*AuthToken, error) {
guard, ok := guards[r.guard]
if !ok || guard == nil {
return nil, ErrorParseTokenFirst
}
if guard.Claims == nil {
return nil, errors.AuthParseTokenFirst
}
if guard.Claims.Key == "" {
return nil, errors.AuthInvalidKey
}
if guard.Token == "" { // <-- Since reusing the code, checking the token is required for other func
return nil, errors.AuthTokenExpired
}
return guard, nil
} |
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! Almost complete.
auth/jwt_guard_test.go
Outdated
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.
So sorry, I made a mistake here. Given we will implement the session driver in the future, the test cases will be used for both jwt and session drivers. Hence, the single jwt_guard_test.go
is unnecessary, we can only keep the auth_test.go
file. Could you please move this file back to auth_test.go
?
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.
No problem @hwbrzzl
I realize there's a lot involved, and I'm grateful for your patient and seamless approach.
What about the parsing, refresh func tests?
The session driver does not have Token
. We assert the in many places.
These might be not be compatible for the session
driver.
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, about the test cases for the session driver, we can optimize them when implementing it.
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.
For now, we can just move jwt_guard_test.go
back.
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.
Done
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.
Amazing PR, thanks 👍 I'll add you to the contributor list. And I'm looking forward to your next feature PR, you will become our core contributor. 💥
Thank you so much, @hwbrzzl , for your incredible patience and guidance throughout this process. I really appreciate your thorough reviews, helpful suggestions, and calm demeanor, even when I was stuck. I learned a lot from you, and I'm very grateful for your support in getting this PR merged. |
Could you update the description based on the latest logic? |
I think we might need to update the |
Yes, please create a PR for goravel/goravel. |
📑 Description
Closes goravel/goravel#478
Add support to custom Auth drivers and decouple the user provider.
This requires some changes in the config and the Auth library signature.
Config
contract/auth
✅ Checks