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

feat: reva app auth #9079

Merged
merged 12 commits into from
Jul 23, 2024
Merged

feat: reva app auth #9079

merged 12 commits into from
Jul 23, 2024

Conversation

DeepDiver1975
Copy link
Member

@DeepDiver1975 DeepDiver1975 commented May 6, 2024

Description

  • Provide a README.md for that service in the root folder of that service.
    • Use CamelCase for section headers.
  • For images and example files used in README.md:
    • Create a folder named md-sources on the same level where README.md is located. Put all the images and example files referenced by README.md into this folder.
    • Use absolute references like https://raw.githubusercontent.com/owncloud/ocis/master/services/<service-name>/md-sources/file to make the content accessible for both README.md and owncloud.dev
      bad <img src="https://github.com/owncloud/ocis/blob/master/services/graph/images/mermaid-graph.svg" width="500" />
      good <img src="https://raw.githubusercontent.com/owncloud/ocis/master/services/graph/images/mermaid-graph.svg" width="500" />
  • If new CLI command are introduced, that command must be described in the README.md.
  • If new global envvars are introduced, the name must start with OCIS_.
  • Add the service to the makefile in the ocis repo root.
  • Make the service startable for binary and individual startup:
    • For single binary add service to ocis/pkg/runtime
    • For individual startup add service to ocis/pkg/commands
    • Add the service config to ocis-pkg/config/defaultconfig.go
  • If the service is using service accounts, add it to ocis/pkg/init/init.go
  • Add the service to .drone.star to enable CI.
  • Inform doc team in an early stage to review the readme AND the environment variables created.
    • The description must reflect the behaviour AND usually has a positive code quality impact.
  • Create proper description strings for envvars - see other services for examples, especially when it comes to multiple values. This must include:
    • base description, set of available values, description of each value.
  • When suggestable commits are created for text changes and you agree, collect them to a batch and commit them. Do not forget to rebase locally to avoid overwriting the changes made.
  • If new envvars are introduced which serve the same purpose but in multiple services, an additional envvar must be added at the beginning of the list starting with OCIS_ (global envvar).
  • Ensure that a service has a debug port
  • If the new service introduces a new port:
  • Make sure to have a function FullDefaultConfig() in pkg/config/defaults/defaultconfig.go of your service. It is needed to create the documentation.
  • Add metrics to the code to enable monitoring. See the proxy service for implementation details.
    • Plus add a monitoring documentation in the README.md file

Related Issue

  • Fixes <issue_link>

Motivation and Context

How Has This Been Tested?

  • test environment:
  • test case 1:
  • test case 2:
  • ...

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

Copy link

update-docs bot commented May 6, 2024

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.

@DeepDiver1975
Copy link
Member Author

Somehow the app auth interface is not what I am expecting.
GenerateAppPassword is not taking the user as argument but extracts it from the context (see below)

The function is designed to be called from an authenticated request of the user.
We cannot call it from the migration service to generate tokens for any user.
We cannot call this function from an cli tool to generate tokens for a user for testing purpose or what ever.

userID := ctxpkg.ContextMustGetUser(ctx).GetId()

@DeepDiver1975
Copy link
Member Author

DeepDiver1975 commented May 6, 2024

Furthermore I don't think these scopes are of any help for the calendar and contacts integration:

We need a scope like calendar.Read files.Read .... no idea how this should fit in here.

From my understanding these scopes can only be verified in the oCIS proxy. But prove me wrong ....

@DeepDiver1975
Copy link
Member Author

Scopes are hardcoded in reva - kind of crazy - but okay ....

var supportedScopes = map[string]Verifier{

@rhafer
Copy link
Contributor

rhafer commented May 7, 2024

Furthermore I don't think these scopes are of any help for the calendar and contacts integration:

We need a scope like calendar.Read files.Read .... no idea how this should fit in here.

From my understanding these scopes can only be verified in the oCIS proxy. But prove me wrong ....

Please. Let's keep this PR limited to the migration topic for now. As was decided in #8522 (comment)

@DeepDiver1975
Copy link
Member Author

Please. Let's keep this PR limited to the migration topic for now. As was decided in #8522 (comment)

Even then - scopes need to be set properly and I was wondering how this shall work. But I answered this myself already. THX

@rhafer
Copy link
Contributor

rhafer commented May 7, 2024

Somehow the app auth interface is not what I am expecting. GenerateAppPassword is not taking the user as argument but extracts it from the context (see below)

😢

The function is designed to be called from an authenticated request of the user. We cannot call it from the migration service to generate tokens for any user. We cannot call this function from an cli tool to generate tokens for a user for testing purpose or what ever.

From the cli tool and the migration service you could just use the machineauth authprovider to impersonate the user for which you want to create an AppToken. Which can be done using the authtype machine:

AuthenticateRequest{
          Type:         "machine",
          ClientId:    claim + ":" + value,
          ClientSecret: <machineauthsecret>,
}

claim can be "username", "userid" or "mail".

Alternatively we could enhance the GenerateAppPasswordRequest and add a userid field or put the desired userid into the Opaque field of the request. But for now I think going the machineauth way is simpler and "good enough".

@DeepDiver1975
Copy link
Member Author

machine auth does not support scopes as far as I can see ....

func (m *manager) Authenticate(ctx context.Context, user, secret string) (*userpb.User, map[string]*authpb.Scope, error) {

but maybe I am missing something.

to sum it up:

  • machine auth for migration service
  • app auth for calendar, contacts and related

@rhafer
Copy link
Contributor

rhafer commented May 7, 2024

to sum it up:

* machine auth for migration service

* app auth for calendar, contacts and related

I guess that was a misunderstanding. I meant you could use machine auth (via the gateway) for getting a token for the target user. Which you can then use with GenerateAppPassword to create a app token for the user.

@mmattel
Copy link
Contributor

mmattel commented May 7, 2024

Hooking myself in as I need to add a page in the admin docs when merged,
else we get build errors there.

@DeepDiver1975
Copy link
Member Author

seeing following error when trying to call GenerateAppPassword in create.go ....

rpc error: code = Unimplemented desc = gateway: error calling GenerateAppPassword: rpc error: code = Unimplemented desc = unknown service cs3.auth.applications.v1beta1.ApplicationsAPI

@micbar
Copy link
Contributor

micbar commented May 7, 2024

seeing following error when trying to call GenerateAppPassword in create.go ....

Saw a similiar issue before, looks like the connection to the provider doesn't work. Maybe a port issue.

@DeepDiver1975
Copy link
Member Author

Saw a similiar issue before, looks like the connection to the provider doesn't work. Maybe a port issue.

THX - sorted with @rhafer ...

if err != nil {
return nil, false
}
r.Header.Add(_headerRevaAccessToken, authenticateResponse.GetToken())
Copy link
Member Author

Choose a reason for hiding this comment

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

@rhafer somehow the token is not evaluated in webdav requests down the line. We must be missing something ...

@DeepDiver1975 DeepDiver1975 force-pushed the feat/reva-app-auth branch 4 times, most recently from 4f85ade to 3100812 Compare May 10, 2024 10:37
Copy link

sonarcloud bot commented May 10, 2024

@tbsbdr
Copy link
Contributor

tbsbdr commented Jul 10, 2024

@DeepDiver1975 what needs to be decided to move this forward?

@DeepDiver1975
Copy link
Member Author

@kobergj will finish it. I am out of this. Thx

@tbsbdr
Copy link
Contributor

tbsbdr commented Jul 11, 2024

Good decision 👍

@kobergj kobergj force-pushed the feat/reva-app-auth branch 3 times, most recently from c37e8ce to 87302bc Compare July 15, 2024 08:30
@kobergj kobergj marked this pull request as draft July 17, 2024 12:53
@kobergj kobergj force-pushed the feat/reva-app-auth branch 4 times, most recently from 0c8332d to edb82c4 Compare July 17, 2024 14:11
Copy link
Contributor

@mmattel mmattel left a comment

Choose a reason for hiding this comment

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

Textwise this LGTM 👍

@kobergj kobergj marked this pull request as ready for review July 18, 2024 07:29
DeepDiver1975 and others added 11 commits July 23, 2024 08:58
Signed-off-by: jkoberg <jkoberg@owncloud.com>
Signed-off-by: jkoberg <jkoberg@owncloud.com>
Signed-off-by: jkoberg <jkoberg@owncloud.com>
Co-authored-by: Martin <github@diemattels.at>
Signed-off-by: jkoberg <jkoberg@owncloud.com>
Co-authored-by: Martin <github@diemattels.at>
Signed-off-by: jkoberg <jkoberg@owncloud.com>
services/auth-basic/README.md Outdated Show resolved Hide resolved
services/auth-bearer/README.md Outdated Show resolved Hide resolved
services/auth-machine/README.md Outdated Show resolved Hide resolved
services/auth-service/README.md Outdated Show resolved Hide resolved
Signed-off-by: jkoberg <jkoberg@owncloud.com>
Copy link

sonarcloud bot commented Jul 23, 2024

@kobergj kobergj merged commit e7340f6 into master Jul 23, 2024
4 checks passed
@kobergj kobergj deleted the feat/reva-app-auth branch July 23, 2024 08:37
ownclouders pushed a commit that referenced this pull request Jul 23, 2024
@micbar micbar mentioned this pull request Jul 30, 2024
20 tasks
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.

7 participants