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.

Sign up for GitHub

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

Jump to bottom

Configure authentication at Build time #795

Merged
merged 2 commits into from
Oct 7, 2024
Merged

Configure authentication at Build time #795

merged 2 commits into from
Oct 7, 2024

Conversation

rmanibus
Copy link
Contributor

@rmanibus rmanibus commented Sep 12, 2024 •
edited by ricardozanini
Loading

Fix #797

Many thanks for submitting your Pull Request ❤️!

Please make sure that your PR meets the following requirements:

  • You have read the contributors guide
  • Your code is properly formatted according to our code style
  • Pull Request title contains the target branch if not targeting main: [0.9.x] Subject
  • Pull Request contains link to the issue
  • Pull Request contains link to any dependent or related Pull Request
  • Pull Request contains description of the issue
  • Pull Request does not include fixes for issues other than the main ticket
How to backport a pull request to a different branch?

In order to automatically create a backporting pull request please add one or more labels having the following format backport-<branch-name>, where <branch-name> is the name of the branch where the pull request must be backported to (e.g., backport-quarkus2 to backport the original PR to the quarkus2 branch).

NOTE: backporting is an action aiming to move a change (usually a commit) from a branch (usually the main one) to another one, which is generally referring to a still maintained release branch. Keeping it simple: it is about to move a specific change or a set of them from one branch to another.

Once the original pull request is successfully merged, the automated action will create one backporting pull request per each label (with the previous format) that has been added.

If something goes wrong, the author will be notified and at this point a manual backporting is needed.

NOTE: this automated backporting is triggered whenever a pull request on main branch is labeled or closed, but both conditions must be satisfied to get the new PR created.

@rmanibus rmanibus force-pushed the build_time branch 12 times, most recently from 67ccec9 to 74a5b9b Compare September 13, 2024 14:16
@rmanibus rmanibus marked this pull request as ready for review September 13, 2024 14:17
@rmanibus rmanibus force-pushed the build_time branch 9 times, most recently from 345641f to 56acf4b Compare September 14, 2024 21:18
@rmanibus
Copy link
Contributor Author

rmanibus commented Sep 14, 2024 •
edited
Loading

there is an issue with the fact that I made the OIDC Filter extension optional. I provided a temporary fix by removing the dependency to AbstractOidcClientRequestReactiveFilter and AbstractOidcClientRequestFilter (see 334c66f),
but there is probably a better way of doing this.

before the fix,
with rest easy classic the build hung with the following error:

2024-09-14 19:00:36,019 WARN [io.qua.arc.pro.BeanArchives] (build-17) Failed to index io.quarkus.oidc.client.reactive.filter.runtime.AbstractOidcClientRequestReactiveFilter: Class does not exist in ClassLoader QuarkusClassLoader:Deployment Class Loader: TEST for BeanValidationTest (QuarkusTest)@4bbf38b8

with rest easy reactive the build hung with the following error:

2024-09-14 18:54:22,716 WARN [io.qua.arc.pro.BeanArchives] (build-12) Failed to index io.quarkus.oidc.client.filter.runtime.AbstractOidcClientRequestFilter: Class does not exist in ClassLoader QuarkusClassLoader:Deployment Class Loader: TEST for BeanValidationTest (QuarkusTest)@1c30cb85

@rmanibus rmanibus force-pushed the build_time branch 2 times, most recently from a8a9251 to 334c66f Compare September 14, 2024 23:09
mcruzdev
mcruzdev approved these changes Sep 17, 2024
View reviewed changes
Copy link
Member

@mcruzdev mcruzdev left a comment

Choose a reason for hiding this comment

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

Hi @rmanibus, amazing Job! I just added a simple comment

@rmanibus rmanibus requested a review from a team as a code owner September 18, 2024 11:45
@rmanibus
Copy link
Contributor Author

@mcruzdev I fixed the imports !

@mcruzdev
Copy link
Member

@mcruzdev I fixed the imports !

I learned a lot, thank you for this pull request!

@rmanibus
Copy link
Contributor Author

@mcruzdev you are very welcome ! Any chance to get this approved again ? The force push dropped the review

hbelmiro
hbelmiro reviewed Sep 23, 2024
View reviewed changes
Copy link
Contributor

@hbelmiro hbelmiro left a comment

Choose a reason for hiding this comment

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

Thank you for this contribution @rmanibus. Nicely done!
I just left some aesthetical nits.

...e/src/main/java/io/quarkiverse/openapi/generator/providers/ApiKeyAuthenticationProvider.java Outdated Show resolved Hide resolved
...java/io/quarkiverse/openapi/generator/deployment/authentication/OpenApiSpecProviderTest.java Outdated Show resolved Hide resolved
...java/io/quarkiverse/openapi/generator/deployment/authentication/OpenApiSpecProviderTest.java Outdated Show resolved Hide resolved
...java/io/quarkiverse/openapi/generator/deployment/authentication/OpenApiSpecProviderTest.java Outdated Show resolved Hide resolved
...java/io/quarkiverse/openapi/generator/deployment/authentication/OpenApiSpecProviderTest.java Show resolved Hide resolved
...e/src/main/java/io/quarkiverse/openapi/generator/ClassicOidcClientRequestFilterDelegate.java Outdated Show resolved Hide resolved
client/runtime/src/main/java/io/quarkiverse/openapi/generator/OidcClient.java Outdated Show resolved Hide resolved
client/runtime/src/main/java/io/quarkiverse/openapi/generator/OpenApiSpec.java Outdated Show resolved Hide resolved
.../src/main/java/io/quarkiverse/openapi/generator/ReactiveOidcClientRequestFilterDelegate.java Outdated Show resolved Hide resolved
.../src/main/java/io/quarkiverse/openapi/generator/ReactiveOidcClientRequestFilterDelegate.java Outdated Show resolved Hide resolved
@rmanibus rmanibus force-pushed the build_time branch 3 times, most recently from 741b9c4 to 1dec5c4 Compare September 24, 2024 11:45
@rmanibus
Copy link
Contributor Author

@hbelmiro thanks a lot ! It is done !

hbelmiro
hbelmiro approved these changes Sep 25, 2024
View reviewed changes
Copy link
Contributor

@hbelmiro hbelmiro left a comment

Choose a reason for hiding this comment

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

Thank you @rmanibus.

fjtirado
fjtirado reviewed Sep 26, 2024
View reviewed changes
...rc/main/java/io/quarkiverse/openapi/generator/providers/CompositeAuthenticationProvider.java
public final void addAuthenticationProvider(final AuthProvider authProvider) {
this.authProviders.add(authProvider);
public CompositeAuthenticationProvider(List<AuthProvider> authProviders) {
this.authProviders = List.copyOf(authProviders);
Copy link
Contributor

@fjtirado fjtirado Sep 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Why do we need to copy this? As far as I can see authProviders is not modified, isnt it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted it to be immutable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is a public getter on it

@ricardozanini
Copy link
Member

ricardozanini commented Sep 26, 2024 •
edited
Loading

@wmedvede will also look since he made many changes to this code, too.

@rmanibus
Copy link
Contributor Author

rmanibus commented Oct 7, 2024

Any updates about this ?

@ricardozanini ricardozanini merged commit d2dd0ce into quarkiverse:main Oct 7, 2024
10 checks passed
@ricardozanini ricardozanini mentioned this pull request Oct 16, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fjtirado fjtirado fjtirado approved these changes

@mcruzdev mcruzdev mcruzdev approved these changes

@hbelmiro hbelmiro hbelmiro approved these changes

@ricardozanini ricardozanini ricardozanini approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Configure authentication at Build time
5 participants
@rmanibus @mcruzdev @ricardozanini @hbelmiro @fjtirado