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

Support http authorization in asyncapi generation #1145

Merged
merged 108 commits into from
Jul 13, 2024
Merged

Support http authorization in asyncapi generation #1145

merged 108 commits into from
Jul 13, 2024

Conversation

akrambek
Copy link
Contributor

No description provided.

akrambek and others added 30 commits October 25, 2023 13:35
…artial data frame while computing crc32c value
Comment on lines 175 to 178
if (isOauthEnabled)
{
options.authorization(authorization).build();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to call .build() here?

@@ -52,7 +52,7 @@ public NamespaceConfig generate(
.inject(n -> injectTcpServer(n, servers, options, metricRefs))
.inject(n -> injectTlsServer(n, servers, options))
.inject(n -> injectProtocolRelatedBindings(n, servers, metricRefs))
.inject(n -> injectProtocolServers(n, servers, metricRefs))
.inject(n -> injectProtocolServers(n, binding.namespace, options, servers, metricRefs))
Copy link
Contributor

Choose a reason for hiding this comment

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

This is correct to pass binding.namespace, but parameter is called qname, which is not consistent.

Suggest we add a transient qname field to HttpAuthorizationConfig and set during AsyncapiNamespaceGenerator.init(BindingConfig), to specify the qualified name of the guard being used for authorization in the binding namespace.

Then this parameter can be removed from the call stack, and the HttpAuthorizationConfig.qname can be used where we originally attempted to use HttpAuthorizationConfig.name, but now qname will just work.

I'm planning to implement this generically in the engine so you wouldn't need to explicitly set qname, but it will help tidy the code if we do it this way for now and I can remove the line where you set qname when I'm adding the general approach later.

}
route
.guarded()
.name(String.format("%s:%s", qname, options.authorization.name))
Copy link
Contributor

Choose a reason for hiding this comment

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

Based on other feedback above, this becomes:

Suggested change
.name(String.format("%s:%s", qname, options.authorization.name))
.name(options.authorization.qname)

@jfallows jfallows merged commit 1a0fd5d into aklivity:develop Jul 13, 2024
5 checks passed
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.

2 participants