Feature/support multiple auth headers in gb #11556

khaykov · 2020-03-30T02:55:27Z

This PR addresses changes in GB side to allow multiple headers to be passed to editor's `OkHttpHeaderInterceptor.

GB sitde changes are here.

To test:

Using a private site, navigate to post that contains an image using GB or Aztec.
Notice that the image is correctly loaded.

PR submission checklist:

I have considered adding unit tests where possible.
I have considered adding accessibility improvements for my changes.
I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

WordPress/src/main/java/org/wordpress/android/ui/utils/AuthenticationUtils.kt

peril-wordpress-mobile · 2020-03-30T03:01:29Z

You can test the changes on this Pull Request by downloading the APK here.

mchowning · 2020-04-06T12:03:09Z

libs/editor/WordPressEditor/src/main/java/org/wordpress/android/editor/AztecEditorFragment.java

@@ -147,6 +147,8 @@ public AztecLoggingException(Throwable originalException) {
    private static final String TEMP_VIDEO_UPLOADING_CLASS = "data-temp-aztec-video";
    private static final String GUTENBERG_BLOCK_START = "<!-- wp:";

+    private static final String AUTHORIZATION_HEADER_NAME = "Authorization";


Would it make sense to reuse the AUTHORIZATION_HEADER_NAME from AuthenticationUtils here instead of declaring a new variable?

Yes, but AztecEditorFragment is part of the Editor submodule, so It has no access to AuthenticationUtils. This is not super elegant, but my understanding is that we slowly but surely switch towards block editor, so I decided not to add any new logic related to it. But maybe there is some simple way to fix it, that I'm missing?

Yes, but AztecEditorFragment is part of the Editor submodule, so It has no access to AuthenticationUtils.

Right, I forgot about that minor detail. 🤦‍♂

mchowning · 2020-04-06T12:51:42Z

WordPress/src/main/java/org/wordpress/android/ui/posts/EditPostActivity.java

        String token = mAccountStore.getAccessToken();
        if (mSite.isPrivate() && WPUrlUtils.safeToAddWordPressComAuthToken(url)
            && !TextUtils.isEmpty(token)) {
-            authHeader = "Bearer " + token;
+            authHeaders.put(AuthenticationUtils.AUTHORIZATION_HEADER_NAME, "Bearer " + token);
        }


In AuthenticationUtils when safeToAddWordPressComAuthToken is false we try to use HTTP auth credentials, and it's not clear to me why we don't do that here. Similarly, we're checking mSite.isPrivate() here, but we don't check that in the AuthenticationUtils.

I know this isn't anything you changed, I'm just curious if you know why there's the difference. If you do know why they are different, it might be useful to move this logic into a method in AuthenticationUtils with some explanation about why the two methods are different. This is not anything that needs to block this PR.

I'm not 100% sure about the difference there, but as far as I'm aware, AuthenticationUtils is mostly used with image loading, and requests that actually get to it are subject to isPrivate() check since it's related to Photon/https (I have only surface level of knowledge there).

I'm not sure why we are not using HTTP auth credentials in the block editor, but since it's not broken I would rather not touch it :)

I'm not sure why we are not using HTTP auth credentials in the block editor, but since it's not broken I would rather not touch it :)

I think that makes sense. Obviously it would be nice to figure out the cause of the inconsistency, but I don't think it's something we have to do now.

mchowning · 2020-04-06T12:53:42Z

I think this looks good @khaykov . 👍 I just had a couple of small comments, and they may not even require any changes.

Also, it's not clear to me why we're making this change, would you mind explaining a bit about why we're making this change?

…ss-Android into feature/support-multiple-auth-headers-in-gb # Conflicts: # libs/gutenberg-mobile

peril-wordpress-mobile · 2020-04-06T19:18:17Z

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

khaykov · 2020-04-06T19:38:48Z

@mchowning Thanks for taking a look at this one! I'm doing it so we can add an extra header for the upcoming private Atomic sites support. A cookie is used there to retrieve private media, but in an unorthodox way (same as a token, without using Set-Cookie header, so we can't use a cookie jar).

mchowning

Looks good! Nice work! 👍

mchowning · 2020-04-08T17:38:01Z

Just a reminder that we need to update the gutenberg-mobile submodule ref now that the GB-side changes have been merged.

khaykov · 2020-04-08T23:11:22Z

Updated the GB reference 👍

mchowning · 2020-04-09T13:12:03Z

Double checked with the update, and everything is working great!

khaykov added 6 commits March 29, 2020 18:33

Updated GB reference

d7644bc

Moved Auth header name to constant

93bb5dc

Updated listener signature

1ebf3e6

Updated headers handling in Aztec

1950a23

Updated headers handling in EditPostActivity

4797668

Using constant for header name in Aztec.

3ce8113

khaykov added [Status] Needs Code Review gutenberg-mobile labels Mar 30, 2020

khaykov mentioned this pull request Mar 30, 2020

Accept multiple headers through OnAuthHeaderRequestedListener wordpress-mobile/gutenberg-mobile#2080

Merged

2 tasks

wpmobilebot reviewed Mar 30, 2020

View reviewed changes

WordPress/src/main/java/org/wordpress/android/ui/utils/AuthenticationUtils.kt Show resolved Hide resolved

Removed empty line.

846f0d2

mchowning reviewed Apr 6, 2020

View reviewed changes

Merge branch 'develop' of https://github.com/wordpress-mobile/WordPre…

6ff531f

…ss-Android into feature/support-multiple-auth-headers-in-gb # Conflicts: # libs/gutenberg-mobile

mchowning approved these changes Apr 7, 2020

View reviewed changes

Updated GB reference.

32efd51

mchowning merged commit c27e85e into develop Apr 9, 2020

mchowning deleted the feature/support-multiple-auth-headers-in-gb branch April 9, 2020 13:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/support multiple auth headers in gb #11556

Feature/support multiple auth headers in gb #11556

khaykov commented Mar 30, 2020 •

edited

Loading

peril-wordpress-mobile bot commented Mar 30, 2020 •

edited

Loading

mchowning Apr 6, 2020

khaykov Apr 6, 2020

mchowning Apr 7, 2020

mchowning Apr 6, 2020 •

edited

Loading

khaykov Apr 6, 2020

mchowning Apr 7, 2020

mchowning commented Apr 6, 2020 •

edited

Loading

peril-wordpress-mobile bot commented Apr 6, 2020 •

edited

Loading

khaykov commented Apr 6, 2020

mchowning left a comment

mchowning commented Apr 8, 2020

khaykov commented Apr 8, 2020

mchowning commented Apr 9, 2020

Feature/support multiple auth headers in gb #11556

Feature/support multiple auth headers in gb #11556

Conversation

khaykov commented Mar 30, 2020 • edited Loading

peril-wordpress-mobile bot commented Mar 30, 2020 • edited Loading

mchowning Apr 6, 2020

Choose a reason for hiding this comment

khaykov Apr 6, 2020

Choose a reason for hiding this comment

mchowning Apr 7, 2020

Choose a reason for hiding this comment

mchowning Apr 6, 2020 • edited Loading

Choose a reason for hiding this comment

khaykov Apr 6, 2020

Choose a reason for hiding this comment

mchowning Apr 7, 2020

Choose a reason for hiding this comment

mchowning commented Apr 6, 2020 • edited Loading

peril-wordpress-mobile bot commented Apr 6, 2020 • edited Loading

khaykov commented Apr 6, 2020

mchowning left a comment

Choose a reason for hiding this comment

mchowning commented Apr 8, 2020

khaykov commented Apr 8, 2020

mchowning commented Apr 9, 2020

khaykov commented Mar 30, 2020 •

edited

Loading

peril-wordpress-mobile bot commented Mar 30, 2020 •

edited

Loading

mchowning Apr 6, 2020 •

edited

Loading

mchowning commented Apr 6, 2020 •

edited

Loading

peril-wordpress-mobile bot commented Apr 6, 2020 •

edited

Loading