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

Refactor in Search of Fixes #59

Merged
merged 20 commits into from
Jun 2, 2018
Merged

Refactor in Search of Fixes #59

merged 20 commits into from
Jun 2, 2018

Conversation

dshanske
Copy link
Member

@pfefferle This seemed to solve your issue of local login...I tested it and was able to log into my test site using my live site.

I also added a series of extra checks for some of the errors people were experiencing, even though I could not reliably reproduce them all.

@dshanske dshanske requested a review from pfefferle May 27, 2018 15:17
@miklb
Copy link
Contributor

miklb commented May 28, 2018

I can confirm using the master branch of micropub & this branch that most of my issues are resolved save the error in Omnibear. But I am now able to authenticate and post from Quill.

These fixes also passed the 800 tests in micropub.rocks (I'm not sure where one fix stops and another begins however)

@pfefferle
Copy link
Member

I still get "Invalid authorization code".

@dshanske
Copy link
Member Author

In the hooks branch?

@dshanske
Copy link
Member Author

I logged into my test site with the endpoint set to my live site.

@dshanske
Copy link
Member Author

You are keeping each site's configuration on local or custom?

@pfefferle
Copy link
Member

I tried the code of this merge request and both sites using „build in endpoints“

@pfefferle
Copy link
Member

@pfefferle
Copy link
Member

pfefferle commented May 29, 2018

Local endpoint does not mean that you can do a local lookup! Perhaps you can check if the redirect/endpoint domain is the same as the blog domain or something similar to do a local lookup...

if (parse_url($endpoint, PHP_URL_HOST) === parse_url(site_url(), PHP_URL_HOST) {
	$params = self::verify_local_authorization_code( $post_args );
}

instead of

if ( 'local' === $option ) {
	$params = self::verify_local_authorization_code( $post_args );
}

...but I am not sure if this makes sense at all.

@pfefferle
Copy link
Member

pfefferle commented May 29, 2018

I will try to explain my use case once again, but from my perspective it is the only valid use case for an IndieAuth login and it is how OpenID and IndieLogin works.

Use case: We have https://indiewp.com as a multiuser blog. This blog is set to local endpoints, but this doesn't matter, because these settings should never be used for this flow (they will only be used if we use indiewp.com to login to another site).

@dshanske should be able to login using david.shanske.com, @aaronpk should be able to login with aaronparecki.com and I should be able to login with notiz.blog so indiewp.com always has to verify the code against the remote endpoints and never use his own!

@pfefferle
Copy link
Member

pfefferle commented May 29, 2018

I think we discussed a lookup against the local DB but it was in a different context... I think it was something with the token endpoint...

@pfefferle
Copy link
Member

@aaronpk am I wrong?

@pfefferle
Copy link
Member

@aaronpk and @dshanske and sorry once again for not verifying the latest merge requests properly...

@aaronpk
Copy link
Member

aaronpk commented May 29, 2018

I think this is confusing because you're trying to have this plugin do two things.

  1. This plugin is an IndieAuth authorization server when signing in to other apps
  2. This plugin also authenticates users using their own IndieAuth server

Again this is why I was trying to have you document the use cases clearly so that we had something to reference while testing the changes.

@pfefferle
Copy link
Member

@aaronpk I can document this, but isn't this the normal case?

@aaronpk
Copy link
Member

aaronpk commented May 29, 2018

I don't know what you mean by "normal case". Certainly some users will only ever sign in to their wordpress with a username and password, and will never use number 2 above. I expect once this plugin is stable, that number 1 above will be the most common case, for signing in to other apps using someone's own wordpress site as their identity.

@pfefferle
Copy link
Member

@aaronpk don't get me wrong... I am not really heavily using this feature, but it is part of the plugin, so I tried to test every possibility...

@pfefferle
Copy link
Member

@aaronpk with "normal" I meant the "normal IndieAuth login flow".

Copy link
Member

@pfefferle pfefferle left a comment

Choose a reason for hiding this comment

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

remove the local verification code

if ( 'local' === $option ) {
	$params = self::verify_local_authorization_code( $post_args );
}

@dshanske
Copy link
Member Author

@pfefferle That isn't a new feature. It was already there. Shouldn't that be a separate PR to revert that change?

@dshanske
Copy link
Member Author

@pfefferle I tried to find a middle ground. It looks to see if the endpoint is on the same site, which would mean it can be verified locally.

@pfefferle
Copy link
Member

Thanks for the quick fix, but is this really a case? That you use david.shanske.com to log in to david.shanske.com using IndieAuth? Because it ends up in a redirect hell and in the end you have to use your login and password anyways. But yes it should work.

But you forgot one check... there were two of these checks in the code, I think!

@dshanske
Copy link
Member Author

The other one doesn't check for an endpoint

@pfefferle
Copy link
Member

That‘s why I would prefer to remove it. The Web Sign-In should never use the local verification.

@dshanske
Copy link
Member Author

Should removing the change be in this PR or a separate one?

@pfefferle
Copy link
Member

I have no preferences for that 😉

@pfefferle
Copy link
Member

pfefferle commented May 30, 2018

But I would vote for a minor version update instead of a patch, after so many changes.

@aaronpk
Copy link
Member

aaronpk commented May 30, 2018

People have definitely installed this plugin with the intent to log in to their site via Twitter RelMeAuth using the same domain.

@dshanske
Copy link
Member Author

So, should we be supporting that, aaronpk?

@pfefferle
Copy link
Member

I thought about that, since yesterday and we should decouple the settings from IndieAuth and Web Sign-In a bit more and in this step we should also add indielogin as option. I would like to propose a settings change (I would also move the settings to a dedicated place, as I did in the webmention plugin) on weekend.

@dshanske
Copy link
Member Author

That's doable, but what will the web sign in feature look like?

@dshanske
Copy link
Member Author

Indielogin isn't a good idea till aaronpk opens it up

@pfefferle
Copy link
Member

What do you mean with „open it up“?

@aaronpk
Copy link
Member

aaronpk commented May 30, 2018

Right now indielogin.com is in beta, and I'm not opening it up for other developers to use that instance just yet. (It's open source so of course you could host your own.) Once I finish email login (and maybe PGP authentication) I'll launch it. However I am also considering requiring that people sign up as a developer in order to use it once it's live, since that will help avoid getting into the situation we're in with indieauth.com now. I could see about making an account for this wordpress plugin though.

@dshanske dshanske merged commit 783b513 into indieweb:master Jun 2, 2018
@dshanske dshanske deleted the hooks branch July 19, 2020 19:05
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.

4 participants