Skip to content

Commit

Permalink
Merge pull request juju#17292 from manadart/dqlite-check-creds
Browse files Browse the repository at this point in the history
juju#17292

This patch constitutes some progress towards using the permissions domain for login authorisation. It is necessarily large due to the number of tests affected by changing the data source for this logic.

We use the `ID` method instead of the `Name` method on user tags in order to preserve the `@domain` section for external users. Such users will reside in the user table, where under Mongo they had an entry only in the model or controller users collections.

In several places we have migrated permission checks to the new access service backing.
- The permission delegator now uses the access service.
- The login authenticator now uses the access service
- Introspection auth checking now shims the access service for permissions checking.

There are a couple of to-do comments regarding:
- Checking for controller access by controller tag (UUID).
- Having to check access for `everyone@external` by direct service access.

The commented block for testing the last user login will be reinstated in a patch to come.

To keep this patch manageable, these changes should follow:
- Rework of the last user login so that a failed login does not indicate a connection to the controller.
- Work towards removal of `state.State` and legacy state model from `serveConn` down through `apiHandler`.
- Ensuring that we can add external users implicitly when they are granted permissions.
- Refactoring common `UserAccessFunc` to use a signature compatible with the access service.

## QA steps
- `juju bootstrap lxd lxd --debug --build-agent`
- `juju add-model work`
- `juju change-user-password admin`
- `juju add-user joe`
- `juju change-user-password joe`
- `juju logout`
- `juju login -u joe -c lxd`
- Should not be able to see any models.
- `juju logout`
- `juju login -u admin -c lxd`
- `juju grant joe admin work`
- `juju logout`
- `juju login -u joe -c lxd`
- Should now be able to access model.

## Documentation changes

None.

## Links

**Jira card:** [JUJU-5913](https://warthogs.atlassian.net/browse/JUJU-5913)

[JUJU-5913]: https://warthogs.atlassian.net/browse/JUJU-5913?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
  • Loading branch information
jujubot authored Jul 1, 2024
2 parents 6fe2248 + 4955292 commit 1eb9a36
Show file tree
Hide file tree
Showing 40 changed files with 887 additions and 727 deletions.
39 changes: 26 additions & 13 deletions apiserver/admin.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,12 @@ import (
"github.com/juju/juju/apiserver/facade"
"github.com/juju/juju/apiserver/observer"
"github.com/juju/juju/core/auditlog"
"github.com/juju/juju/core/database"
"github.com/juju/juju/core/network"
"github.com/juju/juju/core/permission"
"github.com/juju/juju/core/pinger"
"github.com/juju/juju/core/trace"
accesserrors "github.com/juju/juju/domain/access/errors"
"github.com/juju/juju/internal/rpcreflect"
"github.com/juju/juju/rpc"
"github.com/juju/juju/rpc/params"
Expand Down Expand Up @@ -298,6 +300,7 @@ func (a *admin) authenticate(ctx context.Context, req params.LoginRequest) (*aut
authenticated := false
for _, authenticator := range a.srv.loginAuthenticators {
var err error

authInfo, err = authenticator.AuthenticateLoginRequest(ctx, a.root.serverHost, modelUUID, authParams)
if errors.Is(err, errors.NotSupported) {
continue
Expand Down Expand Up @@ -401,7 +404,7 @@ func (a *admin) handleAuthError(err error) error {
if err, ok := errors.Cause(err).(*apiservererrors.DischargeRequiredError); ok {
return err
}
if a.maintenanceInProgress() {
if !a.srv.upgradeComplete() {
// An upgrade, migration or similar operation is in
// progress. It is possible for logins to fail until this
// is complete due to incomplete or updating data. Mask
Expand Down Expand Up @@ -453,16 +456,30 @@ func (a *admin) checkUserPermissions(authInfo authentication.AuthInfo, controlle
// everyone group.
everyoneGroupAccess := permission.NoAccess
if !userTag.IsLocal() {
everyoneTag := names.NewUserTag(common.EveryoneTagName)
everyoneGroupUser, err := state.ControllerAccess(a.root.state, everyoneTag)
if err != nil && !errors.Is(err, errors.NotFound) {

// TODO (manadart 2024-05-27): This is a huge wart.
// Having to acquire the service this way is an abysmal failure of
// encapsulation.
// The best option might be to round up all of these special case
// "everyone@external" checks and push them into either the permission
// delegator or the service itself.
accessService := a.srv.shared.serviceFactoryGetter.FactoryForModel(database.ControllerNS).Access()
everyoneGroupUser, err := accessService.ReadUserAccessForTarget(
context.TODO(),
common.EveryoneTagName,
permission.ID{
ObjectType: permission.Controller,
Key: "controller",
},
)
if err != nil && !errors.Is(err, accesserrors.UserNotFound) && !errors.Is(err, accesserrors.PermissionNotFound) {
return nil, errors.Annotatef(err, "obtaining ControllerUser for everyone group")
}
everyoneGroupAccess = everyoneGroupUser.Access
}

controllerAccess, err := authInfo.SubjectPermissions(a.root.state.ControllerTag())
if errors.Is(err, errors.NotFound) {
if errors.Is(err, accesserrors.PermissionNotFound) || errors.Is(err, accesserrors.UserNotFound) {
controllerAccess = everyoneGroupAccess
} else if err != nil {
return nil, errors.Annotatef(err, "obtaining ControllerUser for logged in user %s", userTag.Id())
Expand All @@ -476,10 +493,10 @@ func (a *admin) checkUserPermissions(authInfo authentication.AuthInfo, controlle

var err error
modelAccess, err = authInfo.SubjectPermissions(a.root.model.ModelTag())
if err != nil && controllerAccess != permission.SuperuserAccess {
return nil, errors.Wrap(err, apiservererrors.ErrPerm)
}
if err != nil && controllerAccess == permission.SuperuserAccess {
if err != nil {
if controllerAccess != permission.SuperuserAccess {
return nil, errors.Wrap(err, apiservererrors.ErrPerm)
}
modelAccess = permission.AdminAccess
}
}
Expand Down Expand Up @@ -529,10 +546,6 @@ func filterFacades(registry *facade.Registry, allowFacadeAllMustMatch ...facadeF
return out
}

func (a *admin) maintenanceInProgress() bool {
return !a.srv.upgradeComplete()
}

// PingRootHandler is the interface that the root handler must implement
// to allow the pinger to be registered.
type PingRootHandler interface {
Expand Down
Loading

0 comments on commit 1eb9a36

Please sign in to comment.