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

WIP: [ZEPPELIN-2881] Fix OIDC logout #2552

Closed
wants to merge 3 commits into from
Closed

Conversation

andreaTP
Copy link
Contributor

What is this PR for?

Fixing logout mechanism for OIDC.
Initial documentation for configuring OIDC in Zeppelin.

What type of PR is it?

[Bug Fix & Improvement]

Todos

What is the Jira issue?

[ZEPPELIN-2881]

How should this be tested?

You can test by using the guide provided in documentation. Please note that you will need to locally deploy pac4j until a stable release will be published officially.

// force authcBasic (if configured) to logout
$http.post(logoutURL).error(function () {

let config = (process.env.PROD) ? {headers: { 'X-Requested-With': 'XMLHttpRequest' }} : {}
Copy link
Member

Choose a reason for hiding this comment

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

what is process.env.PROD?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kept this since this: #2463

let config = (process.env.PROD) ? {headers: { 'X-Requested-With': 'XMLHttpRequest' }} : {}
$http.post(logoutURL, config).then(
function (response) {
alert('ok response status ', response.status)
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure we should have a blocking dialog box here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can remove it of course, this was simply the original behavior... just tell me what to do :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

argh sorry, haven't noticed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed, you were correct, thanks!

@felixcheung
Copy link
Member

felixcheung commented Aug 28, 2017 via email

@andreaTP
Copy link
Contributor Author

yes, the original code I think is the result of a few iterations.
This is the blocking pop-up:


But you can also notice that a double post is performed without meaningful comments to justify why...

I think that someone like @1ambda or @Leemoonsoo maybe can clarify and add it's opinion to decide if keep the pop-up or not (personally I do not care ...)

@andreaTP
Copy link
Contributor Author

sorry @felixcheung misunderstood your comment, should be fixed now!

@felixcheung
Copy link
Member

hi - sorry about the delay, it looks like Jenkins can't locate the test results - could you close/reopen this PR to kick off another test run?

@felixcheung
Copy link
Member

also please update the title to remove WIP if you are ready

@andreaTP
Copy link
Contributor Author

Hi thanks for getting back into this, unfortunately even if pac4j/pac4j#975 got merged we do not have yet a release of pac4j out in public repositories that will enable this fix.
if you agree I wait to have first a release of pac4j and then I will update this PR accordingly.
WDYT?

@felixcheung
Copy link
Member

felixcheung commented Sep 15, 2017 via email

@saba0815
Copy link

Is waiting now over?

@andreaTP
Copy link
Contributor Author

waiting is over but pac4j introduced a few regressions that needs to be fixed to have the documented process working...
I'm looking into, slowly, but if you wanna take over feel free to go ahead and contact me privately if you need any help. @saba0815

@tiboun
Copy link

tiboun commented Nov 29, 2018

Hi, I've made an integration of the oidc logout and have included a new profile (oidc) in order to build zeppelin with oidc related libs. I've started my work on branch-0.8 because master doesn't build with scala 2.11 at the moment. Would you like me to push my PR in this issue or do I have to create a new issue and ask if I can add a new profile for oidc ? I think it would be better to create a new one.

@saba0815
Copy link

@andreaTP with the configuration above what should be printed as username after login? I only get sth like an id.

@saba0815
Copy link

@andreaTP with the configuration above what should be printed as username after login? I only get sth like an id.

Got it. One can set

    pac4jRealm.principalNameAttribute = email

in shiro.ini and it sets the email as displayed login name.

@jolo-dev
Copy link

Hello,
Will this ever go through?

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.

5 participants