-
-
Notifications
You must be signed in to change notification settings - Fork 230
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
Feature: Add API Key support (🚧) #350
base: v1.0.0-alpha
Are you sure you want to change the base?
Conversation
Nice! Thanks for contributing this. I'll set aside some time tomorrow to review it. |
Is there somewhere a swagger documentation or the likes for the API? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work here! Thank you very much for the contribution. If it's OK with you, I can use this branch as the base for auth changes and I can push up some changes/fixes.
Model::preventLazyLoading(!app()->isProduction()); | ||
|
||
Relation::enforceMorphMap([ | ||
EventDomainObject::class => Event::class, | ||
OrganizerDomainObject::class => Organizer::class, | ||
'user' => User::class, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious what the reason for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As soon as I added the traits required for Sanctum to the User class, I received an error about being unable to morph a User object (I unfortunately don't recall the specific error). Upon doing some google research, I discovered some other people suggesting this as a fix to the error, and it worked.
I'm sure if you remove this you'll get the same error, just in case there is a better solution I am unaware of. Or maybe you won't and I was just being gaslit. 😂
@@ -24,6 +24,7 @@ const ALLOWED_UNAUTHENTICATED_PATHS = [ | |||
'widget', | |||
'/product/', | |||
'check-in', | |||
'csrf-cookie', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This array relates to frontend path. From what I can see csrf-cookie
is a backend path.
@@ -20,7 +20,9 @@ export const authClient = { | |||
}, | |||
|
|||
login: async (user: LoginData) => { | |||
const response = await api.post<LoginResponse>('auth/login', user); | |||
const response = await api.get('/csrf-cookie').then(async response => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
async response
can be removed as it's unused.
export const CreateApiKeyModal = ({onClose, onCompleted}: CreateApiKeyModalProps) => { | ||
const createMutation = useCreateApiKey(); | ||
const queryClient = useQueryClient(); | ||
const formErrorHandler = useFormErrorResponseHandler(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this is unused
const handleCreate = useMutation({ | ||
mutationFn: (apiKeyRequest: CreateApiKeyRequest) => apiKeysClient.create(apiKeyRequest), | ||
|
||
onSuccess: (response: NewApiKey) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NewApiKey
doesn't exist
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The interface does exist, but you're right, I didn't import it in this file and I didn't specify it as the result of a Create call in the api-keys.client.ts file.
Thanks!
Co-authored-by: Dave Earley <[email protected]>
You're welcome! Thanks for the review. Definitely feel free to use this as a jumping off point for the auth refactor. |
Resolves #339
This is a WIP change to add API keys. The feature is implemented and admins can create API keys and they can be used to access the APIs in a granular fashion. I still need to complete all translations, and want to give @daveearley an opportunity to review this sooner rather than later.
This also adds a migration to add
account_id
to the token table, so tokens can be set up on a per-account basis, and the auth flow knows which account a token is attached to, as we don't have the JWT payload to tell us. (TODO: May need to implement key filtering from view for multiple-account situations)Tested with expired tokens, tokens with missing abilities, and valid tokens.
Checklist
Thank you for your contribution! 🎉