Skip to content

OAuth: Wizard page#5825

Merged
ogoffart merged 3 commits intomasterfrom
oauth2
Jun 12, 2017
Merged

OAuth: Wizard page#5825
ogoffart merged 3 commits intomasterfrom
oauth2

Conversation

@ogoffart
Copy link
Copy Markdown
Contributor

@ogoffart ogoffart commented Jun 7, 2017

Show smething sensible if the user click on the owncloud icon, and handle errors

Issues: #5813 and #5811

@ogoffart ogoffart requested a review from ckamm June 7, 2017 15:09
Copy link
Copy Markdown
Contributor

@ckamm ckamm left a comment

Choose a reason for hiding this comment

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

Some minor comments. At this point I'd really like to try it out. What kind of setup changes do I need?

Comment thread src/gui/owncloudsetupwizard.cpp Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why? Isn't it okay to stay on the HttpCreds page if auth fails?

Also, isn't this unrelated to OAuth?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oops, it was supposed to be the OAuth page of course. Well spotted.

(This normally should not never happen because if we have a oauth connection, it should normaly work always. )

Comment thread src/gui/wizard/owncloudoauthcredspage.h Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We no longer use Q_DECL_OVERRIDE?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

AFAIK we dropped centos 6 whi had that old compiler. And even then we had upgraded to a more recent one.

Comment thread src/gui/wizard/owncloudwizard.cpp Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is there the need to re-enable the back button on other pages? In case we add further pages, or OAuth isn't supported and we fall back to http creds?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

  1. The QWizard already reset the enabled property on the buttons each time the page change.
  2. Since the next page is the final one and its not possible to go back, either, we are fine.

@guruz guruz added this to the 2.4.0 milestone Jun 8, 2017
@guruz guruz added Design & UX p2-high Escalation, on top of current planning, release blocker labels Jun 8, 2017
We don't want to re-open the browser in that case.
@ogoffart
Copy link
Copy Markdown
Contributor Author

ogoffart commented Jun 9, 2017

@ckamm In order to try it out, you simply need to checkout the oauth2 apps in your apps directory
https://github.com/owncloud/oauth2/ and enable it from the apps setting page.

Normally, since owncloud/oauth2#38 , the client app should automatically be registered, but this did not happen to me or @SamuAlfageme . So I had to manually add the client in the server DB:

INSERT INTO oc_oauth2_clients (identifier, secret, redirect_uri, name) VALUES('xdXOt13JKxym1B1QcEncf2XDkLAexMBFwiT9j6EfhhHFJhs2KM9jbjTmf8JBXE69', 'UBntmLjC2yYCeHwsyj73Uwo9TAaecAetRwMw0xYcvNL9yRdLSUi0hUAHfvCHFeFh', 'http://localhost:*', 'Desktop Client');

Alternatively, you can also register a new client from the admin interface, and change the values in theme.cpp

Copy link
Copy Markdown
Contributor

@ckamm ckamm left a comment

Choose a reason for hiding this comment

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

I'll also do a test run later.

@ckamm
Copy link
Copy Markdown
Contributor

ckamm commented Jun 12, 2017

@ogoffart From testing:

  • Works!
  • I also had to manually insert the "Desktop Client" oauth client. The table was empty initially.
  • "Login Successfull" -> one too many "l" (will fix)
  • I had one http and one oauth account set up for the same user+server, that lead to problems. (same keychain entry, probably) We might want to use some unique identifier to disambiguate. (will make ticket)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Design & UX p2-high Escalation, on top of current planning, release blocker

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants