ILIAS
ILIAS copied to clipboard
ILIAS-eLearning
Fix and Improve OpenID Connect integration
The current OIDC integration has three major flaws:
- It uses the Implicit Flow instead of the Code Flow. The Implicit Flow should be considered deprecated (and will be in OAuth 2.1) and provides no relevant advantages over using the Code Flow, especially when the application is a confidential client (which ILIAS is, since the code is run on the server and not on the client). Since I couldn't find any comment why Implicit Flow is used, I think it was introduced by copying example code from the library's documentation.
- The Logout is totally broken and likely never worked. There is no need to pass any Access Token to the Endsession Endpoint. Instead, you just need to provide the ID Token. This may be caused by incorrect parameter naming in the used library.
- The used library is poorly maintained and has multiple flaws. Therefore, I forked the library and included it in this PR.
All points above are fixed by this PR.
The first and the last change allow to use PKCE if the OpenID Provider supports it.
I am not sure if this is only an improvement. It is also fixes bugs in the logout.
Hi @smeyer-ilias
this came up again in our last TB meeting on the 19-10-2021. Is there anything new regarding this PR? Is there anything we could do to speed up the process?
Best regards!
Hi @smeyer-ilias As Technical Board, we regularly check for pull requests that have been open for a long time. As the assignee, if you don't have any objections, please merge this PR or provide feedback otherwise.
Best regards!
@pascalseeland This PR solves the global logout issue - however it has merge conflicts now!
Hi @smeyer-ilias,
we stumbled upon this PR in our TB meeting. Any updates on this?
Best regards!
Hi @pascalseeland
As Technical Board, we regularly check for pull requests that have been open for a long time. Any Updates on this? Are you looking into this or is @smeyer-ilias on it? Note, that you can also close this, if you are not able or if you not have the ressources to look into it in detail.
Best regards!
Hi @pascalseeland
As Technical Board, we regularly check for pull requests that have been open for a long time. Any Updates on this? Are you looking into this or is @smeyer-ilias on it? Note, that you can also close this, if you are not able or if you not have the ressources to look into it in detail.
Best regards!
Hi @pascalseeland
As Technical Board, we regularly check for pull requests that have been open for a long time. Any Updates on this? Are you looking into this or is @smeyer-ilias on it? Note, that you can also close this, if you are not able or if you not have the ressources to look into it in detail.
Best regards!
Hi @pascalseeland,
we stumbled upon this PR in our TB meeting. Any updates on this?
Best regards!
Hi @pascalseeland
As Technical Board, we regularly check for pull requests that have been open for a long time. Any Updates on this? Note, that you can also close this, if you are not able or if you not have the ressources to look into it in detail.
Best regards!
Sorry for not getting to this in a more timely manner. I don't think using a forked library is a way to go forward. From what I can tell, several of the issues you mentioned have be fixed my your PRs in the meanwhile. I would prefer to only merge the part that fixes to logout. We would also need a mantis issue to track this change. If you wish I would create one.
