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

Added link shortener #1372

Merged
merged 22 commits into from Jun 12, 2019
Merged

Added link shortener #1372

merged 22 commits into from Jun 12, 2019

Conversation

mtrojan-ub
Copy link
Contributor

@mtrojan-ub mtrojan-ub commented May 15, 2019

Here's the first implementation, details can be found here:
https://sourceforge.net/p/vufind/mailman/message/36665613/

A few hints:

  • base62 encoding is used to generate even shorter links (instead of base64 for URL compatibility). Also, we used an own implementation to avoid other dependencies like bcmath
  • URL shortener initalization is dependant on config (see Mailer + ShortlinkController). Centralize somehow?

DB:

  • Only path is stored in DB and not the full URL (Site→url will be stripped and added again later on)
  • used „MEDIUMTEXT“ as DB field type for „path“ instead of VARCHAR to avoid limit for long URLs (e.g. 65535)
  • DB field „created“ was added as timestamp (maybe for later cleanup purposes)
  • DB field „path“ is not unique right now and has no index

As always, i'll be glad for any feedback!

TODO

  • Test with pgsql
  • Add short linking to SMS messages
  • Make sure full test suite is passing
  • Update changelog/database changelog when merging

@demiankatz
Copy link
Member

@mtrojan-ub, this looks like an excellent start! I'll try to test in the near future (after finishing up #1354).

From a first glance, I have a few minor questions:

1.) Do you want any help cleaning up the style issues that are causing Travis to complain? I'm happy to help if necessary.

2.) Do you think there is any value in adding a key field of some sort to the database field, to make it harder to reverse engineer the base62 and examine arbitrary links? That is, have a row besides the id that just contains a random integer, and then generate the link with two base62-encoded values -- one for the id and one for the key. I'm not sure whether privacy concerns are strong enough to justify this, but it wouldn't make the link too much longer and would cover more bases.

3.) If we eventually add handlers to use external link shortener services via API, we will need to work with full URLs, not just paths. Do you think it might make sense to deal in full URLs everywhere for consistency, or should we add the logic to convert paths to full URLs inside future API-based plugins as needed?

4.) Did you look into the possibility of shortening links in the SMS logic as well? (I can work on that if you like -- just trying to think if there are more places where this might be useful).

@mtrojan-ub
Copy link
Contributor Author

@demiankatz: Thanks for having a look!

  1. I tried my best solving most of the problems, but maybe you could have a look at the remaining one.

  2. Not sure if this is really necessary, because the stored shortlinks are not related to any user ID. But if you think it has any benefit, feel free to change it.

  3. The reasons why i chose to use paths instead of full urls:

  • Shortlinks cannot be abused to link pages from other websites (i could not think of any case so far where this would be necessary, since we only store internal links)
  • If the domain is changed, the shortlink controller will still be able to redirect to a correct target. (however => if the user still has a shortlink containing the old domain, of course it will fail anyway to call the controller)

But since the decision whether the full URL or only parts of it is encapsulated via the UrlShortenerInterface, additional plugins are able to store the full URL if needed, so that should not be a problem (so i guess it would be better to implement it in other plugins "as needed").

  1. Of course, the shortener can be re-used wherever possible! Feel free to add it to the SMS logic as well (we don't use the SMS logic by ourselves yet). We also think about using it for special shortlinks used for marketing material (pamphlets and so on), but these will be added manually to the database.

@demiankatz
Copy link
Member

Thanks, @mtrojan-ub, your reasoning on item 3 (not storing full URLs at this time) makes sense to me.

I'll look into fixing the style issue and adding SMS support when time permits in the next week or two.

Regarding "more secure shortlinks," I don't have a really strong feeling about this, except that it might be easier to introduce the security now rather than adding it later if we decide it's needed. I'll investigate whether there's a very simple solution that might work.

Finally, it might make sense to build a command line tool for manually creating short links if that's going to be a need on your end. Let me know if you'd like help setting that up.

@demiankatz
Copy link
Member

@mtrojan-ub, I've had some time to work on this further. A few notes:

1.) I decided not to add the "more secure linking" functionality we discussed; as you say, it may not matter, and if it turns out to be needed, we can always build a DatabaseWithObfuscation service or something at a future date. For now, keeping this as simple as possible seems worthwhile.

2.) Another thought that occurred to me was adding logic to search for existing paths in the database to avoid creation of duplicate rows... but performance concerns (searches will get slower and slower as the table grows) persuaded me not to do that. But it might make sense as a config option in future.

3.) I fixed a handful of bugs and added some test coverage.

4.) I ended up moving the link shortening logic out of the Mailer object and into a view helper. This was necessary to support short links in SMS, and it also makes it easier to introduce short links to other custom mail templates in the future.

5.) It would definitely still be valuable to have some command line utilities for creating short links and cleaning old entries out of the table. If you have time to build those, please share in the future... but for now, I think that the work here is complete enough to merge, so I will do that shortly.

@demiankatz demiankatz merged commit f37a094 into vufind-org:master Jun 12, 2019
@mtrojan-ub mtrojan-ub deleted the upstream_shortlink branch June 24, 2019 10:07
@mtrojan-ub
Copy link
Contributor Author

Thanks @demiankatz!

Sorry for the late response, but i've been on vacation.

I'm not sure if we will create command line utilities for this purpose, but if so, we will be glad to share them.

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