syndesisio / syndesis-react

[FORK] A flexible, customizable, open source platform that provides core integration capabilities as a service.
Apache License 2.0
3 stars 19 forks source link

🛠 Displaying & validating multiple OAuth 2.0 security schemes in API Client Connector #411

Open kahboom opened 5 years ago

kahboom commented 5 years ago

This is a...


[ ] Feature request
[ ] Regression (a behavior that used to work and stopped working in a new release)
[ x ] Bug report  
[ ] Documentation issue or request

Description

In the API Client Connector wizard, security schemes are presented just with their labels (API Key, OAuth 2.0, etc.), so when an API specification has more than one security scheme that uses OAuth, these options are presented identically, as shown below.

You can only choose either an an Authorization URL or an Access Token URL, not both, since you are selecting the security you want for the Connection. It'd make sense just to show one of the fields, in separate options, or only one "OAuth 2.0" option that shows both fields with a radio button as options.

The way the data is returned from the BE makes it difficult to separate them into two options, but it's possible. My question is how we should be displaying this and validating it.

@syndesisio/uxd

Screenshot 2019-06-03 19 37 56

It's quite glitchy in the Angular app as well, and you are allowed to edit both fields, which I don't think is the intended behavior. In this case, the same question would apply even if it's just one OAuth scheme provided by the spec, because both fields are shown (also likely due to the way the data is received).

Screenshot 2019-06-04 16 38 26
dongniwang commented 5 years ago

@zregvart - Maybe you can help provide some information here as well?

I think initially, we only had No security, HTTP Basic Authentication, and OAuth. Not sure why there are two OAuth2.0 in the screenshots. As for the Authorization URL and Access Token URL, I don't have enough knowledge to say yes they are the same or no they're not. If they're essentially the same, we should use radio buttons here to indicate they are mutually exclusive.

Just FYI, here's the original design https://github.com/syndesisio/syndesis/blob/master/ux/designs/apiconnector/apiconnector.md

zregvart commented 5 years ago

The choices are driven by the security definitions in a given OpenAPI document the user provides.

Our story around mixing different security schemes is somewhat thin; meaning that we can support only one security scheme even though operations defined by the OpenAPI document support a mix of them.

I think having several OAuth 2.0 schemes defined is probably only the case when different OAuth flows are defined, in that case it makes sense that if multiple OAuth security definitions are present in the OpenAPI document we pick the only flow we support (the accessCode).

Can I borrow on your expertise @EricWittmann to confirm this?

This would be a backend issue to solve.

@kahboom can you attach or point to the OpenAPI document you tested with to https://github.com/syndesisio/syndesis/issues/5592?

EricWittmann commented 5 years ago

If you're displaying all of the security schemes defined in the OpenAPI doc, then you could show the name of the scheme (a label associated with the scheme by the API designer).

It sounds like you're ignoring the concept of the Security Requirements because you don't support multiple schemes, which I totally get. And I presume you also don't support different security requirements for different operations, which OpenAPI allows. So let's ignore that for now.

I agree that if accessCode is the only OAuth flow supported, then any other OAuth flows should probably be ignored (not shown in the UI).

Also I think that the relevant part of Dongni's design makes sense to me: securityconfigure

Except that the two fields should be something like Authorization URL and Token URL. Those two fields should only be visible when the user selects the OAuth scheme (rather than e.g. Basic Auth). And both fields should be editable and pre-filled with whatever values came from the OpenAPI doc.

I assume that when you select Basic auth (if that is an option in the OAI doc), then the UI presents the user with Username and Password input fields?

All the above is IMO. :)

zregvart commented 5 years ago

Thanks @EricWittmann. Yeah different authentication schemes conflict a bit with the notion of a single connection, or at least make it more difficult to implement and create a good user experience at the same time, so we simplified it as much as we could to both make it easier to implement and to have a more streamlined user experience. I'll create a separate issue for discussion around extending that support.

zregvart commented 5 years ago

I've created https://github.com/syndesisio/syndesis/issues/5595 feel free to comment there.

kahboom commented 5 years ago

@dongniwang - Basically, OAuth is one type of security scheme (others are e.g. Basic Authentication, see the link Zoran posted), and there are different flows for it (e.g. implicit, accessCode). As Zoran said, we only support accessCode. It is also the only flow that has both the authorizationUrl and tokenUrl, the others only use one or the other. This is easy to visualize in Apicurio studio where you can choose the type of OAuth flow from a dropdown, and the respective fields show/hide. The UX is very good--see this API: https://studio.apicur.io/apis/11077/collaboration/accept/25c121ab-e5f8-4ef4-b5d9-28a2eb1cfea6

So, basically, when the user uploads a spec that has an OAuth scheme other than accessCode, we pre-fill only one of the fields. Also, it really isn't clear that we are only supporting accessCode, since it just shows up in the UI as an option called OAuth 2.0.

As @EricWittmann did, I'm ignoring the per-operation and security requirements for right now and focusing on supporting multiple OAuth schemes and how we handle anything other than the accessCode OAuth flow.

@dongniwang 's design brings to mind yet another thing, we don't currently display the scopes as is shown in the design (in the Angular app either). Is that still something we'd want to show? At the moment, I see we do get this from the BE, though I'm not sure if we'd be able to group it for each OAuth 2.0 option offered as I suggested here.

Following up with my other thoughts in the issue Zoran created. :)

EricWittmann commented 5 years ago

@kahboom Yeah I was going to mention the missing scopes. It's not clear to me whether the scopes should be displayed in the UI. The scopes configured on an OpenAPI Security Scheme (which is what is being displayed currently in the UI) are simply a list of possible scopes that the authenticated user might have. When defining the Security Requirements for an operation is when the API designer indicates which of those possible scopes the authenticated user must have when accessing the operation. But when configuring this connector, it doesn't seem like any of that information should be presented as checkboxes, since the scopes that the authenticated user has will come from the identity provider (the auth server) during authentication, not from syndesis. So maybe there is a reason to display the scopes for informational purposes ... maybe ... but not in any interactive way.

IMO - I obviously don't know exactly what this connector does and how it does it. :)

dongniwang commented 5 years ago

I think if the scopes are defined in API Designer (Apicurito), then we should keep the editing part in the API Designer.

My understanding is that we should prefill information we can read from the OpenAPI definition, and maybe support quick decision making - that's in terms of how users want to set up and use the connector in Syndesis. And keep the actual configuration and editing tasks in the API designer. The design I included was from a long time ago (way before we introduced API Designer).

Does this make sense?

zregvart commented 5 years ago

From @EricWittmann:

I assume that when you select Basic auth (if that is an option in the OAI doc), then the UI presents the user with Username and Password input fields?

In this step the user is creating a connector, to use it within an integration the users need to create a connection or connections (think, test, production environments). Part of creating a connection is entering the username/password, API key or connecting via OAuth.

Except that the two fields should be something like Authorization URL and Token URL.

Yeah we name them "Authorization URL" and "Access Token URL" I think we researched this and took the naming from the OAuth RFC.

On OAuth scopes: we simplified this and we're assuming that the user is interested in all scopes, the backend simply uses any scope mentioned in the OpenAPI document. If I remember correctly we prototyped/designed for scopes to be presented to the users and that the users can then select the scopes they're interested in, and that the conclusion there was that it was too complicated to comprehend for them what the consequences of this selection would be. They would be making some of the operations (connector actions) inaccessible by removing a scope required for that operation.

I think our guiding thought was lets get the wizard and the implementation as simple as possible and cover 80% of the cases. And I agree with @dongniwang on editing the scopes in Apicurio, and in principle the functionality that's already in Apicurio we shouldn't duplicate in Syndesis.

zregvart commented 5 years ago

There's a pull request to filter out only supported (access code) OAuth flow and an issue to further refine authentication in API clients, with that I think the conversation can be moved there.

Can this be closed now?