Skip to content

APP-2985: Protos for endpoint to register fusionauth applications #460

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

Merged
merged 24 commits into from
Apr 4, 2024

Conversation

jr22
Copy link
Member

@jr22 jr22 commented Mar 13, 2024

APP-2985

Created protos to register fusion auth apps. Not sure if end_user is the right place for this? was thinking maybe but still getting acquainted with this stuff so open to feedback if it should be moved elsewhere.

Comment on lines 185 to 195
type RegisterApplicationRequest struct {
state protoimpl.MessageState
sizeCache protoimpl.SizeCache
unknownFields protoimpl.UnknownFields

ApplicationName string `protobuf:"bytes,1,opt,name=application_name,json=applicationName,proto3" json:"application_name,omitempty"`
ApplicationId string `protobuf:"bytes,2,opt,name=application_id,json=applicationId,proto3" json:"application_id,omitempty"`
OrgId string `protobuf:"bytes,3,opt,name=org_id,json=orgId,proto3" json:"org_id,omitempty"`
OriginUris []string `protobuf:"bytes,4,rep,name=origin_uris,json=originUris,proto3" json:"origin_uris,omitempty"`
RedirectUris []string `protobuf:"bytes,5,rep,name=redirect_uris,json=redirectUris,proto3" json:"redirect_uris,omitempty"`
}
Copy link
Member Author

Choose a reason for hiding this comment

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

@ohEmily could you weigh in on these params and if they make sense?

Copy link
Member

Choose a reason for hiding this comment

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

Curious about omitempty. I thought we shouldn't do omitempty for OriginUris, RedirectUris, OrgId and ApplicationName, unless we intend to do app-side validation for those. Wouldn't omitempty mean that the fields are optional? Out of these, I believe ApplicationId is the only optional one that should have omitempty.

Copy link
Member Author

Choose a reason for hiding this comment

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

think i commented on the wrong thing here but these were the autogenerated protos so dont think i have control over the omitempty

@jr22 jr22 requested a review from ohEmily April 2, 2024 20:21
Comment on lines 15 to 16
// Allows users to register third party applications using Viam linked to the indicated organization
rpc RegisterApplication(RegisterApplicationRequest) returns (RegisterApplicationResponse);
Copy link
Member

Choose a reason for hiding this comment

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

[minor] can we make it RegisterAuthApplication?


message RegisterApplicationRequest {
string application_name = 1;
string application_id = 2;
Copy link
Member

@ohEmily ohEmily Apr 3, 2024

Choose a reason for hiding this comment

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

Continuing on my comment that I made earlier, I initially thought application_id should be marked optional. Although thinking about it more, maybe we should just omit it entirely and force using an auto-generated application ID. The reason is that this is required to be globally unique, and then we don't have to handle customers using an application ID like "app" and wrapping the FusionAuth response to give a detailed exception.

Copy link
Member Author

@jr22 jr22 Apr 3, 2024

Choose a reason for hiding this comment

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

from the ticket description:

Our gRPC endpoint should have application name and redirect URIs should be required parameters. (i.e. don’t use FusionAuth’s built in feature of generating a random client ID)

was thinking this meant we wanted them to pass an application id from the parenthesis, may have misunderstood. can remove if thats not the case!

Copy link
Member

@ohEmily ohEmily left a comment

Choose a reason for hiding this comment

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

LGTM but I think we should remove application_name to save ourselves the additional effort. To answer your open question, I think putting this in end_user.proto is perfect.

@jr22
Copy link
Member Author

jr22 commented Apr 3, 2024

LGTM but I think we should remove application_name to save ourselves the additional effort. To answer your open question, I think putting this in end_user.proto is perfect.

assume you mean application_id not application_name?

@jr22 jr22 added bug Something isn't working and removed bug Something isn't working labels Apr 4, 2024
@jr22 jr22 added protos-compiled bug Something isn't working and removed protos-compiled labels Apr 4, 2024
@jr22 jr22 removed the bug Something isn't working label Apr 4, 2024
Copy link
Member

@ohEmily ohEmily left a comment

Choose a reason for hiding this comment

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

Yes I meant application_id. Sorry about that!

@jr22 jr22 added bug Something isn't working and removed bug Something isn't working labels Apr 4, 2024
@jr22
Copy link
Member Author

jr22 commented Apr 4, 2024

(ty katherine, trying to help me figure out this protos compiled thing)

@jr22 jr22 added bug Something isn't working and removed bug Something isn't working labels Apr 4, 2024
@jr22 jr22 merged commit 7415469 into viamrobotics:main Apr 4, 2024
4 checks passed
@jr22 jr22 deleted the APP-2985 branch April 4, 2024 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants