Skip to content

Conversation

@dmitrizagidulin
Copy link
Contributor Author

dmitrizagidulin commented Jul 4, 2018

This is a first pass at implementing a better Logout functionality on the server side.
What this PR covers:

  • Calls the host server's logout() (to clear any sessions or whatever)
  • Validates post_logout_redirect_uri per the spec, and 302 redirects to it if present (along with the state)
  • Requires id_token_hint to be present if post_logout_redirect_uri is given (since the token hint is the only way to validate the pre-registered redirect uri).
  • Responds with a 204 No Content if no redirect uri is given

Things to do in the next iteration (and things to discuss):

  • Doesn't prompt the user for consent to also log out of the home pod
  • If no redirect uri is given by the RP client, should we redirect to a pod's default /goodbye uri or something? (Instead of just a 204 No Content)
  • Possibly consider as a special case post-logout redirect uris if they're on the user's home pod?

@dmitrizagidulin
Copy link
Contributor Author

Ok, changed it so that if no post_logout_redirect_uri is provided by the RP, it looks in the host/OP's defaults (to redirect to /goodbye, for example).

PR is mostly ready for review, needs a few more tests though.

it('should redirect to RP if logout uri provided', () => {
let res = HttpMocks.createResponse()
let req = HttpMocks.createRequest({
method: 'GET',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tests need to be changed to POST

@cblakeley
Copy link
Contributor

@dmitrizagidulin : Dmitri,please can you provide an ETA for when #7 will be completed and released?

Of the original pull requests raised by OpenLink 2-3 months ago relating to logout issues, several have been closed with unmerged commits. In these cases, the Solid team has chosen to open new issues and/or pull requests incorporating your own solutions to the base issues. I gather #7 includes most or all of this work. We at OpenLink urgently need this PR closed so that we can sync our forks of the Solid repos and avoid the OpenLink forks diverging any further from the Solid masters. If you could see to this we'd be grateful.

/cc @kidehen @RubenVerborgh @smalinin

@dmitrizagidulin
Copy link
Contributor Author

dmitrizagidulin commented Aug 14, 2018

hi @cblakeley,
i can’t speak officially for the solid team as a whole; on my end, it should be ready for the solid-server 5.0 release.

@kjetilk
Copy link
Member

kjetilk commented Aug 23, 2018

Hi! We're having the meeting with @kidehen and we found that this PR should fix the logout problem without merging nodeSolidServer/solid-auth-client#46 (because that PR fixes the problem in the wrong place).

The problem has been that we haven't felt quite up to the task of reviewing this, so we figured that OpenLink would be better qualified to review it. So, @cblakeley could you please test and review this PR and report back? Make sure that you do not have nodeSolidServer/solid-auth-client#46 running when you test this, since that would make it appear to log out, but it would be the wrong way.

@RubenVerborgh RubenVerborgh added the enhancement New feature or request label Aug 24, 2018
@kjetilk
Copy link
Member

kjetilk commented Aug 30, 2018

I see some conflicts here, but they seem to be mostly about the timeout. Should the timeout be in there, or not?

@dmitrizagidulin
Copy link
Contributor Author

@kjetilk Thanks for the heads up. I've resolved the conflicts, should be fine now.

(Part of the confusion about the timeouts has to do with -- so this PR moves all the individual timeouts into a single Mocha-wide timeout in mocha.opts.)

Copy link

@kidehen kidehen left a comment

Choose a reason for hiding this comment

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

@justinwb justinwb merged commit 9e7dd59 into master Sep 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants