Improve API client usability

[qwerty287]

• add an API token system. This assigns a token to every user which you can view, reset, copy etc. This token allows authorized access to API endpoints using the Authorization header.
• do not require Content-Type and Accept headers, if they allow any type or are missing, ignore them, only if they are wrong an exception is thrown.
• add an endpoint User::getCurrent which allows to get the user which is currently logged in

Closes https://github.com/LycheeOrg/Lychee/issues/1356

[qwerty287]

Does anybody know why the SQLite rollback migration fails and how I can fix it?

[codecov[bot]]

Codecov Report

Merging #1368 (fc3fd82) into master (ed4240e) will decrease coverage by 0.50%. The diff coverage is 97.14%.

:exclamation: Current head fc3fd82 differs from pull request most recent head 35bac96. Consider uploading reports for the commit 35bac96 to get more accurate results

Additional details and impacted files

[qwerty287]

I think CI failure is not related. (Only pgsql fails, MariaDB and locally SQLite work)

[ildyria]

• Merged from master.
• fixed conflicts.
• tested.
• add possibility to disable the token feature by leaving it "empty".

[ildyria]

@qwerty287 can you check if you are happy with my changes? :)

[nagmat84]

Please give me the chance to do a review, too, before merge.

[qwerty287]

@ildyria in general yes, but adding the old api key for the admin means that an api key that had no permissions before now has admin permissions. I can fix it.

[ildyria]

api key that had no permissions before now has admin permissions. I can fix it.

Good point. I missed that one.

[ildyria]

@nagmat84 kind reminder as you requested to have time to review 19 days ago. :)

[nagmat84]

After I went through the whole PR and the frontend I have learned that the token is a symmetric, shared secret. This answers my question from above. But in this case, it should also be treated as one. In particular, it should not be sent back to the user except after initial creation. (This is also how Github does it.) If the user forget the token, then the user must reset the token and obtain a new one.

[qwerty287]

~~The tests don't fail locally for me~~ :thinking: Seems working now.

[qwerty287]

The main difference to Github I see here is that Github allows as many keys as you need, while this PR only allows one per user. This makes it hard to reuse the key if you write it down anywhere (I wouldn't do this if it's not in some app's config) and every time I need a new key I have to recreate it.

[nagmat84]

Yes, I saw that difference. And I am somehow inclined to say that users need to be authenticated first, before they can see their own token. But this argument would hold for passwords as well and we don't show the password to the user either. Moreover I cannot give you a good example of what might possibly go wrong. However, the little security guy inside me tells me that it is a bad idea.

As I have no strong feelings about that we can leave it like that for now. I just wanted to inform you about my bad gut feeling.

[qwerty287]

I know. We can try to build a multi-token system in a later PR that makes this easier for us and the users.

[nagmat84]

I had to simplify the middleware VerifyCrsfToken because I could not stand the number of return in the method which really makes it hard to reason about what this method does.

[ildyria]

However, the little security guy inside me tells me that it is a bad idea.

I am actually more concerned about something else... which is the reason why we hash password. If the database is compromised, at least secrets are non recoverable.

I would actually like to suggest the following. Have the token encrypted in the database. When we receive the token, we encrypt it, and search for the encrypted match.

That way, it is possible for a user to "read" is current token, but if the DB is compromised but not the .env then we are less in a pinch. Relevant: https://laravel.com/docs/8.x/encryption

[nagmat84]

@qwerty287 Given my remark about putting the user object properly into the initialization part. I will create a PR for that. This will close some the TODOs which I added to the source code and it will make your PR for the front-end cleaner.

[nagmat84]

@qwerty287 Given my remark about putting the user object properly into the initialization part. I will create a PR for that. This will close some the TODOs which I added to the source code and it will make your PR for the front-end cleaner.

With PR https://github.com/LycheeOrg/Lychee/pull/1443 the entire user object becomes part of lychee.user in the front-end. This frees this PR from fetching the current user before opening the dialog. Even though User::getAuthenticatedUser becomes unnecessary with that, I would keep that method. It does not harm to have it.

[qwerty287]

@ildyria I tried to implement encryption of the tokens now, but it didn't work. I can't search for encrypted values in the DB, because they are always different. They use some random string to kind of salt it (but the salt isn't necessary to decrypt it). This would only be possible if encryptString always returns the same output for the same input. (See https://github.com/laravel/framework/blob/8.x/src/Illuminate/Encryption/Encrypter.php#L100).

Also, encrypting the token isn't something where I'd say it's necessary. Hashing a password is something else, because they are often used multiple times, but the tokens are random. I don't really see a security risk here, because if your DB is compromised, they have access to every data anyways (except data that is added later of course).

[ildyria]

@ildyria I tried to implement encryption of the tokens now, but it didn't work. I can't search for encrypted values in the DB, because they are always different. They use some random string to kind of salt it (but the salt isn't necessary to decrypt it). This would only be possible if encryptString always returns the same output for the same input. (See https://github.com/laravel/framework/blob/8.x/src/Illuminate/Encryption/Encrypter.php#L100).

Also, encrypting the token isn't something where I'd say it's necessary. Hashing a password is something else, because they are often used multiple times, but the tokens are random. I don't really see a security risk here, because if your DB is compromised, they have access to every data anyways (except data that is added later of course).

true.

[ildyria]

May I suggest to get out in 4.6.1, once @nagmat84 is happy with it ? :)

[d7415]

We could always hash the token. You're back to "show once", but that could well be why "proper" services do it that way. I don't think the argument that these aren't passwords holds.

[nagmat84]

We could always hash the token. You're back to "show once", but that could well be why "proper" services do it that way. I don't think the argument that these aren't passwords holds.

This way my initial idea when I reviewed this PR. I wanted "show once" combined with "hash it". Who gave an argument based on "it is not a password hence we are good"?! :astonished: :open_mouth:

[qwerty287]

That's not what I meant :) Of course it's best to encrypt (or hash) it, but the main difference to a password is that it's randomly generated. The main reason to hash the password is to prevent that you can log in to other accounts with the same password, and since this is a random string, you can't use it to take over other accounts with it.

Yes, I thought about hashing too, but show once is not that user-friendly if you only have one key. I can implement it though.

[nagmat84]

That's not what I meant :) Of course it's best to encrypt (or hash) it, but the main difference to a password is that it's randomly generated. The main reason to hash the password is to prevent that you can log in to other accounts with the same password

No that is not the reason for hashing. One hashes the DB such that an attacker cannot use the password after the attacker has corrupted the DB, because the attacker cannot calculate the pre-image of the hashes. It does not matter whether all passwords are distinct or if some user use the same password.

and since this is a random string, you can't use it to take over other accounts with it.

No, that is not true. If the DB has been corrupted and the attacker can read the tokens, it does not matter whether they have been randomly generated or if they have been chosen by a human (like most passwords).

[qwerty287]

I pushed https://github.com/LycheeOrg/Lychee/pull/1368/commits/d8ccc6e75488edc283dcca95e9e4bd71432ed274 which hashes the tokens using SHA-512 (not bcrypt though) and only allows to see them once.

[qwerty287]

@nagmat84 would be good if you also re-review https://github.com/LycheeOrg/Lychee-front/pull/300.

[nagmat84]

While I believe that the code is correct, I am currently unable to write proper tests for it. Again, the problem is that the application object survives between tests. As we already have had this problem once or twice and posted this question https://github.com/laravel/framework/discussions/44086.

In the past we could always help ourselves with some workaround, but in this case I am lost. I hope some answers.

[nagmat84]

And another one: https://github.com/laravel/framework/discussions/44088

[nagmat84]

I fixed the tests. While SessionOrTokenGuard has become more complicated than I initially thought, I am rather convinced that it should be fine. We have 96% test coverage and the only two lines which are not tested affect the "remember-me" token which we don't use.

Only two things remain:

@qwerty287 please check if you are fine with my changes and if the PR still does what you want it to do.

The issue https://github.com/LycheeOrg/Lychee/pull/1368#discussion_r966251712 needs to be addressed.