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.

Sign up for GitHub

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

Refactors the JWT token generator and adds unit tests for it. #1061

Merged
merged 4 commits into from
Oct 17, 2023
Merged

Refactors the JWT token generator and adds unit tests for it. #1061

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
37a9263
Refactors the JWT token generator and adds unit tests for it.
alesstimec Oct 13, 2023
735057d
Deduplicate clouds before checking user's access level.
alesstimec Oct 16, 2023
1d4e128
Restructured the access_test.go
alesstimec Oct 17, 2023
b3d3b9a
Addressed the comment by ale8k.
alesstimec Oct 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
166 changes: 119 additions & 47 deletions internal/jimm/access.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,12 @@ package jimm
import (
"context"
"fmt"
"sync"

jujuparams "github.com/juju/juju/rpc/params"
"github.com/juju/names/v4"
"github.com/juju/zaputil/zapctx"
"go.uber.org/zap"

"github.com/canonical/jimm/internal/dbmodel"
"github.com/canonical/jimm/internal/errors"
Expand Down Expand Up @@ -111,73 +114,142 @@ func ToOfferRelation(accessLevel string) (openfga.Relation, error) {
}
}

// JwtGenerator provides the necessary state and methods to authorize a user and generate JWT tokens.
type JwtGenerator struct {
jimm *JIMM
// JWTGeneratorDatabase specifies the database interface used by the
// JWT generator.
type JWTGeneratorDatabase interface {
GetController(ctx context.Context, controller *dbmodel.Controller) error
}

// JWTGeneratorAccessChecker specifies the access checker used by the JWT
// generator to obtain user's access rights to various entities.
type JWTGeneratorAccessChecker interface {
GetUserModelAccess(context.Context, *openfga.User, names.ModelTag) (string, error)
GetUserControllerAccess(context.Context, *openfga.User, names.ControllerTag) (string, error)
GetUserCloudAccess(context.Context, *openfga.User, names.CloudTag) (string, error)
CheckPermission(context.Context, *openfga.User, map[string]string, map[string]interface{}) (map[string]string, error)
}

// JWTService specifies the service JWT generator uses to generate JWTs.
type JWTService interface {
NewJWT(context.Context, jimmjwx.JWTParams) ([]byte, error)
}

// JWTGenerator provides the necessary state and methods to authorize a user and generate JWT tokens.
type JWTGenerator struct {
authenticator Authenticator
database JWTGeneratorDatabase
accessChecker JWTGeneratorAccessChecker
jwtService JWTService

mu sync.Mutex
accessMapCache map[string]string
mt names.ModelTag
ct names.ControllerTag
user *openfga.User
callCount int
}

// NewJwtGenerator returns a new JwtAuthorizer struct
func NewJwtGenerator(jimm *JIMM) JwtGenerator {
return JwtGenerator{jimm: jimm}
// NewJWTGenerator returns a new JwtAuthorizer struct
func NewJWTGenerator(authenticator Authenticator, database JWTGeneratorDatabase, accessChecker JWTGeneratorAccessChecker, jwtService JWTService) JWTGenerator {
return JWTGenerator{
authenticator: authenticator,
database: database,
accessChecker: accessChecker,
jwtService: jwtService,
}
}

// SetTags implements TokenGenerator
func (auth *JwtGenerator) SetTags(mt names.ModelTag, ct names.ControllerTag) {
func (auth *JWTGenerator) SetTags(mt names.ModelTag, ct names.ControllerTag) {
auth.mt = mt
auth.ct = ct
}

// SetTags implements TokenGenerator
func (auth *JwtGenerator) GetUser() names.UserTag {
func (auth *JWTGenerator) GetUser() names.UserTag {
if auth.user != nil {
return auth.user.ResourceTag()
}
return names.UserTag{}
}

// MakeToken takes a login request and a map of needed permissions and returns a JWT token if the user satisfies
// all the needed permissions. A loginRequest object should be provided on the first invocation of this function
// after which point subsequent calls can provide a nil object.
// Note that this function is not thread-safe and should only be called by a single Go routine at a time.
func (auth *JwtGenerator) MakeToken(ctx context.Context, initialLogin bool, req *jujuparams.LoginRequest, permissionMap map[string]interface{}) ([]byte, error) {
const op = errors.Op("jimm.MakeToken")
// MakeLoginToken authorizes the user based on the provided login requests and returns
// a JWT containing claims about user's access to the controller, model (if applicable)
// and all clouds that the controller knows about.
func (auth *JWTGenerator) MakeLoginToken(ctx context.Context, req *jujuparams.LoginRequest) ([]byte, error) {
const op = errors.Op("jimm.MakeLoginToken")

if initialLogin {
if req == nil {
return nil, errors.E(op, "Missing login request.")
}
// Recreate the accessMapCache to prevent leaking permissions across multiple login requests.
auth.accessMapCache = make(map[string]string)
var authErr error
auth.user, authErr = auth.jimm.Authenticator.Authenticate(ctx, req)
if authErr != nil {
return nil, authErr
}
var modelAccess string
if auth.mt.Id() == "" {
return nil, errors.E(op, "Desired Model not set")
}
modelAccess, authErr = auth.jimm.GetUserModelAccess(ctx, auth.user, auth.mt)
if authErr != nil {
return nil, authErr
}
auth.accessMapCache[auth.mt.String()] = modelAccess
auth.mu.Lock()
defer auth.mu.Unlock()

if auth.ct.Id() == "" {
return nil, errors.E(op, "Desired Controller not set")
}
var controllerAccess string
controllerAccess, authErr = auth.jimm.GetControllerAccess(ctx, auth.user, auth.ct)
if authErr != nil {
return nil, authErr
if req == nil {
return nil, errors.E(op, "missing login request.")
}
// Recreate the accessMapCache to prevent leaking permissions across multiple login requests.
auth.accessMapCache = make(map[string]string)
var authErr error
auth.user, authErr = auth.authenticator.Authenticate(ctx, req)
if authErr != nil {
zapctx.Error(ctx, "authentication failed", zap.Error(authErr))
return nil, authErr
}
var modelAccess string
if auth.mt.Id() == "" {
return nil, errors.E(op, "model not set")
}
modelAccess, authErr = auth.accessChecker.GetUserModelAccess(ctx, auth.user, auth.mt)
if authErr != nil {
zapctx.Error(ctx, "model access check failed", zap.Error(authErr))
return nil, authErr
}
auth.accessMapCache[auth.mt.String()] = modelAccess

if auth.ct.Id() == "" {
return nil, errors.E(op, "controller not set")
}
var controllerAccess string
controllerAccess, authErr = auth.accessChecker.GetUserControllerAccess(ctx, auth.user, auth.ct)
if authErr != nil {
return nil, authErr
}
auth.accessMapCache[auth.ct.String()] = controllerAccess

var ctl dbmodel.Controller
ctl.SetTag(auth.ct)
err := auth.database.GetController(ctx, &ctl)
if err != nil {
zapctx.Error(ctx, "failed to fetch controller", zap.Error(err))
return nil, errors.E(op, "failed to fetch controller", err)
}
clouds := make(map[names.CloudTag]bool)
for _, cloudRegion := range ctl.CloudRegions {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are cloud regions what we want here instead of just clouds? There could be quite a lot of cloudRegions versus just Clouds right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

well.. these are named cloud regions.. but what they are is cloud region priorities.. so each cloud region will have a priority set on a controller.. usually each controller won't have more than one priority entry per cloud region.. but you are correct.. i really should collect just clouds to reduce potential duplicate checks

clouds[cloudRegion.CloudRegion.Cloud.ResourceTag()] = true
}
for cloudTag, _ := range clouds {
accessLevel, err := auth.accessChecker.GetUserCloudAccess(ctx, auth.user, cloudTag)
if err != nil {
zapctx.Error(ctx, "cloud access check failed", zap.Error(err))
return nil, errors.E(op, "failed to check user's cloud access", err)
}
auth.accessMapCache[auth.ct.String()] = controllerAccess
auth.accessMapCache[cloudTag.String()] = accessLevel
}

return auth.jwtService.NewJWT(ctx, jimmjwx.JWTParams{
Controller: auth.ct.Id(),
User: auth.user.Tag().String(),
Access: auth.accessMapCache,
})
}

// MakeToken assumes MakeLoginToken has already been called and checks the permissions
// specified in the permissionMap. If the logged in user has all those permissions
// a JWT will be returned with assertions confirming all those permissions.
func (auth *JWTGenerator) MakeToken(ctx context.Context, permissionMap map[string]interface{}) ([]byte, error) {
const op = errors.Op("jimm.MakeToken")

auth.mu.Lock()
defer auth.mu.Unlock()

if auth.callCount >= 10 {
return nil, errors.E(op, "Permission check limit exceeded")
}
Expand All @@ -187,12 +259,12 @@ func (auth *JwtGenerator) MakeToken(ctx context.Context, initialLogin bool, req
}
if permissionMap != nil {
var err error
auth.accessMapCache, err = checkPermission(ctx, auth.user, auth.accessMapCache, permissionMap)
auth.accessMapCache, err = auth.accessChecker.CheckPermission(ctx, auth.user, auth.accessMapCache, permissionMap)
if err != nil {
return nil, err
}
}
jwt, err := auth.jimm.JWTService.NewJWT(ctx, jimmjwx.JWTParams{
jwt, err := auth.jwtService.NewJWT(ctx, jimmjwx.JWTParams{
Controller: auth.ct.Id(),
User: auth.user.Tag().String(),
Access: auth.accessMapCache,
Expand All @@ -203,12 +275,12 @@ func (auth *JwtGenerator) MakeToken(ctx context.Context, initialLogin bool, req
return jwt, nil
}

// checkPermission loops over the desired permissions in desiredPerms and adds these permissions
// CheckPermission loops over the desired permissions in desiredPerms and adds these permissions
// to cachedPerms if they exist. If the user does not have any of the desired permissions then an
// error is returned.
// Note that cachedPerms map is modified and returned.
func checkPermission(ctx context.Context, user *openfga.User, cachedPerms map[string]string, desiredPerms map[string]interface{}) (map[string]string, error) {
const op = errors.Op("jimm.checkPermission")
func (j *JIMM) CheckPermission(ctx context.Context, user *openfga.User, cachedPerms map[string]string, desiredPerms map[string]interface{}) (map[string]string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to ensure we have tests for this function too, but I can add that in a follow up if we don't

const op = errors.Op("jimm.CheckPermission")
for key, val := range desiredPerms {
if _, ok := cachedPerms[key]; !ok {
stringVal, ok := val.(string)
Expand Down
Loading