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

Improved authorization demo #2830

Closed
wants to merge 32 commits into from
Closed

Conversation

bentoi
Copy link
Contributor

@bentoi bentoi commented Mar 28, 2023

This PR improves the authorization demo. Fixes #2820.

Copy link
Member

@pepone pepone left a comment

Choose a reason for hiding this comment

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

What are we trying to show with this example? I think that authorization and authentication cannot be an afterthought in IceRPC, which this example and the previous one give the impression that each application needs to roll out its own authentication and authorization mechanism. IceRPC should use oauth2 and JWT tokens and don't reinvent the well, I would rather remove this example, and later provide a solution that is based on well-known standards like the ones I mentioned. This can probably wait until the 0.2 release, but as it is this example gives the impression that this is a proven solution for authorization which is not.

@bentoi
Copy link
Contributor Author

bentoi commented Mar 28, 2023

This demo demonstrates how authorization can be implemented with IceRPC. For authorization to work, you need the client to authenticate. While ASP.NET provides mechanisms for oauth2 or JWT tokens, it would be too complicated to use these APIs in an IceRPC example. In this example, the token is quite simple and is encoded with Slice2.

JWT and oauth2 are designed for web application which is not our primary target. To support oauth2 or JWT, I would also do like ASP.NET and provide an authentication module that provides an interceptor and middleware as well as an abstraction to support multiple authentication schemes (what Microsoft calls authentication bearers).

@ReeceHumphreys
Copy link
Contributor

ReeceHumphreys commented Mar 28, 2023

I would rather remove this example, and later provide a solution that is based on well-known standards like the ones I mentioned.

I agree with @pepone on this. This example is complicated and does not demonstrate how users should actually perform authentication. The goal of the examples should be to illustrate some feature or functionality of IceRpc in the simplest way that uses all of our best practices. Performing authentication this way is not a best practice.

Edit: Perhaps we could rework this example into some other form of content for users, like a blog or tutorial explaining how to write your own middleware and interceptors.

@bentoi
Copy link
Contributor Author

bentoi commented Mar 28, 2023

This example is complicated and does not demonstrate how users should actually perform authentication.

I don't think this example is more complex that the previous session example and it is a good way to demonstrate interceptor and middleware implementation.

It provides the basic principles for authentication/authorization... It's by no means a complete implementation. Just like the implementation of the authenticate method isn't a complete implementation for login/password check 🙂.

@bentoi bentoi requested review from pepone and bernardnormier March 29, 2023 09:03
@bentoi bentoi requested a review from pepone March 31, 2023 17:13
@bentoi
Copy link
Contributor Author

bentoi commented Apr 3, 2023

I did some renaming. AuthenticationBearer wasn't correct. I renamed it IBearerAuthenticationHandler. It handles the token generation and the token validation. It's consistent with the .NET and ASP.NET naming for these interfaces. I realize that we could instead split this in three different interfaces (factory, validator and class to hold the shared info) but I think it would be to complicated for a demo.

@bentoi bentoi requested a review from pepone April 3, 2023 11:56
@bernardnormier
Copy link
Member

I find this reworked example way too complicated. It's not clear to me what this example is trying to demonstrate.

@bentoi
Copy link
Contributor Author

bentoi commented Apr 3, 2023

I find this reworked example way too complicated. It's not clear to me what this example is trying to demonstrate.

I've reworked this example multiple times so it's not clear to me if you are referring to the original session example or a previous update of the example from this PR.

The goal of this change is described on the issue #2820

I'm fine with supporting a single authentication token type. Which one do you prefer?

@bentoi bentoi requested a review from bernardnormier April 4, 2023 13:21
@bentoi
Copy link
Contributor Author

bentoi commented Apr 4, 2023

I think at this point we have to decide if we want to keep this example or not. If we don't keep it, we'll need to decide what to do with the existing authorization demo based on sessions.

@bernardnormier
Copy link
Member

On main:

github.com/AlDanial/cloc v 1.96  T=0.02 s (825.9 files/s, 22003.3 lines/s)
------------------------------------------------------------------------------------
Language                          files          blank        comment           code
------------------------------------------------------------------------------------
C#                                   10             63             59            162
Visual Studio Solution                1              1              1             34
Markdown                              1             11              0             23
MSBuild script                        2              0              0             19
------------------------------------------------------------------------------------
SUM:                                 14             75             60            238
------------------------------------------------------------------------------------

on your PR:

------------------------------------------------------------------------------------
Language                          files          blank        comment           code
------------------------------------------------------------------------------------
C#                                   15            100             94            360
Visual Studio Solution                1              1              1             34
Markdown                              1             15              0             31
MSBuild script                        2              0              0             25
------------------------------------------------------------------------------------
SUM:                                 19            116             95            450
------------------------------------------------------------------------------------

For me, it's essential that each example:

  • remains simple and focused
  • shows best practices

This new version has become too big in my view.

There are also some issues with "best practices". For example, we should not mix-up users (Alice, Bob, Charlie) and roles/groups (Admin, Authenticated Users).

I think at this point we have to decide if we want to keep this example or not. If we don't keep it, we'll need to decide what to do with the existing authorization demo based on sessions.

I think we need a simple, focused, best practices example; unfortunately, we're not there yet.

@bentoi
Copy link
Contributor Author

bentoi commented Apr 4, 2023

I think we need a simple, focused, best practices example; unfortunately, we're not there yet.

This example is focused on showing best practice for implementing authorization. It is indeed not simple because it requires to also show best practice to implement authentication.

So what do you suggest?

  1. Simplify this example by getting rid one of the token handler implementation? Which one?
  2. Simplify this example by getting rid of authentication all together (not really best practice)
  3. Keep the existing session based example? (not best practice)
  4. Get rid of the authorization example and demonstrate a simpler interceptor/middleware implementation?

@bernardnormier
Copy link
Member

So what do you suggest?

  1. Simplify this example by getting rid one of the token handler implementation? Which one?
  2. Simplify this example by getting rid of authentication all together (not really best practice)
  3. Keep the existing session based example? (not best practice)
  4. Get rid of the authorization example and demonstrate a simpler interceptor/middleware implementation?

I don't know. If we keep this example, we should definitely keep only JWT: I find the encoding/decoding with AES ultra complicated.

We would also need more README and comments. E.g. the addition of a custom header for "isAdmin" with JWT definitely deserves a comment.

@bentoi
Copy link
Contributor Author

bentoi commented Apr 4, 2023

I don't know. If we keep this example, we should definitely keep only JWT:

Why? JWT, OAuth, etc are very much web application oriented and IceRPC isn't designed to build web applications. Transferring tokens as JSON data in a request field isn't really the way to go with IceRPC.

The pros with JTW: it provides token validation features (token expiration, etc). The cons: it's encoded as JSON.

I find the encoding/decoding with AES ultra complicated.

I don't see what's more complicated than the writing/reading of the JTW token.

Can you be more specific? Is it because the use of streams? Developers using IceRPC should have basic understanding of streams. I've simplified a bit the encryption of the Slice identity token, hopefully you'll find it is simpler.

The pros with AES+Slice: no need to parse strings, etc. You can just decode a Slice2 encoded token. Cons: it requires to implement additional validation features that your IceRPC application might need (token expiration, etc).

@bentoi
Copy link
Contributor Author

bentoi commented Apr 5, 2023

I'm closing this PR since there's no consensus on the best approach to tackle authorization.

There are many ways to implement authorization:

  • JWT tokens (not encrypted but signed, can carry state),
  • AES tokens (encrypted so opaque to the client, can carry state),
  • session tokens (opaque identifier, state is kept on the server side),
  • probable others

I believe all these methods are used for micro-services authorization and have pros and cons.

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.

Improve authorization demo
5 participants