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

application.get: Add support for retrieving applications by uuid #1074

Merged
3 commits merged into from
Dec 30, 2021

Conversation

thgreasi
Copy link
Member

@thgreasi thgreasi commented Apr 7, 2021

Originally draft just b/c I still haven't added tests.

Resolves: #1016
Change-type: minor
Signed-off-by: Thodoris Greasidis thodoris@balena.io


Contributor checklist
  • Includes tests
  • Includes typings
  • Includes updated documentation
  • Includes updated build output

@thgreasi thgreasi self-assigned this Apr 7, 2021
@thgreasi thgreasi requested a review from pdcastro April 7, 2021 22:11
slug: nameOrSlugOrId.toLowerCase(),
app_name: nameOrSlugOrUuidOrId,
slug: nameOrSlugOrUuidOrId.toLowerCase(),
uuid: nameOrSlugOrUuidOrId.toLowerCase(),
Copy link
Member Author

Choose a reason for hiding this comment

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

This is essentially the only code change.

Copy link

Choose a reason for hiding this comment

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

To confirm, this only accepts "long" or "full" UUID, right? Not something like decade2 which is a valid 7-char short UUID. (I think that accepting short UUIDs here would be bad, creating more ambiguity that goes in the opposite direction of the spirit of #1030.) By the way, we discussed before (not necessarily decided, at least discussed) that there should be a way for the SDK to allow the caller (CLI/UI/user app) to query specifically and unambiguously by UUID or slug or name or ID, instead of nameOrSlugOrUuidOrId. For example, instead of:

    nameOrSlugOrUuidOrId: string | number

Perhaps it could be:

    nameOrSlugOrUuidOrId: string | number | object

And if it was an object, the object would be exactly one of:

  • { id: number }
  • { uuid: string }
  • { name: string }
  • { slug: string }

But, fair enough, a long / full UUID can be assumed to never legitimately clash with an unintended resource. I say "legitimately" because a bad user could copy and paste the UUID of an app and set it as the name of another app, in which case this query would be "illegitimately ambiguous" when nameOrSlugOrUuidOrId was a string.

Copy link

Choose a reason for hiding this comment

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

Or maybe not that harmless... A bit far fetched and just hypothetical, but could user A create a sort of denial of service attack on user B by creating an app that was named after the UUID of one of user B's apps? User A could become aware of the UUID of an app of user B for being a collaborator / observer of that app.

Or, what about public apps? If the UUID of a public app is public, could I create a DoS attack on the public app by naming another public app (of mine?) after that UUID? Such that people's query for the legit UUID would return my illegit app (by name) instead. I haven't tested, just hypothetical.

Copy link
Member Author

@thgreasi thgreasi Apr 8, 2021

Choose a reason for hiding this comment

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

Yes it only accepts full UUIDs, for the reasons you mentioned.
I did have in mind the | AlternateKeyObject, but decided to not implement it in this PR, since I wouldn't want to also support names in that notation, since technicaly the name is not an alternate key (aka table wide natural key).

The denial of service scenario you describe is already possible by having user B name their app with the same name as user A. But user A can in that case use the app slug. This is going to be fully resolved though with #1030.

Copy link
Member Author

Choose a reason for hiding this comment

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

@pdcastro let me know if you would prefer to not merge this before the next major where we drop support for retrieving apps by name in this method.

Copy link

Choose a reason for hiding this comment

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

The denial of service scenario you describe is already possible by having user B name their app with the same name as user A. But user A can in that case use the app slug.

Is it really the same thing? The case of clashing names (multiple apps with the same name) can be resolved by using the app slug which the recommended practice, but for the scenario I described, querying by app UUID does not solve the problem -- and to say "use the slug instead of the UUID" sounds like a design failure. It sounds like we are saying that querying by app UUID could be ambiguous, an ambiguity being introduced by this PR / solution.

if you would prefer to not merge this before the next major

I'm not worried about that, but I am worried about ambiguity when querying by UUID. I'd rather this PR implemented a solution where querying by UUID could not possibly be ambiguous, or subject to DoS.

@thgreasi thgreasi marked this pull request as ready for review April 8, 2021 10:31
@thgreasi thgreasi requested a review from a team as a code owner April 8, 2021 10:31
@thgreasi thgreasi marked this pull request as draft April 9, 2021 10:24
Change-type: patch
Signed-off-by: Thodoris Greasidis <thodoris@balena.io>
Resolves: #1016
Change-type: minor
Signed-off-by: Thodoris Greasidis <thodoris@balena.io>
Signed-off-by: Thodoris Greasidis <thodoris@balena.io>
@thgreasi thgreasi marked this pull request as ready for review December 30, 2021 13:28
@thgreasi
Copy link
Member Author

@balena-ci I self-certify!

@ghost ghost merged commit 6d0df38 into master Dec 30, 2021
@ghost ghost deleted the 1016-app-get-uuid branch December 30, 2021 14:12
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Should support querying applications by UUID
2 participants