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

Make omniauth_openid_connect gem compatible with omniauth v2.0 #95

Merged

Conversation

tim-s-ccs
Copy link
Contributor

As the omniauth is now at version > 2.0, I have updated omniauth_openid_connect so that it is compatible with the latest version. I also updated the other dependencies so that they use the latest versions as well.

There were two major changes I found that needed to be fixed:

  1. In omniauth the following methods request_path was updated to include script_name, The method for script_name is:
def script_name
  @env['SCRIPT_NAME'] || ''
end

and it would raise an error whenever request_path was called as @env would be nil. Therefore, I added a method into lib/omniauth/strategies/openid_connect.rb which will return '' if @env is nil and will use the method in lib/omniauth/strategy.rb as normal if @env is not nil. This is similar to what was done for session already.

  1. There was a change in the omniauth gem for the current_path in lib/omniauth/strategy.rb which changed from using path_info to the path method as path_info has been depreciated. This did not affect the code in the library but it did impact on the tests. This is because path_info was being stubbed. Therefore, I changed the places in the tests where path_info was being stubbed to stub path instead.

After these changes were made, I was able to run all the test successfully.
Screenshot 2021-08-31 at 10 28 10

@formigarafa
Copy link
Collaborator

formigarafa commented Sep 9, 2021

I hope this one get merged.
I have a PR open since April (#88) could not make it pass the specs on github even though they passed local.
There was another on (#84) closed because of lack of activity.

Yours at least is green(edit: They disabled rubocop check), I am crossing my fingers to get this one in.

@tim-s-ccs
Copy link
Contributor Author

I hope this one get merged.
I have a PR open since April (#88) could not make it pass the specs on github even though they passed local.
There was another on (#84) closed because of lack of activity.

Yours at least is green(edit: They disabled rubocop check), I am crossing my fingers to get this one in.

It's been a long time since there were any updates on this gem. I think at this point you're better off just using your own fork of it, that's what we are doing. I can't see this PR making any more progress than yours was able to I'm afraid.

@stale
Copy link

stale bot commented Nov 9, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Nov 9, 2021
@slevesque
Copy link

What is blocking from merging this one ?

@stale stale bot removed the wontfix label Nov 9, 2021
@tim-s-ccs
Copy link
Contributor Author

What is blocking from merging this one ?

The project is not being supported anymore it seems.
This fix can be used however so you will need to target the branch.

@slevesque
Copy link

What is blocking from merging this one ?

The project is not being supported anymore it seems. This fix can be used however so you will need to target the branch.

Time to have a new project maintainer so ?!

@tim-s-ccs
Copy link
Contributor Author

Time to have a new project maintainer so ?!

It looks like someone has already done this #99:
https://github.com/netsphere-labs/omniauth-openid-connect/

@m0n9oose
Copy link
Collaborator

m0n9oose commented Nov 18, 2021

Sorry for this huge delay. You're right, i could use some help with maintaining this project.

Back to subject: just merged very similar PR. Could you please rebase your working branch and resolve conflicts, so i could merge some of these changes.

@tim-s-ccs & @formigarafa if you interested in helping me with with maintaining this repo - please let me know

@tim-s-ccs
Copy link
Contributor Author

Sorry for this huge delay. You're right, i could use some help with maintaining this project.

Back to subject: just merged very similar PR. Could you please rebase your working branch and resolve conflicts, so i could merge some of these changes.

@tim-s-ccs & @formigarafa if you interested in helping me with with maintaining this repo - please let me know

If I had better knowledge of how it all worked I would but unfortunately I don't. My knowledge doesn't go much beyond 'what changes do I need to make this compatible with omniauth v2.0' 😞 . If there is anything else I can do please ask.

@tim-s-ccs
Copy link
Contributor Author

I'm going to close this as a similar PR was merged which does everything this PR does already, just in a slightly different way regarding one scenario.

@tim-s-ccs tim-s-ccs closed this Nov 18, 2021
@formigarafa
Copy link
Collaborator

Sorry for this huge delay. You're right, i could use some help with maintaining this project.

Back to subject: just merged very similar PR. Could you please rebase your working branch and resolve conflicts, so i could merge some of these changes.

@tim-s-ccs & @formigarafa if you interested in helping me with with maintaining this repo - please let me know

I wouldn't mind giving you a hand with that. I can try.

@Eric-Guo Eric-Guo reopened this Dec 30, 2021
@Eric-Guo Eric-Guo merged commit 88ae46e into omniauth:master Dec 30, 2021
@Eric-Guo
Copy link
Collaborator

will testing this branch and feedback later.

papayaah added a commit to health2works/omniauth_openid_connect that referenced this pull request Oct 7, 2022
…0.4.0

* upstream/master: (32 commits)
  Fix date in CHANGELOG.md
  Fixes rubocop issues
  Setup Coveralls.io as Github Action
  Fix rubocop errors
  Replace deprecated rubocop_linter_action
  Update README.md
  Fix date on CHANGELOG.md
  Update VERSION to v0.4.0
  Update CHANGELOG.md for v0.4.0
  Support dynamic parameters to the authorize URI (omniauth#90)
  Upgrade Faker and replace Travis with Github Actions (omniauth#102)
  Try to fix rubocop documentation_url not round error.
  Make `omniauth_openid_connect` gem compatible with `omniauth v2.0` (omniauth#95)
  Fall back to the discovered jwks when no key specified (omniauth#97)
  Allow to update omniauth to 2 (omniauth#88)
  Update README.md
  Bump version to 0.3.5
  bugfix: info from decoded id_token is not exposed (omniauth#61)
  bugfix: NoMethodError (undefined method `count' for #<OpenIDConnect::ResponseObject::IdToken:0x0000000008d9dde0>): (omniauth#60)
  Bump version to 0.3.4
  ...
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.

5 participants