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

Service Accounts #3926

Merged
merged 6 commits into from
Aug 29, 2023
Merged

Service Accounts #3926

merged 6 commits into from
Aug 29, 2023

Conversation

kobergj
Copy link
Contributor

@kobergj kobergj commented Jun 1, 2023

Introduces service-account auth manager

ATTENTION: BREAKS OCIS TEMPORARILY

check owncloud/ocis#6427 before merging

@update-docs
Copy link

update-docs bot commented Jun 1, 2023

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@gmgigi96
Copy link
Member

gmgigi96 commented Jun 7, 2023

Interesting! Which is the context of this?

@kobergj
Copy link
Contributor Author

kobergj commented Aug 8, 2023

@gmgigi96 sorry, I completely missed your comment. Shame on me 😞

We want to have so called service-accounts. We want to use these for inter service communication, instead of impersonating users like we do currently.

Reason for this is that we do not have an active user in some parts of the code, but need to get spaces/filemetadata/users/groups/....

@kobergj kobergj force-pushed the ServiceAccounts branch 2 times, most recently from 5a08459 to ff46118 Compare August 11, 2023 12:35
Signed-off-by: jkoberg <jkoberg@owncloud.com>
Signed-off-by: jkoberg <jkoberg@owncloud.com>
Signed-off-by: jkoberg <jkoberg@owncloud.com>
Signed-off-by: jkoberg <jkoberg@owncloud.com>
@kobergj kobergj force-pushed the ServiceAccounts branch 3 times, most recently from b53cb69 to 64f0747 Compare August 17, 2023 12:27
@kobergj kobergj marked this pull request as ready for review August 18, 2023 09:59
Signed-off-by: jkoberg <jkoberg@owncloud.com>
Copy link
Contributor

@rhafer rhafer left a comment

Choose a reason for hiding this comment

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

Overall this looks fine to me. Just a new nits and questions.

pkg/utils/grpc.go Outdated Show resolved Hide resolved
pkg/utils/grpc.go Outdated Show resolved Hide resolved
pkg/auth/manager/serviceaccounts/serviceaccounts.go Outdated Show resolved Hide resolved
Copy link
Contributor

@rhafer rhafer left a comment

Choose a reason for hiding this comment

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

IMO it's good now for a start.

@kobergj kobergj marked this pull request as draft August 24, 2023 11:46
@kobergj
Copy link
Contributor Author

kobergj commented Aug 24, 2023

Temporarilly converted to draft so it doesn't get merged before #4133 (that one is urgent for the web team)

}

func (a *inmemAuthenticator) Authenticate(userID string, secret string) error {
if a.m[userID] == secret {
Copy link
Contributor

@rhafer rhafer Aug 24, 2023

Choose a reason for hiding this comment

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

Ugh, just noticed that this will happily validate an any userid with an empty password (apart from the ones set in the ServiceUsers slice) . So it's possible to get a token using

authRes, err := gatewayClient.Authenticate(ctx, &gateway.AuthenticateRequest{
        Type:         "serviceaccounts",
        ClientId:    "what-ever-id-you-want",
        ClientSecret: "",
})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Missed that. Fixed now 👍

Signed-off-by: jkoberg <jkoberg@owncloud.com>
@kobergj kobergj marked this pull request as ready for review August 29, 2023 12:45
@kobergj kobergj merged commit 8ba013d into cs3org:edge Aug 29, 2023
9 checks passed
@kobergj kobergj deleted the ServiceAccounts branch August 29, 2023 12:46
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