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

Start with version 5 #211

Merged
merged 33 commits into from
Dec 3, 2023
Merged

Start with version 5 #211

merged 33 commits into from
Dec 3, 2023

Conversation

cicnavi
Copy link
Collaborator

@cicnavi cicnavi commented Dec 2, 2023

Since SimpleSAMLphp v2.1 is out for some time, start with v5 of the module which is compatible with SSP v2.1, so users can start using it.

Copy link

codecov bot commented Dec 2, 2023

Codecov Report

Attention: 341 lines in your changes are missing coverage. Please review.

Comparison is base (895abfd) 52.65% compared to head (b973d1c) 53.26%.

Files Patch % Lines
src/Services/DatabaseMigration.php 0.00% 43 Missing ⚠️
src/Server/Grants/AuthCodeGrant.php 0.00% 41 Missing ⚠️
src/Forms/ClientForm.php 0.00% 34 Missing ⚠️
src/Services/Container.php 0.00% 33 Missing ⚠️
src/ModuleConfig.php 51.16% 21 Missing ⚠️
src/Services/LoggerService.php 0.00% 19 Missing ⚠️
src/Services/SessionService.php 0.00% 16 Missing ⚠️
src/Server/Grants/RefreshTokenGrant.php 0.00% 12 Missing ⚠️
src/Services/IdTokenBuilder.php 0.00% 11 Missing ⚠️
src/Server/Grants/ImplicitGrant.php 0.00% 10 Missing ⚠️
... and 37 more
Additional details and impacted files
@@             Coverage Diff              @@
##             master     #211      +/-   ##
============================================
+ Coverage     52.65%   53.26%   +0.60%     
- Complexity      878      984     +106     
============================================
  Files           108      109       +1     
  Lines          3667     3809     +142     
============================================
+ Hits           1931     2029      +98     
- Misses         1736     1780      +44     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cicnavi
Copy link
Collaborator Author

cicnavi commented Dec 2, 2023

The following will be postponed for version 6:

New features

  • TODO move away from SSP database as store; move to custom store interface
  • TODO key rollover
  • TODO token introspection
  • TODO implement store for different entities?: i.e. client data can use RDB like mysql, whilst short term data
    like tokens can utilize faster stores like memcache, redis...
  • TODO move to SimpleSAMLphp ProcessingChain

Major impact changes

  • TODO (currently dev-simplesamlphp-2.1) SimpleSAMLphp version requirement fixed to v2.1.*
  • TODO move away from SSP database as store; move to custom store interface

Medium impact changes

  • TODO move to SSP (symfony) routing
    • TODO handle CORS

Low impact changes

Below are some internal changes that should not have impact for the OIDC OP implementors. However, if you are using
this module as a library or extending from it, you will probably encounter breaking changes, since a lot of code
has been refactored:

  • TODO upgrade to v5 of lcobucci/jwt https://github.com/lcobucci/jwt
  • TODO move checkers to templates (generics) for proper static type handling
  • TODO move to SSP (symfony) container
  • TODO remove dependency on laminas/laminas-diactoros
  • TODO remove dependency on laminas/laminas-httphandlerrunner
  • TODO create a bridge towards SSP utility classes, so they can be easily mocked

@cicnavi cicnavi requested a review from tvdijen December 2, 2023 11:25
@tvdijen
Copy link
Member

tvdijen commented Dec 2, 2023

It's a bit much to review, but I think it's mostly about the strict_types=1, right?

@cicnavi
Copy link
Collaborator Author

cicnavi commented Dec 2, 2023

Erm, actually there was fair amount of refactoring done. I've described changes in https://github.com/simplesamlphp/simplesamlphp-module-oidc/blob/dev-v5/UPGRADE.md

The most important for me is that you are ok with creating the major release of the module compatible with SSP v2.1 now (without the above mentioned features that I've originally planed in v5).

@tvdijen
Copy link
Member

tvdijen commented Dec 2, 2023

Yeah sure, go ahead!

@cicnavi cicnavi merged commit 9b3cd12 into master Dec 3, 2023
14 checks passed
@cicnavi cicnavi deleted the dev-v5 branch December 3, 2023 11:07
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.

2 participants