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

(#108) influx_auth: fix creation using resource name #109

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lennardk
Copy link

@lennardk lennardk commented Feb 23, 2024

influx_get doesn't support params since c195e50.

This feels like a candidate for an additional test creating tokens with a resource name referenced, however I'm not familiar with the test framework so I'm creating this PR without.

@lennardk lennardk requested review from bastelfreak and a team as code owners February 23, 2024 10:59
@CLAassistant
Copy link

CLAassistant commented Feb 23, 2024

CLA assistant check
All committers have signed the CLA.

@bastelfreak bastelfreak added the bug Something isn't working label Feb 23, 2024
@lennardk
Copy link
Author

Actually, More is needed. Don't know how I thought I succesfully tested it. More to come!

@m0dular
Copy link
Contributor

m0dular commented Mar 4, 2024

Hi @lennardk, is there an issue you're seeing when trying to create or manage tokens? I looked at this and I don't remember what the intent of the code here was, but it doesn't ever actually call influxdb_get there. The map just ends up returning the permissions array as-is and never calls the influxdb_get method. I deleted this block and changed the body to use

permissions: should[:permissions],

instead and it works fine. We should probably make this change since there is some dead code here which could be misleading, but is there an issue you're trying to fix with this PR?

@lennardk
Copy link
Author

lennardk commented Mar 4, 2024

Hi,

Hi @lennardk, is there an issue you're seeing when trying to create or manage tokens
Yes, issue is linked in commit message. With the current code in place, it errors out when trying to create permissions on specific resources (buckets, in my case).

I looked at this and I don't remember what the intent of the code here was, but it doesn't ever actually call influxdb_get there. The map just ends up returning the permissions array as-is and never calls the influxdb_get method. I deleted this block and changed the body to use

permissions: should[:permissions],

instead and it works fine. We should probably make this change since there is some dead code here which could be misleading, but is there an issue you're trying to fix with this PR?
You have a point here, just straight passing the name, maybe adding the org, without adding the ID might be just as valid. That would make things easier. I can't test it right away myself but I'll have a look in the next few days.

@m0dular
Copy link
Contributor

m0dular commented Mar 11, 2024

Ah, I think I see the problem. When you try to create a more restrictive set of permissions, like

      permissions      => [
        {
          'action'   => 'read',
          'resource' => {
            'type'   => 'buckets',
            "name"   => "puppet_data",
            "org"   => "puppetlabs",
          }
        },
    ]

we need to get the IDs of the bucket and org to use in the POST to create the token. I'm not sure if that ever worked properly, but we can start with specifically named buckets since that's probably the most common use case. The code in the current PR seems to account for the bucket name, but not the org name. I'll see if we can just grab the IDs from the cache.

@m0dular
Copy link
Contributor

m0dular commented Mar 11, 2024

Turns out the "org" => "puppetlabs" item doesn't need to be inside the actual resource part of the permission. So the PR works, but I think we could use the bucket cache to make it a bit cleaner. I'll start a review.

@m0dular
Copy link
Contributor

m0dular commented Mar 12, 2024

Hi @lennardk, I opened #110 to fix this and added a unit test there. Let us know if that works for you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants