Skip to content

Add require_prompt as an option #492

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

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Add require_prompt as an option #492

wants to merge 2 commits into from

Conversation

jirian
Copy link

@jirian jirian commented Jul 11, 2023

All Submissions:

Changes proposed in this Pull Request:

This PR adds a "require_prompt" option in the settings. If enabled, it adds the prompt=login parameter to the authentication URL forcing the IdP to show the login form every time, regardless of whether the user checked "Remember Me".

How to test the changes in this Pull Request:

  1. Set Require Prompt to true in settings.
  2. Check whether &prompt=login gets appended to the authentication URL.

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully run tests with your changes locally?

Changelog entry

Enter a summary of all changes on this Pull Request. This will appear in the changelog if accepted.

Adds a "require_prompt" option in the settings. If enabled, it adds the prompt=login parameter to the authentication URL forcing the IdP to show the login form every time, regardless of whether the user checked "Remember Me".

@timnolte
Copy link
Collaborator

@jirian have you considered just using the plugin hooks and custom settings features to add this yourself via a Must Use Plugin? I'm not saying that we won't add this feature, however the plugin is very customizable already to add such a feature without needing to modify the plugin code.

@jirian
Copy link
Author

jirian commented Jul 11, 2023

@timnolte I have now :), but I still feel that since this is a direct part of the OpenID Connect spec, it should be a part of the plugin itself.

@timnolte
Copy link
Collaborator

@jirian excellent! I hadn't taken a look at the spec yet to determine whether this was actually part of the spec or possibly something to a specific IDP. When things seem to be specific to an IDP I'm hesitant to incorporate it directly into the plugin.

It looks like there are failing checks can you double check and run PHPCS to ensure that your code changes meet standards? I think there is something odd going on with PHPStan, I'll check into that.

@jirian
Copy link
Author

jirian commented Jul 11, 2023

@timnolte I understand that. Just in case, I wrote a must-use plugin too (https://github.com/jirian/oidc-require-prompt-addon), but this really is a direct part of the spec, not a IdP-specific thing.

I linted the code using phpcs, it should pass the checks now.

@timnolte timnolte self-assigned this Jul 11, 2023
@timnolte timnolte added the status: needs review PR that needs review. label Jul 11, 2023
@timnolte
Copy link
Collaborator

@jirian not sure what's going on with the static analysis. GitHub Actions & local runs on the current dev branch works just fine, but your PR is failing.

@jirian
Copy link
Author

jirian commented Jul 12, 2023

@timnolte I'm not sure either, it's reporting the errors on lines that I didn't change at all. I have no clue what is going on.

@timnolte timnolte deleted the branch oidc-wp:develop December 23, 2023 00:56
@timnolte timnolte closed this Dec 23, 2023
@timnolte timnolte reopened this Dec 23, 2023
@timnolte timnolte changed the base branch from dev to develop December 23, 2023 01:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: needs review PR that needs review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants