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

Add a trusted domain wizard #10584

Merged
merged 3 commits into from
Aug 22, 2014
Merged

Conversation

LukasReschke
Copy link
Member

Adds a little button to the trusted domain warning, if an admin clicks on the warning he will be redirected to ownCloud and asked whether he want to trust this domain.

By far not the cleanest code, or clean at all, but does the job and I don't see a reason to make a lot of changes for this little improvement. I think there is no need in making "yet another" admin setting, automagic is at least preferred by myself ;-)

A check on the wording would be much appreciated.

How it looks:
screen shot 2014-08-21 at 22 22 56
screen shot 2014-08-21 at 22 23 06

@MTRichards @craigpg @karlitschek

Adds a little button to the trusted domain warning, if an admin clicks on the warning he will be redirected to ownCloud and asked whether he want to trust this domain.

By far not the cleanest code, or clean at all, but does the job and I don't see a reason to make a lot of changes for this little improvement.
@ghost
Copy link

ghost commented Aug 21, 2014

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/6834/

@MorrisJobke
Copy link
Contributor

@LukasReschke I like your admin name :)

@karlitschek
Copy link
Contributor

very nice improvement. 👍 Not tested.
If the user is currently not logged-in as admin than this probably fails without feedback to the user, right?

@LukasReschke
Copy link
Member Author

User is not logged-in: Should get redirected to login page and after login to the admin page where this window is open.

User has no admin rights: User is redirected to default app since he can't open the admin page. So no message.

@MorrisJobke
Copy link
Contributor

User is redirected to default app since he can't open the admin page. So no message.

But then you can access the ownCloud instance: shouldn't that be denied by the trusted domain setting?

@MorrisJobke
Copy link
Contributor

Got it. You redirect to a valid domain.

@LukasReschke
Copy link
Member Author

Yup. The URL generator uses the first specified valid domain in case a domain is untrusted.

}

if(isset($_POST['trustedDomain'])) {
$trustedDomains = OC_Config::getValue('trusted_domains');
Copy link
Member

Choose a reason for hiding this comment

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

OC_Config is depricated

@DeepDiver1975
Copy link
Member

admin.js is not loaded and as a result clicking the button "Add ...." just reloads the admin settings for me

@DeepDiver1975
Copy link
Member

the message box is not closing after hitting yes ?

@LukasReschke
Copy link
Member Author

Well. It should - what browser?

@DeepDiver1975
Copy link
Member

chromium

@DeepDiver1975
Copy link
Member

but it looks like there are some js errors in master for me 👿

@DeepDiver1975
Copy link
Member

looks like odfviewer and texteditor has issues ...

@DeepDiver1975
Copy link
Member

👍

@DeepDiver1975
Copy link
Member

well - hitting reload after the message box disappeared will show the message box again and again

@LukasReschke
Copy link
Member Author

well - hitting reload after the message box disappeared will show the message box again and again

I'll take a look at this.

OC_Config is depricated

Requires changes to the public API since not everything is exposed. I'll take care of it.

@DeepDiver1975
Copy link
Member

Requires changes to the public API since not everything is exposed. I'll take care of it.

It would be fine for me for now to replace OC_Config:: by \OC::$server->getConfig()->

@LukasReschke
Copy link
Member Author

It would be fine for me for now to replace OC_Config:: by \OC::$server->getConfig()->

Then we should also be able to use setValue ;-)

@DeepDiver1975
Copy link
Member

Then we should also be able to use setValue ;-)

I see - hmmmm - there was a reason for not adding this ....

.. what the heck - let's add the method

@LukasReschke
Copy link
Member Author

@DeepDiver1975 I exposed setSystemValue and added a JS redirect.

@scrutinizer-notifier
Copy link

A new inspection was created.

@DeepDiver1975
Copy link
Member

ready for merge from my pov 👍

@ghost
Copy link

ghost commented Aug 22, 2014

💣 Test Failed. 💣
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/6840/

@DeepDiver1975
Copy link
Member

💣 Test Failed. 💣
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/6840/

only js unit tests

DeepDiver1975 added a commit that referenced this pull request Aug 22, 2014
@DeepDiver1975 DeepDiver1975 merged commit a77d468 into master Aug 22, 2014
@DeepDiver1975 DeepDiver1975 deleted the simple-wizard-trusted-domains branch August 22, 2014 15:12
@DeepDiver1975
Copy link
Member

not a bug fix but still interesting for oc7

@karlitschek ???

@LukasReschke
Copy link
Member Author

@karlitschek What about backporting this for 7.0.3?

@karlitschek
Copy link
Contributor

yes. please backport :-)

@MorrisJobke
Copy link
Contributor

stable7 dac8dea 09ab1f1 0a2e471

@ronnicek
Copy link

ronnicek commented Sep 5, 2014

One more thing here.. I just test that and it didn´t add port (if you are using different one) to the configuration. My web is running on test.domain.com:4443 and in config is only test.domain.com and owncloud is asking again and again for trusted domain.

at least on OC 7.0.2

@MTRichards
Copy link
Contributor

Good catch. @LukasReschke how are custom ports handled normally in this domain?

@MTRichards MTRichards added this to the 2014-sprint-03-current milestone Sep 5, 2014
@LukasReschke
Copy link
Member Author

I think they are ignored in the wizard. I haven't thought of that case...

Let me create a PR for this next week. I'm off this weekend.

LukasReschke added a commit that referenced this pull request Sep 23, 2014
@lock lock bot locked as resolved and limited conversation to collaborators Aug 16, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants