search

delta-incubator / delta-sharing-rs

A Minimalistic Rust Implementation of Delta Sharing Server.
MIT License
77 stars 8 forks source link

[WIP] Improve authorization flow #19

Closed ognis1205 closed 6 months ago

ognis1205 commented 6 months ago

The authentication process should be handled in the front end and BFF, so the admin/auth middleware should be switched based on the feature flag to enable or disable it:

https://medium.com/@ognis1205/deltahub-data-sharing-platform-1-bdee60e74245

TODO:

The entire PR is going to be large, so I will follow the WIP pattern to reduce the load of the upcoming PRs. Therefore, this PR is marked as WIP just for sharing the context of the implementation. Any feedbacks including the options against the implementation would be welcome.

@roeap @rtyler

roeap commented 6 months ago

Hey @ognis1205 :)

Agreed, that we need to look into how the full authorization flow works to make it flexible to integrate with various providers.

One general approach I thought about is to also here split things up into smaller crates to "force" a clear separation of concerns. This could be under the generic deltalake namespace or in a sharing specific one. A straight forward example would be to have the protocol specific stuff in one crate, and the current admin stuff in another.

One aim would be that is becomes quite easy to "mix and match" the actual server instance by mounting individual sub-routers based on the specific needs of a given server instance.

I guess that would also make it quite straight forward to extend the server with delta-hub specific stuff as that project takes hold.

This "may" avoid feature flags and maybe allow to have customization largely done via config for the reference server implementation that we will eventually ship via a Docker container.

Specifically for auth I see two larger options (there likely are more).

  1. we make the auth middleware pluggable (via flags or config)
  2. we implement an auth provider set of APIs the do a token exchange. i.e. you bring your own token from an external provider, and we issue a token that should then be used for requests against the sharing APIs.

Currently I am leaning towards 2 as I believe it provides a better separation of concerns. If we get a split between the crates mentioned above right, we may be able to also support both. e.g. have the default server work with pattern 2, but make it very simple to build a server that works with the first pattern.

as for renaming admin -> catalog, I would be fine with renaming it, but catalog is such a heavily used term that we may want to think about if there is a clearer term available?

Or split the admin API as well into an auth part that deals with negotiation permissions, and a catalog part where new shares etc can be registered.

Let me know what you think...

ognis1205 commented 6 months ago

Hi @roeap Thank you for sharing your insights!

One general approach I thought about is to also here split things up into smaller crates... Currently I am leaning towards 2 as I believe it provides a better separation of concerns. If we get a split between the crates mentioned above right, we may be able to also support both. e.g. have the default server work with pattern 2, but make it very simple to build a server that works with the first pattern.

Please give me a little more time, including the design proposal. I will create materials that can be shared by the end of this weekend. I think you have talked to me a bit before, and I'll also use that as a reference:

https://github.com/roeap/flight-fusion/tree/main/rust/azure-jwt-auth

as for renaming admin -> catalog, I would be fine with renaming it, but catalog is such a heavily used term that we may want to think about if there is a clearer term available?

I will reconsider the naming as well!

ognis1205 commented 6 months ago

@roeap

By the way, what use cases is you considering when implementing the auth/oidc-related crates?

The main purpose of this pull request is to replace all the login-related middleware implemented in the admin-related API of delta-sharing-rs. These are currently implemented in the frontend/BFF (delta-hub-ts) using next-auth in compliance with oauth2.0/oidc.

The goal is to either remove or replace all login-related middleware in delta-sharing-rs using feature flags, allowing for specific changes based on use cases. The plan was to address the necessary authorization flow for delta sharing by having delta-sharing-rs act as an authorization server, issuing its own tokens.

roeap commented 6 months ago

@ognis1205

Well, the goal is always to implement as little as possible ourselves :).

What you described is more or less what i meant with patter 2. We leverage some external oauth/oidc provider to have users login (and negotiate permissions). Then some delts-sharing specific token is issued. This would be kind of like the token exchange in the oauth protocol. And exactly as you said, the sharing server would then also act as an auth server. In this case I guess the auth middleware for that sharing APIs would always be the same, as it always needs to validate our internal tokens.

The other approach would be to also allow for just fully integrating with other oauth providers - i.e. rather then issuing a specific token, users get a token form the provider, and delta sharing just validates ... This would add some complexity around modelling permissions in the external service, so I guess the previous approach is easier for now.

Since it is modelled via middlewares we likely are already quite pluggable.

ognis1205 commented 6 months ago

@roeap

Well, the goal is always to implement as little as possible ourselves :). What you described is more or less what i meant with patter 2

Thank you for the clarification, and I apologize for my ambiguous explanation.

The other approach would be to also allow for just fully integrating with other oauth providers - i.e. rather then issuing a specific token, users get a token form the provider, and delta sharing just validates ...

This idea sounds good too! Alongside implementation, I will also explore technical validation and design. I will share any progress with you.

Thanks again!

ognis1205 commented 6 months ago

Recapped TODO:

As for the term catalog, the APIs under catalog/ are used for registering new shares(or catalogs) etc as @roeap mentioned. I think it would be faster if you take a look at the pull request, so I'll proceed with this. If you have any additional feedback, let's discuss it after reviewing the implementation in the pull request.

roeap commented 6 months ago

refactor DB schema, e.g., password on account table should be optional

not sure what you opinion is on this, but since we have never actually released from this repo, we may just skip building up migrations for the changes we make before initial release?

Not sure where I personally stand on this yet, but something to think about :)

ognis1205 commented 6 months ago

@roeap

not sure what you opinion is on this, but since we have never actually released from this repo, we may just skip building up migrations for the changes we make before initial release?

As for password column on account table, I am going to remove the login functionality (auth) from the delta-sharing-rs implementation because frontend/bff should handle the auth but some use-cases might still need this functionality with the opt-in feature flag so I leave it optional column. It might be better to completely remove the functionality.

Regarding the skipping migrations, just to clarify, are asking for a more compact migration process, that merges all migration files in a single one, and only opens a single transaction to modify the database at once? If so, I totally agree with you.

ognis1205 commented 6 months ago

@roeap According to your comment:

https://github.com/ognis1205/delta-sharing-rs/pull/5#issuecomment-1851467298

it seems like you have implemented several updates. Would you like to merge this here once? What do you think? I marked this PR to be ready to merge for now.