Skip to content
This repository has been archived by the owner on Jan 27, 2021. It is now read-only.

Use the ocis-proxy URL to make sure auth works correctly #41

Merged
merged 2 commits into from
Aug 6, 2020

Conversation

PVince81
Copy link
Contributor

@PVince81 PVince81 commented Aug 6, 2020

Since a recent change in ocis-accounts that makes a distinction between
user id and user name, the thumbnails cannot properly authenticate
against the Reva API endpoint (port 9140) using the provided Bearer
token.

Since the ocis-proxy is handling authentication correctly, the default
setting has been changed here to connect to ocis-proxy.

Since a recent change in ocis-accounts that makes a distinction between
user id and user name, the thumbnails cannot properly authenticate
against the Reva API endpoint (port 9140) using the provided Bearer
token.

Since the ocis-proxy is handling authentication correctly, the default
setting has been changed here to connect to ocis-proxy.
@PVince81 PVince81 requested review from butonic and refs August 6, 2020 10:07
@PVince81 PVince81 self-assigned this Aug 6, 2020
@update-docs
Copy link

update-docs bot commented Aug 6, 2020

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@PVince81
Copy link
Contributor Author

PVince81 commented Aug 6, 2020

Fixes https://github.com/owncloud/ocis-thumbnails/issues/40

To test, check out owncloud/ocis#409 and the matching go.mod replaces.

Before this fix, the issue from https://github.com/owncloud/ocis-thumbnails/issues/40 happens.
After this fix, thumbnails appear again in Phoenix.

@PVince81 PVince81 mentioned this pull request Aug 6, 2020
63 tasks
req, err := http.NewRequest(http.MethodGet, u.String(), nil)
if err != nil {
return nil, fmt.Errorf("could not get the image \"%s\" error: %s", file, err.Error())
}

// FIXME: make this configurable!!
http.DefaultTransport.(*http.Transport).TLSClientConfig = &tls.Config{InsecureSkipVerify: true}
Copy link
Member

Choose a reason for hiding this comment

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

you could add your certificate to your the list of trusted certs

Copy link
Member

Choose a reason for hiding this comment

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

Ideally we should address this before merging

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do we have this pattern somewhere, like config switches ? or rely on running debug mode vs non-debug ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

found it, ocis-proxy has some config pattern. I'll use the same.

@@ -27,12 +28,17 @@ type WebDav struct {
func (s WebDav) Get(ctx context.Context, file string) (image.Image, error) {
u, _ := url.Parse(s.baseURL)
u.Path = path.Join(u.Path, file)
fmt.Printf("url: %s", u.String())
Copy link
Member

Choose a reason for hiding this comment

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

I'd use a logger, not print

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll remove this, it was purely for debugging

@PVince81
Copy link
Contributor Author

PVince81 commented Aug 6, 2020

@refs PR adjusted.

Note that I have made insecure to be true by default to follow the same pattern like in ocis-proxy...
We should change this and pass around the correct values from the ocis single binary: owncloud/ocis#435

@sonarcloud
Copy link

sonarcloud bot commented Aug 6, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@refs refs merged commit c15d98b into master Aug 6, 2020
@refs refs deleted the use-ocis-proxy branch August 6, 2020 14:46
ownclouders pushed a commit that referenced this pull request Aug 6, 2020
Merge: 523fad2 52163c9
Author: Alex Unger <zyxancf@gmail.com>
Date:   Thu Aug 6 16:46:33 2020 +0200

    Merge pull request #41 from owncloud/use-ocis-proxy
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants