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

when sharing via ocs look up user by username #1281

Merged
merged 2 commits into from
Nov 23, 2020

Conversation

butonic
Copy link
Contributor

@butonic butonic commented Oct 27, 2020

The ocs api returns usernames when listing share recipients, so the lookup when creating the share needs to search the usernames and not the userid.

related: owncloud/ocis#627

@butonic butonic requested a review from labkode as a code owner October 27, 2020 14:23
@butonic butonic force-pushed the ocs-lookup-users-by-username branch 2 times, most recently from 8b28fa5 to 2dd898f Compare October 27, 2020 17:27
Signed-off-by: Jörn Friedrich Dreyer <[email protected]>
@butonic butonic force-pushed the ocs-lookup-users-by-username branch from 2dd898f to d7dd62a Compare October 27, 2020 19:31
@butonic butonic requested a review from ishank011 October 28, 2020 08:45
@labkode
Copy link
Member

labkode commented Oct 28, 2020

@ishank011 does this affect the OCM layer?

Copy link
Contributor

@ishank011 ishank011 left a comment

Choose a reason for hiding this comment

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

It doesn't directly affect it, but this creates inconsistency with other invocations of CreateShare/CreateOCMShare.

https://github.com/cs3org/reva/blob/master/internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/shares.go#L474-L476 here we still assume that we're getting the opaque ID as the input. Also in the cmd actions share-create and ocm-share-create, we still assume the opaque ID, so it'd be good to have the same thing across all the calls.

@butonic
Copy link
Contributor Author

butonic commented Oct 28, 2020

@ishank011 @labkode we are looking into tho ocs and ocdav apis. we tried to use the userid / opaque id because it is the only real stable identifier that does not change.

ocs and ocdav really only know about usernames. with the oc10 limitation that usernames cannot change. also testsuites, web, desktop and mobile clients all use that username. but if we use the userid as the username it has different properties, like userid is case sensitive, username is not ...

I have been trying hard to introduce a stable userid. And for the cs3 api that is the user opaqueid. But for the legacy ocs and ocdav apis that is thh username.

now ocm has a federated sharing id ... which imo should use the cs3 opaqueid. but if you want to stay backwards compatible you need a migration strategy anyway.

I haven't looked at the OCM code. What property are you using to construct the federated sharing id? oc10 uses the username...

@ishank011
Copy link
Contributor

@butonic for OCM, we ensure that the cs3 userID (opaque ID + IdP) is unique and use that to identify remote users.

@labkode
Copy link
Member

labkode commented Oct 28, 2020

@ishank @butonic What about having the ocdav and ocs layers using the username externally but mapping it to userId when calling internal services? This assumes the username is unique (which currently is).

@butonic
Copy link
Contributor Author

butonic commented Oct 28, 2020

yes, the ocs ocdav and imo ocm endpoints should only expose the username, because that is what existing clients expect. the services should map them to a proper cs3 user id.

well, if ocm exposes the internal userid that might make sense because it needs stable user identifiers. for a migration you could make the existing username the userid (since they are currently unique and should not collide with uuids which should be used in the future)

@butonic
Copy link
Contributor Author

butonic commented Nov 3, 2020

@labkode @ishank011 should I update the ocm service to expect the username? I'm not too deep into that ui, so I don't know if that would match the spec.

@butonic
Copy link
Contributor Author

butonic commented Nov 3, 2020

Looking at https://rawgit.com/GEANT/OCM-API/v1/docs.html#null%2Fpaths%2F~1shares%2Fpost I can only find shareWith and the webdav option username ... Which AFAIU both correlate to our username or ocs user ID, which is just named confusingly.

shareWith and owner are interesting. They seem to be composed of username and a provider / issuer. Will dig in the oc10 impl.

@butonic
Copy link
Contributor Author

butonic commented Nov 3, 2020

Ok that is the cloudId, formerly known as federated sharing id, which consists of the username and the host. See https://github.com/owncloud/core/blob/master/apps/federatedfilesharing/lib/Address.php for details.

The translateUid function is even some old magic that uses the login used for ldap accounts. This is obviously already a deprecated fragile design, because it does cover changing usernames. Changing that is out of scope for Reva. So I think the correct way to handle this for now is to only expose the username. That will migrating users to ocis/reva easier. A stable an persistent identifier can be introduced later. Following protocol driven development we should propose this change to the ocm spec.

@ishank011
Copy link
Contributor

yes, the ocs ocdav and imo ocm endpoints should only expose the username, because that is what existing clients expect. the services should map them to a proper cs3 user id.

Yes, using the username would be fine, but we'd still need to provide the ID of the provider as users across different providers can have the same username. This should be added to the spec IMO.

Regarding retrieving users from other claims, I can add a similar method to the OCM user service, so that shouldn't be a problem.

@labkode labkode merged commit 90850c5 into cs3org:master Nov 23, 2020
@butonic butonic deleted the ocs-lookup-users-by-username branch December 8, 2020 13:27
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