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

Add Optional Support for Rocket-Okapi in IntrospectedUser #559

Merged
merged 14 commits into from
Sep 26, 2024

Conversation

NewtTheWolf
Copy link
Contributor

@NewtTheWolf NewtTheWolf commented Jun 9, 2024

Draft: Add Optional Support for Rocket-Okapi in IntrospectedUser

Overview

This pull request introduces the initial implementation of optional Rocket-Okapi support for the IntrospectedUser type within the zitadel-rust project. The goal is to enable automated OpenAPI documentation generation for Rocket-based applications using this type.

#558

Current Implementation

I have added preliminary code to integrate Rocket-Okapi, which is currently functional but still requires further review and enhancements. Here are the specific changes made:

  • Cargo.toml: Added rocket_okapi as an optional dependency.
  • src/rocket/introspection/guard.rs: Implemented OpenApiFromRequest for IntrospectedUser, including security scheme setups and default responses.

Status

The current implementation works as intended, enabling the IntrospectedUser to be utilized within the OpenAPI framework provided by Rocket-Okapi. However, this is still a draft, and the implementation is open for review and suggestions on improvements.

Next Steps

  • Documentation: I will add comprehensive documentation explaining how to enable and utilize this feature.
  • Examples: I plan to provide examples demonstrating the practical use of this integration.
  • Testing: Comprehensive tests will be developed to ensure reliability and to prevent any potential impact on existing functionalities.

Invitation for Feedback

I am actively seeking feedback on the current implementation and suggestions on how we can further refine this feature. Any input on additional aspects to consider, potential issues, or improvements would be greatly appreciated.

Thank you for reviewing this draft pull request. I look forward to your suggestions and contributions to finalize this feature.

@NewtTheWolf NewtTheWolf requested a review from buehler July 29, 2024 07:47
@FrTerstappen
Copy link
Contributor

Hello @NewtTheWolf,

this is a very nice proposal. This helped me a lot as I needed an OpenApi spec for my API implemented with rocket using zitadel.

While this allows the usage it seems to not work without problems.
I am unable to use the generated swagger documentation to login.
Is this a problem you are aware of or does it work for you?
If the later is the case it may be a problem of my zitadel configuration or project setup.

The other problem is that a typescript client generated from a spec made with this using the https://github.com/OpenAPITools/openapi-generator tool does not send the authorization header by default.
If I add the access_token manually to the request headers everything works perfectly.
This may also be a problem with the generation or my usage of it.
Do you use something similar and can confirm that it works?

I would also propose to implement the second method get_responses from the OpenApiFromRequest trait.
This allows you to add additional statuses to what can be returned by an endpoint requiring this request guard.
A 401 Unauthorized sounds like a addition that makes sense on every request with this guard.
But there may also be others that make sense like a bad request if no authorization header is being send.

@NewtTheWolf
Copy link
Contributor Author

Hello @FrTerstappen,

Is this a problem you are aware of or does it work for you?

My primary focus for my own project was to continue development without needing to create a New Type, so it hasn’t been thoroughly tested yet. I'll make it a priority to test this issue and try to fix it as soon as possible.

Do you use something similar and can confirm that it works?

I haven't encountered this use case yet, but I will test it and try to reproduce the issue. If I can replicate it, I’ll work on a fix.

I would also propose to implement the second method get_responses from the OpenApiFromRequest trait.

I hadn’t thought of that—great idea! Implementing additional status codes, like a 401 Unauthorized or a bad request when no authorization header is sent, definitely makes sense.

My biggest challenge right now is understanding how to properly use the configuration that this client is intended for. This client is based on Code Config, meaning that you configure it within your code instead of using an external configuration file like a .env or a Rocket.toml. Do you have any ideas on how to solve the configuration problem by any chance?

@NewtTheWolf
Copy link
Contributor Author

I’m considering adding two additional features, "rapidoc" and "swagger", to the project. The idea is that only one of these features would be active at a time:

  • rapidoc: This feature would be used when OAuth2 is required, as it works best with RapiDoc.
  • swagger: This feature would be used when OpenID Connect is needed, as it only functions correctly with Swagger.

Would this approach make sense, given the specific requirements for each tool? Any feedback or suggestions would be appreciated!

@NewtTheWolf
Copy link
Contributor Author

@FrTerstappen

Is this a problem you are aware of, or does it work for you?

I tested it today, and the login worked without any issues. You'll need to add the following to your Rocket.toml:

[default]
authority = "https://auth.yourzitadel.de"

Also, make sure to add the redirect URL to http://127.0.0.1:8000/swagger/oauth2-redirect.html. The /swagger part and the port should be adjusted based on your specific configuration.

@NewtTheWolf
Copy link
Contributor Author

Just created GREsau/okapi#151 to ask for help regarding configuration access within the OpenApiFromRequest implementation in rocket_okapi.

@FrTerstappen
Copy link
Contributor

@NewtTheWolf

Thank you for your feedback. I will take a deeper look at your changes and test them on Monday.
But a first look at the changes looks good to me.

Now that you have confirmed that the login on the swagger docs should work I will invest some time to find out why it did not work for me.
But seeing your configuration for the redirect url and having my doc mounted under a different path this may already be the problem.

Also, make sure to add the redirect URL to http://127.0.0.1:8000/swagger/oauth2-redirect.html. The /swagger part and the port should be adjusted based on your specific configuration.

I'm not sure I understand what you want to use the two proposes features for.
The decision to use rapidoc or swagger is made by the consumer of this library and there is no setup for included here.

Or do you want to adjust the schema for the login in the docs based on which is activated?
As I only use swagger at the moment I have not seen the need for adjustments but it seems like a sensible approach to have these two features to support the different authentication modes.
But if this is the case I would propose calling them like the authentication modes as that is a better description of what they do.
Something like rocket_okapi_oicd which would include the rocket_okapi feature.

I also do not know how to solve the problem of configuration.
It seems to be a use case not really supported by the okapi crate.
As such it should be the right place to ask for a solution like you have done.

The current solution of specifying the authority in the Rocket.toml file works good enough for me.
As it is a file I already have for other configuration that gets adjusted on deployment it is a nice fit for my use case.

@NewtTheWolf
Copy link
Contributor Author

@FrTerstappen

Thanks for your feedback, keep me updated!

I'm not sure I understand what you want to use the two proposed features for.

The main difference (in docs) between OAuth2 and OIDC is that OIDC does not work in Rapidoc and vice versa. My idea is to allow activating the feature that best suits your preferred documentation. In case you configure both docs in your Rocket app, you wouldn't be able to have both activated in zitadel-rust. This is why I think it makes sense to have these features configurable.

The current solution of specifying the authority in the Rocket.toml file

I think it would also be a great idea to support a general function to configure the client, like with .with_config(), because the Rocket.toml file is the intended place to configure your Rocket application. You could then use keys like oauth2.zitadel(.authority, client_id, client_secret, etc....).

What do others think about this approach?

@FrTerstappen
Copy link
Contributor

@NewtTheWolf

Adjusting the redirect URL helped.

Having features for the different authentication schemes or for the different viewer for the OpenAPI spec (swagger, rapidoc, ...) seems like good idea.
But this would need to be shown prominently in the documentation as it will lead to people using a non working combination of authentication and documentation viewer if not communicated.

I also tried your latest changes and while the additional response types show up I realized that you also specified a schema for the response.
Does this match what is returned by the guard in an error case?
The errors also look overly specific.

Bad Request - Multiple authorization headers found.
Unauthorized - The request requires user authentication.

Looking at the errors used by the guard it seem like many errors are possible.

custom_error! {
    /// Error type for guard related errors.
    pub IntrospectionGuardError
        MissingConfig = "no introspection config given to rocket managed state",
        Unauthorized = "no HTTP authorization header found",
        InvalidHeader = "authorization header is invalid",
        WrongScheme = "Authorization header is not a bearer token",
        Introspection{source: IntrospectionError} = "introspection returned an error: {source}",
        Inactive = "access token is inactive",
        NoUserId = "introspection result contained no user id",
}

I'm not sure if the best way is to add them all or to just specify a more generic version of the error, e.g. only Bad Request

The errors also do not reflect all status codes used by the guard.
For example in https://github.com/NewtTheWolf/zitadel-rust/blob/feature/rocket-okapi/src/rocket/introspection/guard.rs#L141 the guard return an internal server error.
I'm not sure what is best practice and how this is generally handled in rocket_okapi but I feel like all codes used in the guard should be reflected in the possible responses.

@NewtTheWolf
Copy link
Contributor Author

@FrTerstappen,

I’m glad to hear that adjusting the redirect URL helped!

Regarding the error handling, I agree that generalizing the Bad Request error makes sense.

I was thinking of proposing adding a struct with JsonSchema for errors definition. This could simplify the spec management and reduce boilerplate. What do you think of this approach? Do you think it might be too much?

I’ll handle the additional features in a separate pull request. For now, my focus will be on Swagger users since that’s what I primarily use.

@FrTerstappen
Copy link
Contributor

@NewtTheWolf

It was actually changing the redirect URL in combination with adding a key to my app in zitadel.
I'm setting up zitadel from the application when it gets deployed.
The api call to add the frontend app gives me something that looks like the secret and was enough for my frontend to work.
Making an extra request to generate a new key finally gave me something that also works in swagger.
I think my problems come mostly from my zitadel setup and has little to do with swagger or the generation of the documentation.

I think having a struct with JsonSchema for the errors sounds like a good idea.
That is also what I do in the rest of my project.

I think focusing on swagger and doing everything else in a follow up PR sounds like a good idea to move forward.
It may even be worth it to remove the additional responses again and also add them in a separate PR later as this is probably a decision that the maintainer(s) need to make.

FrTerstappen pushed a commit to FrTerstappen/zitadel-rust that referenced this pull request Sep 8, 2024
@NewtTheWolf
Copy link
Contributor Author

@buehler

At the moment, this is the best solution we can achieve. I believe this pull request is ready for a final review. I will keep track of the Issue on okapi

Everything else, like an error struct for doc generation, the generic 403 response, and Rapidoc, I would implement in separate pull requests.

I'll change the status to "Ready for Review" after a quick check to make sure the code is clean.

@NewtTheWolf NewtTheWolf marked this pull request as ready for review September 18, 2024 13:50
@NewtTheWolf NewtTheWolf changed the title Draft: Add Optional Support for Rocket-Okapi in IntrospectedUser Add Optional Support for Rocket-Okapi in IntrospectedUser Sep 24, 2024
@buehler
Copy link
Collaborator

buehler commented Sep 26, 2024

Hi @NewtTheWolf
This seems to be reasonable :)

@buehler buehler merged commit 2fb5786 into smartive:main Sep 26, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants