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

the key for clientAuthorizations.add must be the same as the one of the SecurityDefinition #134

Merged
merged 1 commit into from
Oct 25, 2017

Conversation

fretje
Copy link
Contributor

@fretje fretje commented Oct 24, 2017

see securitydefinition defined in startup.cs

options.AddSecurityDefinition("bearerAuth", new ApiKeyScheme()

see also swagger-api/swagger-ui#1244 (comment)

…he securitydefinition (in the startup class)
@acjh
Copy link
Contributor

acjh commented Oct 24, 2017

No, this won't work. The code currently uses ApiKeyAuthorization to mimic JWT authentication, so the header must be abp.auth.tokenHeaderName (please find out what that is before replacing it with a magic string). If you want to "fix" this, find out if bearer authentication is supported and configure that.

@fretje
Copy link
Contributor Author

fretje commented Oct 24, 2017

Well, the header is actually still abp.auth.tokenHeaderName (see the line before my edit).

What I changed is the key for the add operation of clientAuthorizations, which must be identical to the key set in the securitydefinition for the swagger document. That key is now in startup.cs also a "magic string".

To test this, I added a simple call to abp.swagger.login() at the end of on-complete.js. Then some dialogs appear which ask for tenant, username and password, and then you don't have to manually log in.

I can only say that this automatic login procedure wasn't working for me until I made the change in this pull request.

@acjh
Copy link
Contributor

acjh commented Oct 24, 2017

Which "line before my edit"?

abp.swagger.login() is a helper with an independent mechanism: aspnetboilerplate/aspnetboilerplate#2454 (comment)

@@ -11,7 +11,7 @@ var abp = abp || {};
return false;
}
var cookieAuth = new SwaggerClient.ApiKeyAuthorization(abp.auth.tokenHeaderName, 'Bearer ' + authToken, 'header');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@acjh I'm talking about this line. Here the headername gets set.

@@ -11,7 +11,7 @@ var abp = abp || {};
return false;
}
var cookieAuth = new SwaggerClient.ApiKeyAuthorization(abp.auth.tokenHeaderName, 'Bearer ' + authToken, 'header');
swaggerUi.api.clientAuthorizations.add(abp.auth.tokenHeaderName, cookieAuth);
swaggerUi.api.clientAuthorizations.add('bearerAuth', cookieAuth);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

And here the header gets added to the clientAuthorizations of the swaggerui api. The key here is not necessarily the same as the headername. See swagger-api/swagger-ui#1244 (comment) for a more in depth explanation.

@acjh
Copy link
Contributor

acjh commented Oct 24, 2017

@fretje
Copy link
Contributor Author

fretje commented Oct 24, 2017

The name of the header, yes indeed.

But the name of what you add to swaggerUi.api.clientAuthorizations should match

options.AddSecurityDefinition("bearerAuth", new ApiKeyScheme()

@acjh
Copy link
Contributor

acjh commented Oct 24, 2017

Did you try abp.swagger.login() without the change?

@fretje
Copy link
Contributor Author

fretje commented Oct 24, 2017

Yes, I did.
On the master branch of this repo. When I run abp.swagger.login(), I get this in the console:
image

But when I try a request after that with swagger ui, I still get 401 (Unauthorized) as the swagger ui doesn't send the "Authorization" header:
image
When I make the change in this pull request, all goes well.

@acjh
Copy link
Contributor

acjh commented Oct 24, 2017

I see. Thank you for the clarification.

@hikalkan
Copy link
Member

So, should I merge this :)

@acjh
Copy link
Contributor

acjh commented Oct 25, 2017

Yes 👍

@alirizaadiyahsi alirizaadiyahsi merged commit e0c0664 into aspnetboilerplate:master Oct 25, 2017
@acjh
Copy link
Contributor

acjh commented Oct 25, 2017

Explanation: The original PRs were tested separately and the helper works on its own, but adding the security definition requires that the client authorization added by the helper to have the same name.

@fretje fretje deleted the bearerAuth branch October 25, 2017 16:43
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.

4 participants