Skip to content
This repository has been archived by the owner on Aug 11, 2024. It is now read-only.

feat: validate API responses #333

Merged
merged 27 commits into from
Jan 24, 2024
Merged

feat: validate API responses #333

merged 27 commits into from
Jan 24, 2024

Conversation

wpf500
Copy link
Member

@wpf500 wpf500 commented Jan 10, 2024

Adds validation to the API responses to ensure we aren't accidentally sending back data that the user doesn't have access to.

This is implemented with an interceptor and validation using class-validator. It optionally allows properties to be marked as admin only.

Todo

  • Handle export endpoints

@wpf500 wpf500 force-pushed the feat/917-response-validation branch from 535ae81 to cbab324 Compare January 12, 2024 10:28
Base automatically changed from feat/917-transformers to master January 17, 2024 18:16
@wpf500 wpf500 marked this pull request as ready for review January 17, 2024 18:45
@wpf500 wpf500 requested a review from JumpLink January 18, 2024 19:10
@wpf500 wpf500 marked this pull request as draft January 19, 2024 14:44
@wpf500 wpf500 marked this pull request as ready for review January 19, 2024 14:44
Copy link
Contributor

@JumpLink JumpLink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me except for one question

@wpf500
Copy link
Member Author

wpf500 commented Jan 24, 2024

@JumpLink what is that question? 😄

@JumpLink
Copy link
Contributor

JumpLink commented Jan 24, 2024

@wpf500 Sorry, here is the question:

Any API key has admin and superadmin roles in src/models/ApiKey.ts:

  get activeRoles(): RoleType[] {
    return ["admin", "superadmin"];
  }

As far as I understand, admins can create ApiKey's. Does this mean that in this way an admin can get SuperAdmin rights this way? So for example the ApiKey would have superadmin roles and could also change the roles of the user who created the api key. Is that the case or have I overlooked something?

@wpf500
Copy link
Member Author

wpf500 commented Jan 24, 2024

Ah yeah I just found your question in my GitHub notifications, will also reply here :).

That's a good point and I'll downgrade API keys to admin. In practice the API only uses admin privileges at the moment anyway but definitely good to fix this now 👍

@wpf500
Copy link
Member Author

wpf500 commented Jan 24, 2024

Actually that makes me think that a much cleaner way of implementing this is just to make creating API keys for superadmins only, that way it doesn't need to do some arbitrary check of what the authentication source is. And it would also help prevent normal admins from accidentally creating API keys (or being tricked into it)

@JumpLink
Copy link
Contributor

JumpLink commented Jan 24, 2024

@wpf500 Another solution would be to also give API keys different roles. In this way, a user could create an API key that does not have admin rights (in the case that [selected] users should also be able to do this in the future) and an admin can only create API keys that have maximum admin rights etc. But that would probably be a lot more work and might be something for the Backlog.

@JumpLink
Copy link
Contributor

JumpLink commented Jan 24, 2024

Or an APIKey is linked to the user and simply always has the same rights as the linked user. But if an API key can also be deliberately restricted, this could be useful in some cases

@wpf500 wpf500 merged commit 7e6dca9 into master Jan 24, 2024
3 checks passed
@wpf500 wpf500 deleted the feat/917-response-validation branch January 24, 2024 14:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants