Skip to content
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

Merged
merged 38 commits into from
Mar 20, 2025

Conversation

praem90
Copy link
Contributor

@praem90 praem90 commented Mar 16, 2025

📑 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

	config.Add("auth", map[string]any{
		// Authentication Defaults
		"defaults": map[string]any{
			"guard": "user",
		},

		// Authentication Guards
		"guards": map[string]any{
			"user": map[string]any{
				"driver":   "jwt",
				"provider": "user",
			},
		},

                // User Providers
		"providers": map[string]any{
			"user": map[string]any{
				"driver": "orm",
			},
		},
	})

contract/auth

package auth

import (
	"time"
)

type GuardDriver interface {
	// Check whether user logged in or not
	Check() bool
	// Check whether user *not* logged in or not | !Check()
	Guest() bool
	// User returns the current authenticated user.
	User(user any) error
	// ID returns the current user id.
	ID() (token string, err error)
	// Login logs a user into the application.
	Login(user any) (token string, err error)
	// LoginUsingID logs the given user ID into the application.
	LoginUsingID(id any) (token string, err error)
	// Parse the given token.
	Parse(token string) (*Payload, error)
	// Refresh the token for the current user.
	Refresh() (token string, err error)
	// Logout logs the user out of the application.
	Logout() error
}

type Auth interface {
	GuardDriver
	Guard(name string) (GuardDriver, error)
	Extend(name string, fn GuardFunc)
	Provider(name string, fn UserProviderFunc)
}

type UserProvider interface {
	RetriveByID(user any, id any) error
	GetID(user any) any
}

type UserProviderFunc func(auth Auth) (UserProvider, error)
type GuardFunc func(name string, auth Auth, userProvider UserProvider) (guard GuardDriver, err error)

✅ Checks

  • Added test cases for my code

@praem90 praem90 requested a review from a team as a code owner March 16, 2025 18:07
Copy link

codecov bot commented Mar 16, 2025

Codecov Report

Attention: Patch coverage is 87.59124% with 34 lines in your changes missing coverage. Please review.

Project coverage is 69.31%. Comparing base (ab139f2) to head (9672566).
Report is 8 commits behind head on master.

Files with missing lines Patch % Lines
auth/jwt_guard.go 90.95% 12 Missing and 6 partials ⚠️
auth/auth.go 77.27% 9 Missing and 6 partials ⚠️
auth/service_provider.go 0.00% 1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@hwbrzzl
Copy link
Contributor

hwbrzzl commented Mar 17, 2025

Thanks, checking

Copy link
Contributor

@hwbrzzl hwbrzzl left a 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.

@praem90
Copy link
Contributor Author

praem90 commented Mar 17, 2025

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.

Copy link
Contributor

@hwbrzzl hwbrzzl left a comment

Choose a reason for hiding this comment

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

Great improvement 👍

@praem90 praem90 requested a review from hwbrzzl March 18, 2025 18:07
@praem90
Copy link
Contributor Author

praem90 commented Mar 19, 2025

The existing Refresh token flow. Note down that this func does not check for Token's length.

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
}

@praem90 praem90 requested a review from hwbrzzl March 19, 2025 08:55
Copy link
Contributor

@hwbrzzl hwbrzzl left a comment

Choose a reason for hiding this comment

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

Thanks! Almost complete.

@praem90 praem90 requested a review from hwbrzzl March 19, 2025 16:08
Copy link
Contributor

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?

Copy link
Contributor Author

@praem90 praem90 Mar 20, 2025

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.

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@praem90 praem90 requested a review from hwbrzzl March 20, 2025 07:12
Copy link
Contributor

@hwbrzzl hwbrzzl left a 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. 💥

@hwbrzzl hwbrzzl merged commit f8c37dd into goravel:master Mar 20, 2025
16 of 17 checks passed
@praem90
Copy link
Contributor Author

praem90 commented Mar 20, 2025

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.

@hwbrzzl
Copy link
Contributor

hwbrzzl commented Mar 20, 2025

Could you update the description based on the latest logic?

@praem90
Copy link
Contributor Author

praem90 commented Mar 20, 2025

I think we might need to update the goravel/goravel's config/auth.go to reflect the changes.

@hwbrzzl
Copy link
Contributor

hwbrzzl commented Mar 20, 2025

Yes, please create a PR for goravel/goravel.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

✨ [Feature] The guard driver of Auth support custom driver
2 participants