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

Allow limiting access only to short URLs created with the same API key. #795

Closed
captainAldi opened this issue Jun 26, 2020 · 17 comments
Closed
Labels
Milestone

Comments

@captainAldi
Copy link

Summary

Hello, there is a feature like different api key and then differenct short link they have ?
cause i dont see it in documentation
and if not it will be great if there is

i mean, now the api key that i made for the same domain it's still can seeing the other url with different api key

it's possible ?

Thanks before

@acelaya
Copy link
Member

acelaya commented Jun 27, 2020

So, let me see if I'm getting it.

Your proposal is that you can make interacting with short URLs to be limited to those URLs created with the same API key, right?

So that you cannot list, edit, delete or get stats for short URLs created with other API keys.

Is that correct?

@captainAldi
Copy link
Author

So, let me see if I'm getting it.

Your proposal is that you can make interacting with short URLs to be limited to those URLs created with the same API key, right?

So that you cannot list, edit, delete or get stats for short URLs created with other API keys.

Is that correct?

yes, that's right
is that possible? 😁

@acelaya
Copy link
Member

acelaya commented Jun 27, 2020

I'm afraid this is currently not supported.

However it looks like something nice to have. I will see how hard would be to get that.

@acelaya acelaya changed the title Different API Key Different Link Allow limiting access only to short URLs created with the same API key. Jun 27, 2020
@captainAldi
Copy link
Author

I'm afraid this is currently not supported.

However it looks like something nice to have. I will see how hard would be to get that.

Sounds Great ! Thanks 😁

@antwonw
Copy link

antwonw commented Sep 10, 2020

+1 for this. But I could think there should be an "admin" API (maybe the first one generated) that can see all short URLs.

If this does get developed into Shlink, I'd also like to suggest that an API key can have a default domain set (at API creation) for each given API key.

Example:

  • KEY1's default domain is foo.com
  • KEY2's default domain is bar.com

@acelaya
Copy link
Member

acelaya commented Sep 19, 2020

I'm thinking on a slightly different approach (mostly because this would need to be backwards compatible), which is adding a, let's call it "capability flag" to the API keys, where you tell if they are limited to interactions with short URLs created with them only, or instead, they can be used for any short URL, as it works now, being the latter its default behavior.

On regards to domains, I think that would be a different feature that could be used together with the one discussed here or independently.

So basically, you could define:

  • "admin" API keys: Default behavior. Can do everything.
  • API keys limited to the short URLs they created: You can only interact with the short URLs created with this API key, regardless of the domain.
  • API keys for one specific domain: You can only interact with the short URLs for that domain, regardless of the API key used to create them.
  • API keys limited to the short URLs they created on a specific domain: You can only interact with the short URLs created with this API key on that domain.

Each "capability" would be added/removed separately to/from the API key.

Other than this, there are questions to answer regarding tags, both for domain-based limits and creation-based limits.

  • Should tag lists include only those tags that have been used for short URLs fulfilling the API key permissions?
  • Should tag stats include only visits on short URLs fulfilling the API key permissions?

In both cases I would say yes, but I'm open to feedback here.

@antwonw
Copy link

antwonw commented Sep 21, 2020

  • "admin" API keys: Default behavior. Can do everything.
  • API keys limited to the short URLs they created: You can only interact with the short URLs created with this API key, regardless of the domain.
  • API keys for one specific domain: You can only interact with the short URLs for that domain, regardless of the API key used to create them.
  • API keys limited to the short URLs they created on a specific domain: You can only interact with the short URLs created with this API key on that domain.

I think that's a solid approach.

  • Should tag lists include only those tags that have been used for short URLs fulfilling the API key permissions?
  • Should tag stats include only visits on short URLs fulfilling the API key permissions?

I'd concur. Yes to both.

@acelaya
Copy link
Member

acelaya commented Jan 3, 2021

Hey @antwonw @rubenmdh @captainAldi. I'm pinging you as you have been involved in this thread or reacted to some of the comments.

I have been working on this feature and I'm more or less half-way there. I have managed to sort the hard part, which is deciding how to approach the issue in a maintainable and future-proof way, and I'm now applying it to all the endpoints that require the extra permission checks.

I will most probably release an alpha version this comming week, containing this feature. I will drop a message here once it's released.

Since the changes are deep, if any of you could test it, it would be great. In the meantime I will close a few minor tasks that I want to release with v2.5

There's no need to agree to anything. I will just drop the message here, and whoever can test the alpha version, it's perfect.

If by the time I have finished working on the rest of the tickets for v2.5 milestone nobody brought up any issues, I will release it as stable.

In any case, I expect at least one patch for v2.5 to be needed, fixing missbehaviors on this feature, or regressions on others.

@rubenmdh
Copy link

rubenmdh commented Jan 4, 2021

I am willing to test it as soon as you are ready to release the alpha version. Looking forward to it :)

@acelaya
Copy link
Member

acelaya commented Jan 4, 2021

Amazing. Thanks @rubenmdh 🙂

@captainAldi
Copy link
Author

that's great, will test it when available 😁

@acelaya
Copy link
Member

acelaya commented Jan 11, 2021

Hey!

I have just published shlink v2.5.0-alpha.2 (the docker image is getting built), which includes the new feature to restrict access to API keys based on the short URLs they created or one domain.

The feature is implemented as described in this comment above, but you can also see the full checklist of things that have been implemented in this issue

It also includes a list of considerations that I will copy here. These are some things I have left out in order to simplify this (as it's quite big already), but also things that cannot be done because of how shlink is architected.

Considerations:

  • Operations from CLI will always behave as admin. Also, created short URLs will not be attached to an API key, and therefore, will not be affected by "author only" API keys.
  • Imported short URLs will also not be attached to an API key.
  • Custom slugs are globally unique. That means that a conflict error could occurr when providing a custom slug that "looks" that it's not being used, but it's actually in use by a short URL created with another API key.
  • When creating short URLs with a "single domain" API key, they will all be created for that domain implicitly, regardless of the domain provided. Provided one will always be ignored.
  • The "single domain" restriction cannot be used for the default domain.
  • For simplicity, editing and deleting tags will not be allowed for non-admin API keys, with the intention to improve this behavior in future versions.
  • Roles can only be set during API key generation, as allowing to edit them might have unexpected results. If you want to restrict access, disable the existing API key and create a new one with less permissions.

The interaction with API keys is still done from the CLI. The api-key:generate command now accepts flags to set roles on the generated API key, and the api-key:list command now includes all the roles that were added to every API key.

You can run them with --help to get further info.

And that's more or less it. Give it a try and let me know if the feature is working more or less as you expected and I didn't mess up too much 😅.

In the meantime I'll continue with the checklist in #882, as this still needs to be documented and I also want to do some load tests.

@rubenmdh
Copy link

It seems I'm the first one! It works fine on my server.
I updated from a previous install without any problems using the bin/updater script. The database and config was successfully imported.
Old API keys are admin keys. I guess this is related to backwards compatibility.
New keys are also created as admin keys unless otherwise specified with the --author-only parameter. I'd prefer it to be this way as default but that's just my personal opinion.
Last but not least, I think it would be a good idea for the admin API key to be able to see each link's API key from the user who created it.
No complaints or bugs found for now!

@acelaya
Copy link
Member

acelaya commented Jan 12, 2021

Hey @rubenmdh, thanks a lot for spending some time testing it.

Old API keys are admin keys. I guess this is related to backwards compatibility

Yes, exactly. Anyone should be able to update to this version and everything should continue working the same.

New keys are also created as admin keys unless otherwise specified with the --author-only parameter

This is more or less related with backwards compatibility too. However, I may give the option to customize the default behavior. Probably on a future version.

I think it would be a good idea for the admin API key to be able to see each link's API key from the user who created it.

I'm not sure what you mean by this. Do you mean that, when listing short URLs with an admin API key, you should be able to see the author API key?

@rubenmdh
Copy link

I'm not sure what you mean by this. Do you mean that, when listing short URLs with an admin API key, you should be able to see the author API key?

Exactly, this is what I meant.
I forgot to add that I am using v3.0.1 of the web client, but I guess this isn't super elevant for this alpha version to work.
Keep up the great work :)

@acelaya
Copy link
Member

acelaya commented Jan 13, 2021

Ok, got it. Sadly, that's a bit tricky.

The fact that you cannot interact with API keys through the API, is intended, for security reasons.

Having now other endpoints return API keys arbitrarily would have a lot of security considerations.

They could of course be masked, by using a neme/title for every API key, but that's a bit cumbersome while managing API keys.

So I'm afraid I won't add this to API endpoints.

However, I could consider extending CLI commands to include extra info.

@acelaya
Copy link
Member

acelaya commented Jan 17, 2021

Shlink v2.5.0 is being built and will be released any time. The docs for API key roles are already available here https://shlink.io/documentation/api-docs/api-key-roles/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants