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

refactor: authz provider implementation and authz users basic implementation #4676

Merged
merged 20 commits into from
Aug 4, 2022

Conversation

NicholasBlaskey
Copy link
Contributor

Description

Refactor the user api both echo and grpc to use the new authz pattern.

Test Plan

existing e2e tests + new integration tests

Commentary (optional)

Checklist

  • User-facing API changes need the "User-facing API Change" label.
  • Release notes should be added as a separate file under docs/release-notes/.
    See Release Note for details.
  • Licenses should be included for new code which was copied and/or modified from any external code.
  • If modifying /webui/react/src/shared/ verify make -C webui/react test-shared passes.

@netlify
Copy link

netlify bot commented Jul 29, 2022

Deploy Preview for determined-ui canceled.

Name Link
🔨 Latest commit 4373c8d
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/62eaef3fad73650008960767

Copy link
Contributor

@eecsliu eecsliu left a comment

Choose a reason for hiding this comment

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

Looks good! Couple of questions below

}
return nil, errors.Wrap(grpcutil.ErrPermissionDenied, err.Error())
}

Copy link
Contributor

Choose a reason for hiding this comment

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

no check for if the user can patch other users?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PatchUser in grpc I think only allows for setting a users display name currently

The check for setting other use display names is in CanSetUsersDisplayName I think?

return
}

knownAuthZTypes = make(map[string]bool)
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like knownAuthZTypes can be remade if this is called more than once? Should be locked down with a once like in the provider

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a nil check for knownAuthZTypes above -- which I missed for like a couple minutes wondering how it even worked lol

Copy link
Contributor

Choose a reason for hiding this comment

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

oh lmao I'm just blind

@eecsliu
Copy link
Contributor

eecsliu commented Aug 1, 2022

Is this dependent on Ilia's authz work? Would you want to merge this into rbac-user-groups branch on top of his changes before merging all of the user groups stuff into master together?

@NicholasBlaskey
Copy link
Contributor Author

Is this dependent on Ilia's authz work? Would you want to merge this into rbac-user-groups branch on top of his changes before merging all of the user groups stuff into master together?

Ilia's authz work is already in this PR and I think I would rather to keep commit history cleaner keeping this change separate?

Also I think I changed my mind about CanGetUser not returning an error. I am thinking it would be pretty bad UX for a user to like try to do an operation on another user and get not found when they should get a database failure error. I'm going to make CanGetUser return (canViewUser bool, serverError error) to try and make it clear that an error shouldn't be returned for denying viewing a user.

@NicholasBlaskey NicholasBlaskey merged commit aad8011 into determined-ai:master Aug 4, 2022
@dannysauer dannysauer modified the milestones: 0.0.102, 0.19.1 Feb 6, 2024
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.

4 participants