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

Start up a new machine auth provider in the storage service #2534

Merged
merged 5 commits into from
Oct 18, 2021

Conversation

ishank011
Copy link
Contributor

No description provided.

@@ -37,7 +37,7 @@ func (c *cs3backend) GetUserByClaims(ctx context.Context, claim, value string, w
}

res, err := c.authProvider.Authenticate(ctx, &gateway.AuthenticateRequest{
Type: "machine",
Type: "bearer",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is that bearer now instead of machine but still using the machineAuthAPIKey?

Copy link
Contributor Author

@ishank011 ishank011 Sep 24, 2021

Choose a reason for hiding this comment

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

That's because machine is just a driver for bearer auth. https://github.com/owncloud/ocis/blob/master/storage/pkg/command/authbearer.go#L105-L117

And the auth registry recognizes only the bearer auth type https://github.com/owncloud/ocis/blob/master/storage/pkg/command/gateway.go#L157

I think we can somehow combine the bearer driver flag with the proxy backend one to make sure that we're only using machine in case the backend being used is cs3.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. Don't we need to change the flagset value to machine then? In my current understanding it would break if we don't since the default driver of the bearer provider is oidc.

Value: flags.OverrideDefaultString(cfg.Reva.AuthBearerConfig.Driver, "oidc"),

Copy link
Contributor

Choose a reason for hiding this comment

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

Or maybe start another auth provider along with the existing basic and bearer providers for the machine authentication.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, both ideas sound good. I wouldn't want to change the default flag value but I can add a check

if PROXY_ACCOUNT_BACKEND_TYPE == "cs3" && STORAGE_AUTH_BEARER_DRIVER != "machine"
    raise error

Or add a separate auth provider. Which one would you prefer?

Copy link
Contributor

Choose a reason for hiding this comment

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

In my opinion a separate auth provider would be cleaner.

@ishank011 ishank011 changed the title Add config to skip encoding user groups in reva tokens Start up a new machine auth provider in the storage service Oct 14, 2021
@ishank011 ishank011 marked this pull request as draft October 14, 2021 15:06
@sonarcloud
Copy link

sonarcloud bot commented Oct 18, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

12.2% 12.2% Coverage
20.5% 20.5% Duplication

@ishank011 ishank011 marked this pull request as ready for review October 18, 2021 08:11
Copy link
Contributor

@C0rby C0rby left a comment

Choose a reason for hiding this comment

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

LGTM

@C0rby C0rby merged commit c51a663 into owncloud:master Oct 18, 2021
ownclouders pushed a commit that referenced this pull request Oct 18, 2021
Merge: 669175b 375e912
Author: David Christofas <dchristofas@owncloud.com>
Date:   Mon Oct 18 10:55:52 2021 +0200

    Merge pull request #2534 from ishank011/token-fixes

    Start up a new machine auth provider in the storage service
@ishank011 ishank011 deleted the token-fixes branch October 18, 2021 09:55
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.

2 participants