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

Allowing access to Swift containers shared by ACL in files_external #19003

Closed

Conversation

tjdett
Copy link
Contributor

@tjdett tjdett commented Sep 14, 2015

OpenStack Swift provides the ability to share read/write access to a container without requiring the accessing user to be a member of the container's tenant/project. This is useful for providing ownCloud access to containers without also providing it privileges beyond that container.

To avoid a confusing extra field, the bucket field is overloaded to optionally take a URL, which allows ownCloud to use its Swift user credentials with arbitrary containers.

To avoid a confusing extra field, the bucket field is overloaded to
optionally take a URL.
@scrutinizer-notifier
Copy link

A new inspection was created.

@tjdett tjdett changed the title Allowing access to Swift containers shared by ACL Allowing access to Swift containers shared by ACL in files_external Sep 14, 2015
@karlitschek
Copy link
Contributor

@butonic can you have a look please?

@LukasReschke
Copy link
Member

@owncloud-bot This is okay to test.

@DeepDiver1975 CLA bot? ;-)

@DeepDiver1975 DeepDiver1975 added this to the 9.0-next milestone Sep 23, 2015
@PVince81
Copy link
Contributor

The problem with reusing the "bucket" field is that in the admin UI it will not be clear to users that it can also be a URL. What are the arguments against having this as an extra field instead ?
Or is that URL the bucket's URL ? (note: I don't know enough about SWIFT to be able to judge this)

@tjdett
Copy link
Contributor Author

tjdett commented Oct 21, 2015

@PVince81 terminology note first: what are commonly referred to as "buckets" in OpenStack are "containers", but they're essentially the same thing. Technically "tenant" is an out-dated term too, but because most people still use the old v2 OpenStack Keystone API interface I'm going to use it rather than "project" below.

This patch allows you to specify the container's absolute URL instead of the container's name in the bucket/container field. All containers are accessed via a HTTP interface, and their absolute URLs look something like this:

<scheme>://<host>:<port>/<api_version>/AUTH_<tenant_id>/<container_name>

Normally the OpenStack libraries use the OpenStack Keystone service catalogue to work out this URL from the container name, in combination with the service name, the tenant name and the region. Things get more tricky though when you want to use Swift read/write ACLs to share a container.

The Keystone service catalogue is only provided after authentication, and could technically vary depending on authenticating user. In addition, the tenant name is part of the user authentication credentials, so a shared container will have a different tenant name/id to the authenticating tenant. Put short: you need a lot more information, and some of it won't be easily discoverable.

The way the Swift CLI client resolves this is to use --os-storage-url, which supplies the base URL up to, but not including, the container name. The container name is then appended to the URL. To mirror this functionality, we'd add an extra field. However, the storage URL is only ever used in conjunction with the container name - read/write ACLs don't allow you to discover other containers, so it's rather useless on its own. I suspect the client mainly does things this way because it makes argument parsing easier, though I think it can also be used to interact with Swift accounts without Keystone in dev environments. That's not terribly relevant in the ownCloud context though, as ownCloud only operates on single containers, not the account that holds the containers.

Rather than add an extra field which would not be used most of the time (container sharing is relatively uncommon), I instead just overloaded the existing field to optionally take the full container URL. In both cases you're supplying the location of the container in that field, just in different forms.

In the context of the existing number of fields, I think this was the right way to go but I'm happy to hear suggestions about how an additional field would work if it still sounds less confusing.

@RobinMcCorkell
Copy link
Member

I think it would make sense to overload the Bucket field in this way, but it needs to be documented properly.

@PVince81
Copy link
Contributor

Thanks for the explanation.

There is also another "url" field but that seems to be the authentication URL which, I guess, can be different than the one from the bucket.

I'm fine having a URL in the bucket field, maybe the field's label should be changed accordingly to "Bucket/URL"

@RobinMcCorkell
Copy link
Member

@PVince81 To be honest, this use case is very advanced from my PoV, we should avoid confusing 'normal' users and aim to keep the field labels as short as possible (especially when thinking of translations). I think leaving it as 'Bucket' makes sense for its primary use case, and for this advanced use case putting a full URL as the Bucket makes sense to a degree. Just as long as it's properly documented.

This is one of those cases where we could really use having configurable tooltip messages, e.g. "Specify a bucket accessible by your tenant or a full URL"

@tjdett
Copy link
Contributor Author

tjdett commented Oct 22, 2015

@Xenopathic I agree a hover-over would be ideal. The placeholder text is likely too short to avoid confusion for "normal" users. I'm happy to document this wherever makes sense.

This is an advanced feature. Its primary use is to prevent your ownCloud instance having OpenStack credentials that can be abused. Really useful for "enterprise" users, particularly if multiple OpenStack tenants are being used for accounting purposes, but likely too complicated for most other users.

@PVince81
Copy link
Contributor

I tested the regular way and swift storage can still be mounted properly, no regression 👍

@PVince81
Copy link
Contributor

@butonic @Xenopathic second reviewer ?

@MorrisJobke
Copy link
Contributor

PR for CI reasons moved to #21008

@MorrisJobke MorrisJobke closed this Dec 7, 2015
@lock lock bot locked as resolved and limited conversation to collaborators Aug 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants