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

Acr value support #388

Merged
merged 6 commits into from
Mar 9, 2022
Merged

Acr value support #388

merged 6 commits into from
Mar 9, 2022

Conversation

Glowsome
Copy link
Contributor

@Glowsome Glowsome commented Mar 8, 2022

All Submissions:

Changes proposed in this Pull Request:

Fixes #331
This PR enhances the plugin with capabillities on handling acr_values - in which the request to the IDP includes a specific authentication contract as defined on the IDP.
Next to that enhancements have been made to also check - when set that the - if defined authentication contract is honored.

How to test the changes in this Pull Request:

  1. Define the added acr_values field option with a contract that exists on the IDP ie. urn:domain:contract:my_method_of_auth ( this is the exact name of the authentication uri as defined on a/the IDP.
  2. Test login, and see that the added (acr_-)values to the url will influence the behaviour of the authentication contract with the IDP.
  3. Verify manual url-manipulation check so it will error / disallow login ( remove the added acs_values from url manually and try it on your IDP)

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.

  • Added acr_values support & verification checks that it when defined in options is honored.

acr-values support
Fixed reported code-standard issues found
@Glowsome
Copy link
Contributor Author

Glowsome commented Mar 8, 2022

@timnolte would it be possible to trigger the code standard workflow also via workflow_dispatch: ?
If i understand the workings this is stopping me to run the review prior to opening/adding a PR

  • Glowsome

@timnolte
Copy link
Collaborator

timnolte commented Mar 8, 2022

@Glowsome actually I think GitHub changed how GitHub Actions work. I know that one of the big issues recently was the fact that someone could change the GitHub Actions in a PR and could compromise the integrity of the code. Right now I have to approve all of the GitHub Actions to run. However, as I mentioned in the previous PR I have setup the project so that you can run all of these checks locally, and actually if you have your local development environment setup correctly it should be not allowing you to even commit your changes locally because there are checks in place. When you setup the project locally are you running npm run setup? If you run that the entire project should be setup the right way for local development. I obviously don't have any way to enforce that people do this but I've done a ton of work to make sure people have all of the tools they need to contribute. I'll try taking a look at what has changed with the GitHub Actions permissions, which I'm pretty sure would still prevent you from running a workflow on-demand as well.

@Glowsome
Copy link
Contributor Author

Glowsome commented Mar 8, 2022

@timnolte you are thinking too high of me, i just fiddle around in code, and when needed use github desktop (on a windows box)
so in interacting with you i am sort-of self-studying in a crash-course manner to get everything in order so it can be incorporated in the upcoming release.
To me all the workflows/unittests etc is all like a complete new world for me ... (i'm just an old guy, so i got stuck somewhere in the past regarding this)

  • Glowsome

@timnolte
Copy link
Collaborator

timnolte commented Mar 8, 2022

Ah, sure thing. No worries we'll work through it. Thanks for all your patience!

fixing code-indentment after report
fix for reported code-standard errors
Fixing code-standard reported errors
@Glowsome
Copy link
Contributor Author

Glowsome commented Mar 8, 2022

@timnolte i also thank you for your patience with me ... i seem to be claiming alot of your time here.
Maybe somewhere in the future there is time for something like a session to guide me more then just having a convo over a/the PR :)
... can you re-trigger the code-standard workflow ?

  • Glowsome

re-aligned line 225 ( indentment error)
@Glowsome
Copy link
Contributor Author

Glowsome commented Mar 9, 2022

@timnolte awesomesauce .... seems all checks have now completed without issues so i'd guess you should be good for incorporating it into a/the new release ?

@timnolte timnolte self-assigned this Mar 9, 2022
@timnolte timnolte added enhancement Issues & PRs related to new features. status: needs review PR that needs review. labels Mar 9, 2022
@timnolte timnolte added this to the 3.9.0 milestone Mar 9, 2022
Copy link
Collaborator

@timnolte timnolte left a comment

Choose a reason for hiding this comment

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

@Glowsome I made a couple of notes. I'll take care of the tweaks after I merge. Thanks for all the work!

@timnolte timnolte merged commit c42bf78 into oidc-wp:dev Mar 9, 2022
@timnolte timnolte added status: approved PRs that have been approved and ready to be merged. and removed status: needs review PR that needs review. labels Mar 9, 2022
@timnolte
Copy link
Collaborator

timnolte commented Mar 9, 2022

@Glowsome just a heads up if you are interested to check the adjustments I made to your code you can check them out in this PR: #389

I did actually find a mistake that I missed in the initial code review so it was a good thing that I took time to refactor some of this code. Thanks for your contribution!

@Glowsome
Copy link
Contributor Author

@Glowsome just a heads up if you are interested to check the adjustments I made to your code you can check them out in this PR: #389

I did actually find a mistake that I missed in the initial code review so it was a good thing that I took time to refactor some of this code. Thanks for your contribution!

I have taken a quick look at it, and i like it - and i enjoyed working with you in getting this enhancement into play, as to me it really is a value-add to the plugin in regards of functionality.
But on the other side it makes me feel old having to consume so much time from you to get myself up to par and my aged skills.

  • Glowsome

@Glowsome Glowsome deleted the acr-value-support branch June 3, 2023 23:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Issues & PRs related to new features. status: approved PRs that have been approved and ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for sending acr_values
2 participants