feat: Laravel Sanctum API implementation #1222
Conversation
…nto feature/api
…nto feature/api
itinerare
added
enhancement
There was a problem hiding this comment.
small feature request, i think this would be most convenient if there was a way for the tokens to be generated on interaction with certain endpoints of the API, if that makes sense, rather than the user having to set their token (in the case of members)
I'm thinking about this from a minigame perspective, but considering (I believe) this feature is used by your arcade centre addon, maybe it already exists and I'm just not quite seeing it?
Nah you're right, it doesn't exist yet. It makes a lot of sense, I'll see if that's doable -- and this actually helps with wrapping my mind around something I haven't solved yet. Makes me realize that I don't think the api_access staff permission is necessary, because I can just use checking that we already have (such as, if someone makes an API request to pull data, it can just do a verification against the edit_data power). No reason to stop normal users from having PATs. I'll refactor the feature with this in mind! |
…ng currently authenticated user's token
|
Just made some updates based on previous discussion:
Let me know if anyone thinks the route should live somewhere else ("account" made the most sense to me) or any other feedback! |
|
This is a feature for users now? Then I'm going to have to ask if you can add the option to hide it from users. Some administration may not want their users to do this, at all. |
|
Due to popular demand, yes, it was added for users! But that's easy enough. Just added a new site setting where API token generation for users is disabled by default. It does two checks -- on the settings page, and also within the postGenerateToken function. It's worth noting that, despite the visual warning, the default implementation doesn't allow for the PAT to actually do anything. It was a conscious choice. There's one example route -- allowing people with edit_data to pull data for a specific character -- but that's it. |
|
Thank you for adding that. Unfortunately, I do not have the knowledge rn to properly approve your PR, and some of this is going way over my head. I'll try to schedule some time to read up on this properly later on. :) |
|
Added another site setting that, in combination with the other site setting, allows for API access/token generation to be blocked entirely. |
|
|
||
| $this->addSiteSetting('allow_users_to_delete_profile_comments', 0, '0: Users cannot delete profile comments, 1: Users can delete profile comments.'); | ||
|
|
||
| $this->addSiteSetting('allow_users_to_generate_tokens', 0, '0: Users cannot generate their own API tokens, 1: Users can generate their own API tokens.'); |
There was a problem hiding this comment.
This is better than initial, but I think it would be best if the generation of tokens is entirely outside of non-staff's hands.
When I mean generation of a token on an endpoint, I meant an automatic dispense of the token to either browser session or some other carrying method!
There was a problem hiding this comment.
See my other comment -- I can definitely remove this code if that's the general consensus, but I'd like to try to standardize as many API use cases as I can (since that's the main purpose of this PR, standardizing an API implementation).
(and also I replied to these in backwards order but thank you for reviewing this!!)
| UserUpdateLog::create(['staff_id' => $user->id, 'user_id' => $user->id, 'data' => json_encode([]), 'type' => 'Generated API Token']); | ||
|
|
||
| flash('Token created successfully:')->success(); | ||
| flash($token)->success(); |
There was a problem hiding this comment.
yeah ideally we do not let the users do this at all. most members will have no idea what to do with this and ideally shouldn't
There was a problem hiding this comment.
I think I've received conflicting feedback regarding whether or not users should have the feature. I can see circumstances where we might want it for users -- such as giving a discord bot their API key, like I've seen with bots that connect to MMO APIs.
My happy medium would be a feature that can be disabled with a site setting, but if it's strongly desired, I can revert back to the api_access solution.
Maybe I can have it be disabled in the config rather than as a site setting so it has to be enabled more "deliberately"?
There was a problem hiding this comment.
I think this entire endeavour can be done without Sanctum, is there any benefit to using Sanctum at this point in time?
Just concerned about introducing another dependency if we can do this vanilla
There was a problem hiding this comment.
I chose Sanctum as it's widely supported and also the lightest API solution for Laravel. I understand the concern about adding an additional package, but think it's more secure to go with a frequently updated library rather than something bespoke, which may fall out disuse and have less resources for people building off of it.
There was a problem hiding this comment.
i get that! but what im saying is is there any specific thing that sanctum is providing here that we can't just get with regular endpoints?
There was a problem hiding this comment.
You'll have to forgive me if I'm getting something wrong but from what I was reading, Fortify isn't designed for API authentication and Sanctum/Passport is preferred. We could use the basic routes and code our own token generation but I'm hesitant to circumvent Fortify in such a way, plus it just feels like reinventing the wheel? Feels more secure to keep the API authentication and session-based authentication (mostly) separate
There was a problem hiding this comment.
ah yep i misunderstood! all good on that front
| * Run the migrations. | ||
| */ | ||
| public function up(): void { | ||
| Schema::create('personal_access_tokens', function (Blueprint $table) { |
There was a problem hiding this comment.
I would prefer this to be named user_api_tokens or similar to follow a more consistent schema
There was a problem hiding this comment.
Will update -- this is just the default Sanctum migration.
There was a problem hiding this comment.
On second thought this is probably more trouble than it's worth? I'd have to override the default model, and that could get messy: laravel/sanctum#130
Looks like they never ended up putting it in the config file when I went digging lol
4 participants
This current implementation only allows for one API token for user. This API token will have all the permissions that the user currently has. I considered adding granular permissions for API tokens, but it seemed like it would get too invasive to current features to add a second sort of power checking.