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

We shall only call the sso end session endpoint in case we still have… #2961

Merged
merged 1 commit into from Mar 26, 2020

Conversation

DeepDiver1975
Copy link
Member

… the id_token. In some sceanrios the id_token is no longer known (expiry)

Description

Related Issue

  • Fixes <issue_link>

Motivation and Context

How Has This Been Tested?

  • test environment:
  • test case 1:
  • test case 2:
  • ...

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

Open tasks:

  • ...

@update-docs
Copy link

update-docs bot commented Feb 4, 2020

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@DeepDiver1975 DeepDiver1975 self-assigned this Feb 4, 2020
@DeepDiver1975 DeepDiver1975 marked this pull request as ready for review March 10, 2020 14:34
@DeepDiver1975
Copy link
Member Author

I guess the test scenarios need to be adjusted ....

@LukasHirt
Copy link
Contributor

The problem with tests is that logout leads us to "Access denied" page. Not sure if this is happening also in a setup with oidc provider or only with oauth2.

@LukasHirt
Copy link
Contributor

LukasHirt commented Mar 24, 2020

The access denied page is appearing because of this line https://github.com/owncloud/phoenix/blob/f4eeac80a042b1d6ea4f72f004627e7631bc9476/src/store/user.js#L110 Would it be fine using here "Login" page instead or removing completely @DeepDiver1975 ? Can the user be unloaded because of some error? Or is it only when doing a logout? I guess also expiration? But even then I'd still expect to see login screen instead as a user.

@kulmann
Copy link
Member

kulmann commented Mar 25, 2020

The access denied page is appearing because of this line

https://github.com/owncloud/phoenix/blob/f4eeac80a042b1d6ea4f72f004627e7631bc9476/src/store/user.js#L110

Would it be fine using here "Login" page instead or removing completely @DeepDiver1975 ? Can the user be unloaded because of some error? Or is it only when doing a logout? I guess also expiration? But even then I'd still expect to see login screen instead as a user.

As a bonus you would come back to the route where you were before the logout (also in case of expiration), right? As of now you come back to the access denied route when you re-login after expiration.

@DeepDiver1975
Copy link
Member Author

Would it be fine using here "Login" page instead or removing completely @DeepDiver1975 ? Can the user be unloaded because of some error? Or is it only when doing a logout? I guess also expiration? But even then I'd still expect to see login screen instead as a user.

in the very first implementation we always redirected to the login screen - this was changed upon customer request.

I find it rather complicated to differentiate when to display which page .... 🙈

Copy link
Contributor

@LukasHirt LukasHirt left a comment

Choose a reason for hiding this comment

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

Changelog pls

@DeepDiver1975
Copy link
Member Author

Changelog pls

added

Copy link
Contributor

@LukasHirt LukasHirt left a comment

Choose a reason for hiding this comment

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

Seems the ocis tests are failing again but I don't think it's related to this PR since they passed before the changelog commit.

@LukasHirt
Copy link
Contributor

Hmm, looking at https://drone.owncloud.com/owncloud/phoenix/9041 the ocis tests seems to perform better than here. Maybe it is after all related somehow to this PR?

@DeepDiver1975
Copy link
Member Author

Maybe it is after all related somehow to this PR?

seems to be - who has enough understanding on having a look? I lost track with ocis these days 🙈

@LukasHirt
Copy link
Contributor

who has enough understanding on having a look?

@dpakach Would you have time to take a look at the ocis tests here?

@dpakach
Copy link
Contributor

dpakach commented Mar 26, 2020

@dpakach Would you have time to take a look at the ocis tests here?

@LukasHirt I will try to run them locally and see what's going on

@dpakach
Copy link
Contributor

dpakach commented Mar 26, 2020

All tests are failing on login so seems like something has changed in login flow

… the id_token. In some sceanrios the id_token is no longer known (expiry)
@dpakach dpakach force-pushed the bugfix/logout-without-id-token branch from e5ea55c to 90313f2 Compare March 26, 2020 10:52
@dpakach
Copy link
Contributor

dpakach commented Mar 26, 2020

Sorry, l am unable to reproduce this locally. I just rebased just to see what happens.

@LukasHirt
Copy link
Contributor

LukasHirt commented Mar 26, 2020

All tests are failing on login so seems like something has changed in login flow

The changes are actually in the opposite - logout.

Sorry, l am unable to reproduce this locally. I just rebased just to see what happens.

It's green now, I'll try to run them one more time and if they'll still be green I'll merge it . Just to be sure 😉 Thanks a lot for taking a look!

@LukasHirt LukasHirt merged commit cd2bb9e into master Mar 26, 2020
@LukasHirt LukasHirt deleted the bugfix/logout-without-id-token branch March 26, 2020 14:38
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.

None yet

4 participants