ocis icon indicating copy to clipboard operation
ocis copied to clipboard

feat: reva app auth

Open DeepDiver1975 opened this issue 9 months ago • 5 comments

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:

DeepDiver1975 avatar May 06 '24 10:05 DeepDiver1975

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.

update-docs[bot] avatar May 06 '24 10:05 update-docs[bot]

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.

https://github.com/owncloud/ocis/blob/afc6ed1e416c8c1abef832df3e6912b7487cccbd/vendor/github.com/cs3org/reva/v2/pkg/appauth/manager/json/json.go#L140

DeepDiver1975 avatar May 06 '24 17:05 DeepDiver1975

Furthermore I don't think these scopes are of any help for the calendar and contacts integration: https://github.com/owncloud/ocis/blob/afc6ed1e416c8c1abef832df3e6912b7487cccbd/vendor/github.com/cs3org/go-cs3apis/cs3/auth/provider/v1beta1/resources.pb.go#L77

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 avatar May 06 '24 18:05 DeepDiver1975

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

https://github.com/owncloud/ocis/blob/b6d5cd78f0b5185df1c175246fafde0fa88995e1/vendor/github.com/cs3org/reva/v2/pkg/auth/scope/scope.go#L33

DeepDiver1975 avatar May 06 '24 18:05 DeepDiver1975

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

https://github.com/owncloud/ocis/blob/afc6ed1e416c8c1abef832df3e6912b7487cccbd/vendor/github.com/cs3org/go-cs3apis/cs3/auth/provider/v1beta1/resources.pb.go#L77

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 https://github.com/owncloud/ocis/pull/8522#issuecomment-2092985475

rhafer avatar May 07 '24 06:05 rhafer

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

DeepDiver1975 avatar May 07 '24 06:05 DeepDiver1975

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)

:cry:

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".

rhafer avatar May 07 '24 07:05 rhafer

machine auth does not support scopes as far as I can see .... https://github.com/owncloud/ocis/blob/afc6ed1e416c8c1abef832df3e6912b7487cccbd/vendor/github.com/cs3org/reva/v2/pkg/auth/manager/machine/machine.go#L73

but maybe I am missing something.

to sum it up:

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

DeepDiver1975 avatar May 07 '24 07:05 DeepDiver1975

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.

rhafer avatar May 07 '24 07:05 rhafer

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

mmattel avatar May 07 '24 08:05 mmattel

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

DeepDiver1975 avatar May 07 '24 09:05 DeepDiver1975

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.

micbar avatar May 07 '24 11:05 micbar

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

THX - sorted with @rhafer ...

DeepDiver1975 avatar May 07 '24 12:05 DeepDiver1975

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

tbsbdr avatar Jul 10 '24 11:07 tbsbdr

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

DeepDiver1975 avatar Jul 10 '24 11:07 DeepDiver1975

Good decision 👍

tbsbdr avatar Jul 11 '24 05:07 tbsbdr

great that this is moving forward, thx @kobergj !

tbsbdr avatar Jul 16 '24 12:07 tbsbdr