Skip to content
This repository was archived by the owner on Jul 14, 2025. It is now read-only.

Conversation

ursinewalrus
Copy link
Contributor

Hi!

The way the old json walk worked prevented us from pulling the email address out of the json response our oauth provider sent back. The format looked something like

{ "emails" : 
   [ { "value" : "theemail@adress.com"} ]
}

The old way would return { "value" : "theemail@address.com"}, where of course we only wanted the value of "value".
It is changed so now you can write your path like
emails[index].value
to get it, or if you do emails.value it will still default to an index of 0 so as not to break old paths.

@ursinewalrus
Copy link
Contributor Author

ursinewalrus commented Apr 10, 2020

In the plugin_spec.rb class there is the comment

Need to load plugin.rb to avoid:

 NameError:
   uninitialized constant OAuth2BasicAuthenticator

 And need to mock various methods to avoid:

 NoMethodError:
   undefined method `enabled_site_setting' for main:Object

 etc.

How would I go about doing these things, it's not entirely clear to me.

@davidtaylorhq
Copy link
Member

davidtaylorhq commented Apr 14, 2020

Hi @ursinewalrus, thanks for the PR. It looks the build is failing because of rubocop linting issues. You can find more info about running that in your development environment here: https://meta.discourse.org/t/rubocop-has-landed-on-discourse-policewoman-policeman/67211 Edit: Oops, I looked at the wrong build. I see you got that fixed

The comments in plugin_spec.rb are describing the code - no need for you do do anything there. You should be able to run the plugin specs using rake plugin:spec[discourse-oauth2-basic]. Let us know if you run into any issues getting them running.

We will need to get some new rspec tests added for this new feature, so that we can be sure it won't regress in future.

@ursinewalrus
Copy link
Contributor Author

Hi David!

Perhaps there is a difference here, I am running it following https://meta.discourse.org/t/beginners-guide-to-install-discourse-on-macos-for-development/15772 , which I think might have some differences from the install that assumes you have a domain?

Running "rake plugin:spec[discourse-oauth2-basic]"
gets me
"zsh: no matches found: plugin:spec[discourse-oauth2-basic]"

Running
"rake plugin:spec"
Seems to start to load all the plugins, but when it gets to this one it throws the error
" uninitialized constant OAuth2BasicAuthenticator"
and when I run
"LOAD_PLUGINS=1 bundle exec rspec plugins/discourse-oauth2-basic/spec/plugin_spec.rb" - what I was originally trying to run, it's just mentioned at the bottom of the readme, I get the same
" uninitialized constant OAuth2BasicAuthenticator" error. I think I am missing some baseline bit of info here, some unknown unknowns.

@davidtaylorhq
Copy link
Member

"zsh: no matches found: plugin:spec[discourse-oauth2-basic]"

zsh is the new default shell for macOS, but I was still using bash. I tried switching to zsh and ran into the same issue as you. After chatting to some other members of the team I found the solution - I wrote a quick topic on Meta about it, so that other people can search and find the solution in future: https://meta.discourse.org/t/solving-zsh-no-matches-found-error-when-running-rake-task/148011

uninitialized constant OAuth2BasicAuthenticator

I'm not sure exactly why this was happening in development, but working fine in CI. Regardless, I just pushed 47a8211 to master which should resolve the issue, and remove some of the hacks we had in plugin_spec.rb. Please try rebasing your branch onto master and let me know if it works.

@ursinewalrus ursinewalrus force-pushed the json_walk_handles_arrays branch from a253c6d to 1ed49bf Compare April 15, 2020 13:57
SiteSetting.oauth2_json_email_path = 'emails[1].value'
result = authenticator.json_walk({}, JSON.parse(json_string), :user_id)

expect(result).to eq nil
Copy link

Choose a reason for hiding this comment

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

shouldn't we be expecting test2@example.com here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Had a copy issue and because of how I was sort of pre-testing the tests locally the issue with it incorrectly passing for me didn't show up. The passed in token also should have been :email, or the oauth2_json path should have been the oauth2_json_user_id_path

@ursinewalrus ursinewalrus force-pushed the json_walk_handles_arrays branch from 4cce401 to 0695af7 Compare April 17, 2020 15:55
@mcwumbly
Copy link

@riking - it looks to me like @ursinewalrus addressed your concerns above. anything else blocking this PR now?

@davidtaylorhq
Copy link
Member

This is looking good to me, thanks @ursinewalrus and @mcwumbly

@davidtaylorhq davidtaylorhq merged commit ca5f555 into discourse:master Apr 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants