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

Add JImmAdmin field to User struct #1101

Merged
merged 5 commits into from
Nov 28, 2023

Conversation

kian99
Copy link
Contributor

@kian99 kian99 commented Nov 20, 2023

Description

Slight refactor to make our code more DRY. Instead of repeating checks like the following,

isControllerAdmin, err := openfga.IsAdministrator(ctx, user, j.ResourceTag())
	if err != nil {
		return errors.E(op, err)
	}

in every facade handler, we instead do the check once when we authenticate the user and pass this information along with the user.

Also made various function signatures more consistent by calling the user variable just u. In some places it was user and others u.

The main issue with this change is that it heavily affects our tests. Tests inside jujuapi are largely unaffected because these tests act like integration tests, opening up a client connection as any external client would. Tests inside the jimm package however are more deeply affected as they call JIMM's handler methods directly and expect the methods to perform an OpenFGA check to determine if the user is a superuser, so these tests required some tweaking to pass the user's admin info from the test itself. Overall I think this is actually a good thing as we can move closer to making jimm tests purely unit tests (though some methods still make calls to OpenFGA which we should consider mocking out), and then place integration tests inside jujuapi.

Engineering checklist

Check only items that apply

  • Documentation updated
  • Covered by unit tests
  • Covered by integration tests

Copy link
Collaborator

@alesstimec alesstimec left a comment

Choose a reason for hiding this comment

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

LGTM.. but.. i don't thing renaming user to u was worth it.. it just makes the PR harder to review and brings little to no value - i think calling variable user is better than u..

@@ -279,7 +279,7 @@ func (auth *JWTGenerator) MakeToken(ctx context.Context, permissionMap map[strin
// 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 (j *JIMM) CheckPermission(ctx context.Context, user *openfga.User, cachedPerms map[string]string, desiredPerms map[string]interface{}) (map[string]string, error) {
func (j *JIMM) CheckPermission(ctx context.Context, u *openfga.User, cachedPerms map[string]string, desiredPerms map[string]interface{}) (map[string]string, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

why? saving 4 characters is hardly worth it :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well the nice thing about having it consistent is that it's easier to rename now. So I'll change it to user.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree renaming u to user makes sense honestly

@@ -29,7 +29,8 @@ func NewUser(u *dbmodel.User, client *OFGAClient) *User {
// to check user's access rights to various resources.
type User struct {
*dbmodel.User
client *OFGAClient
client *OFGAClient
JimmAdmin bool
Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense to me, good job

Copy link
Contributor

@ale8k ale8k left a comment

Choose a reason for hiding this comment

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

lgtm, nice work

Copy link

⚠️ This PR contains unsigned commits. To get your PR merged, please sign those commits (git commit -S --amend --no-edit) and force push them to this branch (git push --force-with-lease).

If you're new to commit signing, there are different ways to set it up:

Sign commits with gpg

Follow the steps below to set up commit signing with gpg:

  1. Generate a GPG key
  2. Add the GPG key to your GitHub account
  3. Configure git to use your GPG key for commit signing
Sign commits with ssh-agent

Follow the steps below to set up commit signing with ssh-agent:

  1. Generate an SSH key and add it to ssh-agent
  2. Add the SSH key to your GitHub account
  3. Configure git to use your SSH key for commit signing
Sign commits with 1Password

You can also sign commits using 1Password, which lets you sign commits with biometrics without the signing key leaving the local 1Password process.

Learn how to use 1Password to sign your commits.

Watch the demo

1 similar comment
Copy link

⚠️ This PR contains unsigned commits. To get your PR merged, please sign those commits (git commit -S --amend --no-edit) and force push them to this branch (git push --force-with-lease).

If you're new to commit signing, there are different ways to set it up:

Sign commits with gpg

Follow the steps below to set up commit signing with gpg:

  1. Generate a GPG key
  2. Add the GPG key to your GitHub account
  3. Configure git to use your GPG key for commit signing
Sign commits with ssh-agent

Follow the steps below to set up commit signing with ssh-agent:

  1. Generate an SSH key and add it to ssh-agent
  2. Add the SSH key to your GitHub account
  3. Configure git to use your SSH key for commit signing
Sign commits with 1Password

You can also sign commits using 1Password, which lets you sign commits with biometrics without the signing key leaving the local 1Password process.

Learn how to use 1Password to sign your commits.

Watch the demo

@kian99 kian99 force-pushed the CSS-6089-controller-permission-check branch from 7ec991c to 679092c Compare November 28, 2023 07:20
@kian99 kian99 merged commit ec92aa0 into canonical:v3 Nov 28, 2023
4 checks passed
@kian99 kian99 deleted the CSS-6089-controller-permission-check branch November 28, 2023 08:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants