Skip to content

Updated logout step to work in karaf mode#36

Merged
asfgit merged 1 commit intoapache:masterfrom
bostko:fix_swagger_menu
Mar 6, 2017
Merged

Updated logout step to work in karaf mode#36
asfgit merged 1 commit intoapache:masterfrom
bostko:fix_swagger_menu

Conversation

@bostko
Copy link
Copy Markdown
Contributor

@bostko bostko commented Nov 9, 2016 •
edited
Loading

  • BROOKLYN-323: Simplify LogoutApi
  • relative urls for swagger html

PR depends on apache/brooklyn-server#578

Previously when you are on the API page dropdown menu was not visible.
screenshot from 2016-11-09 13-17-35

@bostko bostko force-pushed the fix_swagger_menu branch 3 times, most recently from df1b3a5 to bac945e Compare November 10, 2016 00:44
@tbouron
Copy link
Copy Markdown
Member

tbouron commented Nov 10, 2016

Tested, LGTM

@neykov
Copy link
Copy Markdown
Member

neykov commented Nov 10, 2016 •
edited
Loading

I think that's the wrong way to fix the problem. The swagger page is opened with target=_blank so it's a separate one from the main application. This makes it surprising - say you click on the rest api page which opens a new tab (you could even miss that), then click back on applications. Now you have to identical tabs.
We should either:

  • remove the frame from the swagger page so it's obvious that a new tab is opened. With no way to click on the application tab (which brings even more confusion)
  • Open the swagger ui in the same window

@bostko bostko force-pushed the fix_swagger_menu branch 2 times, most recently from be1b067 to 7b42f28 Compare November 10, 2016 12:26
@bostko
Copy link
Copy Markdown
Contributor Author

bostko commented Nov 10, 2016

@neykov I changed it to open in the same tab.
Can you check again whether it looks consistent UX.

@neykov
Copy link
Copy Markdown
Member

neykov commented Nov 10, 2016

LGTM. This has been changed back and forth in the past, so @ahgittin, @m4rkmckenna any comments?

@bostko bostko force-pushed the fix_swagger_menu branch 3 times, most recently from c673054 to 07d782d Compare November 24, 2016 13:02
ahgittin
ahgittin reviewed Nov 24, 2016
View reviewed changes
Copy link
Copy Markdown
Contributor

@ahgittin ahgittin left a comment

Choose a reason for hiding this comment

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

a few comments i think need addressing, then good to move

the swagger page is a bit of a bastard so we can forgive if everything isn't perfect but i think you are pretty close @bostko

Comment thread src/main/webapp/assets/js/config-swagger.js Outdated
@@ -0,0 +1,12 @@
require.config({
Copy link
Copy Markdown
Contributor

@ahgittin ahgittin Nov 24, 2016

Choose a reason for hiding this comment

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

where is this file used?

does it need the RAT header?

Copy link
Copy Markdown
Member

@neykov neykov Nov 24, 2016

Choose a reason for hiding this comment

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

Somehow related, #38 activates the rat plugin for the project. So good point about adding license here.

Comment thread src/main/webapp/assets/html/swagger-ui.html Outdated
@@ -81,15 +83,15 @@
<a class="logo" href="#" title="Brooklyn, Version 0.10.0-SNAPSHOT"><!-- Logo added via CSS --></a> <!-- BROOKLYN_VERSION -->
Copy link
Copy Markdown
Contributor

@ahgittin ahgittin Nov 24, 2016

Choose a reason for hiding this comment

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

the line 2 above, don't we need to change href="/logout" to href="../../v1/logout" ?

@ahgittin ahgittin mentioned this pull request Nov 24, 2016
Merged
@sjcorbett
Copy link
Copy Markdown
Contributor

sjcorbett commented Jan 23, 2017

@bostko could you revisit Alex and Svet's comments?

@bostko bostko force-pushed the fix_swagger_menu branch 4 times, most recently from 0dc27be to 61333cc Compare March 2, 2017 08:48
@bostko bostko changed the title Fix nav menu in Brooklyn API page Authorization header, go to serverside html Mar 2, 2017
@bostko bostko mentioned this pull request Mar 2, 2017
Merged
@bostko bostko changed the title Authorization header, go to serverside html Authorization header, going to serverside html Mar 2, 2017
@bostko bostko changed the title Authorization header, going to serverside html Updated logout step to work in karaf mode Mar 2, 2017
@bostko bostko force-pushed the fix_swagger_menu branch from 61333cc to a82f066 Compare March 2, 2017 17:36
@sjcorbett
Copy link
Copy Markdown
Contributor

sjcorbett commented Mar 3, 2017

@bostko looks good. Will test and merge if ok.

@sjcorbett
Copy link
Copy Markdown
Contributor

sjcorbett commented Mar 3, 2017

@bostko Looks like this negatively impacts logging out of the non-Karaf distribution. After logging out I've found that:

  • master non-karaf: prompts for re-authentication.
  • master karaf: 405 error POST is not supported by this URL
  • this pr karaf: displays page with "login" in big text. Clicking it prompts for re-authentication.
  • this pr non-karaf: displays page with "login" in big text. Clicking it logs the user back in without requiring re-authentication.

We should prompt the user to re-authenticate in the standard distribution.

@bostko bostko force-pushed the fix_swagger_menu branch from a82f066 to b9d4618 Compare March 3, 2017 17:16
@bostko
Updated logout step to work in karaf mode
f79b98a 
- BROOKLYN-323: Simplify LogoutApi
- relative urls for swagger html
@bostko bostko force-pushed the fix_swagger_menu branch from b9d4618 to f79b98a Compare March 3, 2017 17:16
@bostko
Copy link
Copy Markdown
Contributor Author

bostko commented Mar 3, 2017

@sjcorbett thanks for the notes!
I added in apache/brooklyn-server#578 an unauthorize call which seems to solve the issues in classic mode.
Could you check both PRs again,
Thanks!

@sjcorbett
Copy link
Copy Markdown
Contributor

sjcorbett commented Mar 6, 2017

@bostko thanks for the update. I've confirmed that the browser requires re-authentication in both cases.

@asfgit asfgit merged commit f79b98a into apache:master Mar 6, 2017
asfgit pushed a commit that referenced this pull request Mar 6, 2017
This closes #36
d9b82df 
Updated logout step to work in karaf mode
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ahgittin ahgittin ahgittin left review comments

+1 more reviewer

@neykov neykov neykov left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@bostko @tbouron @neykov @sjcorbett @ahgittin @asfgit