Bug 1395356 - Use auth0 instead of login.taskcluster.net for SSO by helfi92 · Pull Request #3144 · mozilla/treeherder

helfi92 · 2018-01-17T16:40:38Z

Rough summary of the changes

Front end

The auth callback is written in React and lives under the /login.html endpoint. It communicates with Treeherder using the localStorage.

Credential expiration

The Django user session expiration is set to expire when the client access token or the id token expires (whichever one expires first). These values are controlled by the IAM team. Presently, the access token expires after 1 day and the id token expires after a week. That being said, the session will therefore expire after 1 day. If you want this value change, we simply need to send a request to the IAM team.

Credential renewal

Renewals are set to happen every 15 minutes or so. The renewal is skewed slightly so that different open tabs don't renew at the same time. Once renewal happens, both tokens are renewed and the Django session is updated.

Migration

If the userSession localStorage key is not set, then the user will be logged out including logging out from the Django session. In other words, all users will be automatically logged out when the merge to production happens.

helfi92 · 2018-01-18T15:10:43Z

@djmitche Could you review at a minimum the auth client-side code seeing you've done something similar for tools. Thanks!

edmorley

Hi! Massive massive thanks for working on this :-D

I've left some initial top-level comments below.

I also noticed that this PR increased the size of the vendor bundle by 200KB (even after minification), which seems a bit more than I would have expected? (Given that taskcluster-client-web is sharing the same Hawk version as taskcluster-client, so is already bundled)

edmorley · 2018-01-18T16:00:18Z

neutrino-custom/base.js

    neutrino.config
        .plugin('provide')
        .use(webpack.ProvidePlugin, {
+            auth0: require.resolve('auth0-js'),


This list here is leftover from before we used webpack. We're moving towards using imports where possible - could this be imported instead?

edmorley · 2018-01-18T16:03:45Z

ui/js/auth.js

+
+const authApp = angular.module('auth', ['LocalStorageModule', 'auth0.auth0', 'react']);
+
+authApp.config(['angularAuth0Provider', 'localStorageServiceProvider', '$locationProvider',


Would you mind adding this as a purely React entrypoint instead? (eg like for ui/test-view/index.js)

Auth0 docs have some examples for React here:
https://auth0.com/docs/quickstart/spa/react

(Our use of React components embedded in an AngularJS app for the other entrypoints is purely as a transition step, rather than something we like doing hehe 😄)

edmorley · 2018-01-18T16:06:26Z

treeherder/config/settings.py


+# Auth0 setup
+AUTH0_DOMAIN = env('AUTH0_DOMAIN', default="auth.mozilla.auth0.com")
+AUTH0_AUDIENCE = env('AUTH0_AUDIENCE', default="login.taskcluster.net")


Does the audience of login.taskcluster.net mean that cross-origin Django logins (either localhost:5000 UI -> localhost:8000/api, or localhost:5000 UI -> treeherder.mozilla.org/api) will work, since it will be the same for all environments?

If so, that will be awesome :-)

This part will, yes. The access_token will contain that audience, indicating to the login service that credentials should be granted corresponding to the access_token. Regardless of origin.

Now that I look again, this value should be used on the frontend when initiating an Auth0 authentication, but should not appear anywhere on the backend.

edmorley · 2018-01-18T16:17:20Z

package.json

    "angular-toarrayfilter": "1.0.3",
    "angular-ui-router": "0.4.3",
    "angular1-ui-bootstrap4": "2.4.22",
+    "auth0-js": "^9.0.2",


Looking at the Auth0 docs I see them mentioning an iframe version called auth0-lock, with "Auth0 Lock" having been mentioned in the bug too. Which should we be using?

The comparison in their docs (https://auth0.com/docs/libraries/when-to-use-lock) plus the bug comments make me think Lock is soon going to be the preferred approach? Or is that not possible with what we need here? (I've not looked at Lock in detail)

At Mozilla, we want to use the "hosted lock" as it embodies all the logic of who can login with LDAP, who with github, etc.

So it sounds like my question above is relevant? ie should be using the auth0-lock library instead?

I think one would use the lock widget if they want to customize some aspect of the dialog's appearance and behavior. Also, I think if you use the lock widget, then some features will be blocked.

From the auth0 docs:

The added customatization stuff makes auth0-lock a very heavy package heh. Using something like ncdu, I get:

Oh so auth0-lock != "hosted lock" ?

Either way, all the better if we don't need it then, given the size :-)

The docs are very confusing. I think we are using a Custom UI (not the original auth0 lock) in order to support things like LDAP logins. We are using our own lock UI which is stored in https://github.com/mozilla-iam/auth0-deploy/blob/master/pages/login.html.

Yeah, it's all very confusing. The "Hosted" lock is one the IAM team implemented and uploaded to auth.mozilla.auth0.com, not auth0-lock.js.

edmorley · 2018-01-18T16:20:05Z

package.json

    "redux": "3.7.2",
    "redux-debounce": "1.0.1",
-    "taskcluster-client": "2.5.4"
+    "superagent-promise": "^1.1.0",


superagent-promise doesn't appear to be directly by Treeherder, so unless it's a peerDependency of something can be removed.

It's deprecated now, anyway -- superagent uses promises directly.

edmorley · 2018-01-18T16:23:40Z

package.json

  "dependencies": {
    "ajv": "6.0.1",
    "angular": "1.6.8",
+    "angular-auth0": "^3.0.0",


Once the login callback page is pure React, this can be removed

edmorley · 2018-01-18T16:26:21Z

vagrant/env.sh


+# Auth0 Config
+export AUTH0_DOMAIN='auth.mozilla.auth0.com'
+export AUTH0_AUDIENCE='login.taskcluster.net'


settings.py has defaults for these, so these can probably be removed (unless we need to vary them more per environment)

edmorley · 2018-01-18T16:27:12Z

ui/login.html

@@ -0,0 +1,12 @@
+<!DOCTYPE html>


This template can be replaced with webpack's htmltemplate once this is pure React (see the test-view's template config in neutrino-custom/base.js for an example)

edmorley · 2018-01-18T16:56:31Z

requirements/common.txt

 enum34==1.1.6; python_version < '3' \
    --hash=sha256:6bd0f6ad48ec2aa117d3d141940d484deccda84d4fcd884f5c3d93c23ecd8c79
+
+python-jose==2.0.0 \


Looking at the Auth0 docs they say to use python-jose-cryptodome rather than python-jose (https://auth0.com/docs/quickstart/backend/python#install-the-dependencies).

However their recommendation doesn't have much GitHub activity and appears to just be a fork of python-jose that changed the vulnerable pycrypto dependency for pycryptodome (blog post about the vulnerability), but (a) hasn't kept up with python-jose, (b) is now redundant since python-jose has just switched to pycryptodome too (mpdavis/python-jose@98406bc). So ignoring the Auth0 docs seems to be the right thing to do.

However looking at https://jwt.io/#libraries-io there appears to be yet another JWT option that's much more popular/active than python-jose:
https://github.com/jpadilla/pyjwt/

As such, I think it would be good to:

Double check which of python-jose and PyJWT is best for our use-case/long term reliability

At some point file an issue against the Auth0 docs getting them to at least recommend python-jose and not the stale python-jose-pycryptodome package (or even suggest they use PyJWT if appropriate)

Created auth0/docs#5611.

Still no answer. I will leave it as python-jose (recommended by auth0) until I do hear back.

Could you bump this dep to the newly release 2.0.2? (https://pypi.python.org/pypi/python-jose)

edmorley · 2018-01-18T16:57:43Z

treeherder/auth/backends.py

 from django.contrib.auth.models import User
 from django.core.exceptions import ObjectDoesNotExist
-from rest_framework.reverse import reverse
-from taskcluster import Auth


This makes the taskcluster python package no longer used, so it and its subdependencies can be removed from requirements/common.txt 😃

(I love being able to reduce the number of deps - particularly in this case since the Python 3 only deps of taskcluster-py are very noisy for pyup updates!)

djmitche

@andrewkrug am I totally off-base with the idea that an id_token is most useful for identifying a user? The general gist of what we're doing here is building an SPA which additionally wants to establish a cookie-based session with a backend that identifies the user.

djmitche · 2018-01-18T22:22:28Z

ui/js/components/auth/auth.js

+            // for silent renewal, auth0-js opens this page in an iframe, and expects
+            // a postMessage back, and that's it.
+            if ($window !== $window.top) {
+                $window.parent.postMessage(window.location.hash, window.origin);


heh, $window is an angular reference to the browser's window object: https://docs.angularjs.org/api/ng/service/$window.

djmitche · 2018-01-18T22:25:55Z

ui/js/services/auth.js

+        });
+        const user = userResponse.data;
+
+        localStorageService.set('taskcluster.credentials', taskclusterCredentials);


These won't be valid for more than a few minutes -- is it really worth storing them?

I guess the only advantage is for the multiple Treeherder tabs use-case (since each listens to localstorage changes).

djmitche · 2018-01-18T22:26:27Z

ui/js/services/auth.js

+        const userResponse = await $http.get(loginUrl, {
+            headers: {
+                authorization: `Bearer ${userSession.accessToken}`,
+                clientId: taskclusterCredentials.clientId,


Why does the backend need the clientId? What would happen if I sent my access_token and your clientId?

It doesn't if I request the email scope before receiving the jwt token. Thanks for suggesting that!

djmitche · 2018-01-18T22:29:11Z

tests/ui/unit/init.js

 window.treeherderApp = jsContext('./treeherder_app.js');
 window.perf = jsContext('./perf.js');
 window.failureViewerApp = jsContext('./failureviewer.js');
+window.authApp = jsContext('./auth.js');


Should changes to this file in 9a5e427 be in a different commit?

djmitche · 2018-01-18T22:31:22Z

treeherder/auth/backends.py

+        now_in_milliseconds = int(round(time.time() * 1000))
+
+        # The Django user session expiration should be set to match the expiry time of the Auth0 token
+        request.session.set_expiry((expires_at_in_milliseconds - now_in_milliseconds) / 1000)


This probably shouldn't happen until the access_token is validated

djmitche · 2018-01-18T22:34:31Z

treeherder/auth/backends.py

+        # Per https://auth0.com/docs/quickstart/backend/python/01-authorization#create-the-jwt-validation-decorator
+        jsonurl = urlopen('https://' + AUTH0_DOMAIN + '/.well-known/jwks.json')
+        jwks = json.loads(jsonurl.read())
+        unverified_header = jwt.get_unverified_header(token)


Hm, I don't like the idea of hitting .well-known/jwks.json on every call to this method. In JS this gets cached automatically by an Auth0 library. It probably wouldn't be the worst thing to cache it on startup, assuming this runs on some platform that periodically restarts (like Heroku)

Yeah I was thinking this wasn't ideal too. The web dynos will be restarted every 24 hours (and "restart all the things" is an easy step that people are likely to perform if issues are seen), so caching once per process at startup seems fine. We could even have the file refetched iff validation fails.

djmitche · 2018-01-18T22:35:01Z

treeherder/auth/backends.py

+                    "use": key["use"],
+                    "n": key["n"],
+                    "e": key["e"]
+                }


Yikes, but this really seems like something a library should be doing for us :(

Ah, but now I see it copied from https://auth0.com/docs/quickstart/backend/python/01-authorization so I guess that's OK!

OK, so @andrewkrug pointed me to https://github.com/mozilla-iam/sso-dashboard/blob/master/dashboard/auth.py which uses josepy, but python-jose should be similar. I worry a lot about the urlopen call in this PR especially -- what if that 503's or we get rate-limited? He let me know that the public key at /.well-knon/jwks.json is under our (Mozilla's) control and that changing it would be a big thing with lots of notice in advance. So, I think we could configure a single JWK statically in the Treeherder config, and verify against that, similar to what is happening in the dashboard code linked above.

OK, this is much better now :)

djmitche · 2018-01-18T22:35:20Z

treeherder/auth/backends.py

+                    "e": key["e"]
+                }
+
+        if rsa_key:


else.. we just trust it? That's not good!

Good catch. I will throw an error if there's no rsa_key.

djmitche · 2018-01-18T22:39:42Z

treeherder/auth/backends.py

+                    token,
+                    rsa_key,
+                    algorithms=['RS256'],
+                    audience=AUTH0_AUDIENCE,


There's no need for the backend to verify this -- whether the access_token matters to login.taskcluster.net or not doesn't make a difference to the TH backend.

It seems to be required by python-jose. From the docs:

audience: The intended audience of the token. If the “aud” claim is included in the claim set, then the audience must be included and must equal the provided claim.

djmitche · 2018-01-18T22:41:00Z

treeherder/auth/backends.py

+                                    " token."}, 400)
+
+        client_id = request.META.get("HTTP_CLIENTID", None)
        email = self._extract_email_from_clientid(client_id)


Yikes! So if I get an access_token for me, then I can send a request to the backend with ClientId: email/emorley@mozilla.com and I'll be logged in as emorley?!

The user's identity needs to be determined from trusted data -- and that trusted data is the access_token. Honestly, I think the id_token is the better choice here. This page differentiates the two nicely (at the bottom):

ID tokens carry identity information encoded in the token itself, which must be a JWT

Access tokens are used to gain access to resources by using them as bearer tokens

So I think the id_token is the right tool to use here. The backend should either validate this itself and use the information in the claims -- if that's enough -- or call auth0 to get the required profile information. If I remember correctly, you can request additional fields (like email) in the id_token by passing them as scopes in the authentication request. So you could request email and then just use the email claim from the validated id_token.

If I use the id_token with python-jose, then I would still need to use the access_token when decoding the jwt.

From the docs:

access_token (str): ... If the “at_hash” claim is included in the claim set, then the access_token must be included, and it must match the “at_hash” claim.

Please correct me if I'm wrong here, but from my understanding, I will need the access token to get the email address.

helfi92 · 2018-01-23T17:26:15Z

Responded to the first iteration of comments.

djmitche

OK, I squinted at this and checked in with @andrewkrug and I'm pretty happy with it!

I'm still not convinced that we should be using an access_token here. If this were a "regular" Django application, we would be using the id_token to identify the user -- and it has all of the necessary claims (sub and email).

The objection (in one of the outdated comments above) was that the access_token was required if at_hash appears in the id_token. If so, let's pass both of them! But if memory serves, that id_token does not include an at_hash claim, so we should be OK.

In this case, the access token brings with it access to the Auth0 userinfo endpoint and all Taskcluster scopes afforded to the user. If we can avoid sending that to the backend, that'd be nice (but again, if we have to to verify the access_token hash, that's fine).

Benefits of the id_token:

fits definition of the tokens better
no concern for audiences
contains the info we need -- no reliance on /userinfo
always a JWT per OIDC standard (OIDC allows access_token to be an opaque identifier)
can be validated with the same JWT validation code already included here

djmitche · 2018-01-23T18:25:22Z

treeherder/auth/backends.py

+    def _get_clientid_from_userinfo(self, user_info):
        """
-        Extract the user's email from the client_id
+        Get the user's client_id from the jwt sub property


Can we rename this to "username" instead of "client_id"? We will likely change the format of client IDs issued by the login service at some point.

djmitche · 2018-01-23T18:25:51Z

treeherder/auth/backends.py

+        elif "email" in subject:
+            return "email/" + email
+        else:
+            raise AuthenticationFailed("Unrecognized identity")


I think this will fail for Github logins, which have "github" in the subject..

djmitche · 2018-01-23T18:38:17Z

treeherder/auth/backends.py

            # the user doesn't already exist, create it.
            logger.warning("Creating new user: {}".format(client_id))
-            return User.objects.create_user(client_id, email=email)
+            return User.objects.create_user(client_id, email=user_info['email'])


User.objects expects a username, so let's call this "username" and not "client_id", just to avoid confusion.

djmitche · 2018-01-23T18:52:33Z

ui/js/auth/AuthService.js

+        });
+        const loginUrl = `${location.protocol}//${location.host}/api/auth/login/`;
+
+        const taskclusterCredentials = await credentialAgent.getCredentials();


I don't think you need this anymore? It's generating credentials that are only good for a few minutes anyway..

@helfi92, could you confirm if used or else remove if not? :-)

djmitche · 2018-01-23T19:03:25Z

treeherder/auth/backends.py

+                token,
+                rsa_key,
+                algorithms=['RS256'],
+                audience=AUTH0_AUDIENCE,


I think this should be AUTH0_DOMAIN + "/userinfo" per https://auth0.com/docs/tokens/access-token -- that is the audience for access tokens returned from an authorization with scope=openid. Basically all we're doing with this token is handing it to that /userinfo endpoint (in _get_user_info), so I think that's sufficient.

Or, if verifying an id_token, it can be omitted entirely.

djmitche · 2018-01-23T19:36:11Z

treeherder/auth/backends.py

+                    "use": key["use"],
+                    "n": key["n"],
+                    "e": key["e"]
+                }


OK, so @andrewkrug pointed me to https://github.com/mozilla-iam/sso-dashboard/blob/master/dashboard/auth.py which uses josepy, but python-jose should be similar. I worry a lot about the urlopen call in this PR especially -- what if that 503's or we get rate-limited? He let me know that the public key at /.well-knon/jwks.json is under our (Mozilla's) control and that changing it would be a big thing with lots of notice in advance. So, I think we could configure a single JWK statically in the Treeherder config, and verify against that, similar to what is happening in the dashboard code linked above.

djmitche · 2018-01-23T19:39:02Z

treeherder/config/settings.py


+# Auth0 setup
+AUTH0_DOMAIN = env('AUTH0_DOMAIN', default="auth.mozilla.auth0.com")
+AUTH0_AUDIENCE = env('AUTH0_AUDIENCE', default="login.taskcluster.net")


Now that I look again, this value should be used on the frontend when initiating an Auth0 authentication, but should not appear anywhere on the backend.

djmitche · 2018-01-23T19:41:50Z

treeherder/auth/backends.py

+            urlopen(Request(user_info_url, headers={'Authorization': request.META.get('HTTP_AUTHORIZATION', None)})).read())
+
+    def authenticate(self, request):
+        self._validate_token(request)


So I think this is not strictly necessary for an access_token, since the next step is to pass it to /userinfo, which will also validate it. OIDC also doesn't guarantee that access_token will be a JWT, so it feels a little funny to be making this check.

djmitche

I think this is in good shape!

djmitche · 2018-01-24T20:12:15Z

treeherder/auth/backends.py

+                    "use": key["use"],
+                    "n": key["n"],
+                    "e": key["e"]
+                }


OK, this is much better now :)

edmorley

I'm really happy with how this is working out!

I've left a few comments below - some might affect the rest of the review, so will hold off the full review (/testing locally) for the moment.

Thank you for all your work on this!

edmorley · 2018-01-24T20:17:24Z

package.json

    "angular-toarrayfilter": "1.0.3",
    "angular-ui-router": "0.4.3",
    "angular1-ui-bootstrap4": "2.4.22",
+    "auth0-js": "^9.0.2",


So it sounds like my question above is relevant? ie should be using the auth0-lock library instead?

edmorley · 2018-01-24T20:26:24Z

ui/js/auth/AuthService.js

+        });
+        const loginUrl = `${location.protocol}//${location.host}/api/auth/login/`;
+
+        const taskclusterCredentials = await credentialAgent.getCredentials();


@helfi92, could you confirm if used or else remove if not? :-)

edmorley · 2018-01-24T20:33:21Z

treeherder/auth/backends.py

-            # skip and let another backend have a try at it.
-            return None
+        cached_jwks = json.load(open('treeherder/auth/jwks.json'))
+        cache.set(cache_key, cached_jwks)


If we're using a local file I think we should skip caching in Redis and load once at initial runtime?

Caching or not, I think both are fine. I'll send a commit to remove caching.

I think there are a few possible approaches (assuming embedding jwks.json in the codebase, which I think is a sensible approach):

(the approach used here previously) For each incoming auth request, check if the jwks is cached, if so use that, otherwise load from disk, and then store to cache.

(the approach used after the PR update) For each incoming auth request, load jwks from disk

At process start, load the jwks into a constant at the top of this file, which is referenced by each request

These all have different trade-offs:

Approach 1 makes a roundtrip to the remote Redis instance (it's not a localhost server or Django's locmem cache) for every request, to fetch a constant. Every N minutes (where N defaults to 5) it would have a cache miss and have to hit the disk again.

Approach 2 hits the disk for each request (the Heroku dynos most likely aren't great for disk I/O)

Approach 3 means keeping the key permanently in memory (but given it's tiny I don't think it's even worth worrying about)

Approach 3 was what I was meaning above (I probably could have been clearer - sorry!) - but open to ideas? :-)

Thanks for elaborating :) Approach 3 sounds great to me! I pushed a commit for that.

edmorley · 2018-01-24T20:38:14Z

package.json

    "deepmerge": "1.5.2",
    "eslint-plugin-react": "7.5.1",
    "font-awesome": "4.7.0",
+    "got": "8.0.3",


What are you thoughts on:
https://npmcompare.com/compare/axios,fetch,got,request,superagent

...particularly for consumers that only need to support recent browsers (eg our babel config only specifies "last 1 version" of the main 4).

That said, it's only used in the function that might be removed, so we might not need to use any library after all :-)

@edmorley In that case, I could just rely on https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API. No need to bring in a new library :)

edmorley · 2018-01-24T20:43:21Z

package.json

    "redux-debounce": "1.0.1",
-    "taskcluster-client": "2.5.4"
+    "taskcluster-client": "2.5.4",
+    "taskcluster-client-web": "^5.0.1"


Could you make this a fixed version rather than a tilde range? (Renovate bumps the versions for us, so we don't use tilde ranges. The .yarnrc in the repo root is configured with save-exact true - did you use npm/add manually instead?).

I used yarn add taskcluster-client-web.

What version of yarn? (save-exact support was added in 1.1.0, in yarnpkg/yarn#4471)

edmorley · 2018-01-24T20:44:50Z

neutrino-custom/base.js

        .end();
+    neutrino.config
+        .entry('login')
+        .add(path.join(UI, 'entry-login.js'))


Could you rename the file to .jsx? (#3163 will soon make ESLint check for this)

helfi92 · 2018-01-24T20:59:35Z

I can't seem to comment on #3144 (comment)...

I don't think you need this anymore? It's generating credentials that are only good for a few minutes anyway..

Sure, I can remove it but I will need to create a custom event to update the thTaskcluster factory to include the updated client credentials. Something like:

// AuthService.js
window.dispatchEvent(
    new CustomEvent('taskcluster-credentials', { detail: taskclusterCredentials });
);

Then in taskcluster.js, update the client:

// taskcluster.js
$window.addEventListener("taskcluster-credentials", function ({ detail: credentials }) {
    client.config({ credentals: credentials || {} });
});

I'll send a commit for that.

helfi92 · 2018-01-25T13:24:03Z

@edmorley Any idea how I could remove the warning error:

WARNINGS:
?: (security.W019) You have 'django.middleware.clickjacking.XFrameOptionsMiddleware' in 
your MIDDLEWARE_CLASSES, but X_FRAME_OPTIONS is not set to 'DENY'.
The default is 'SAMEORIGIN', but unless there is a good reason for your site to
serve other parts of itself in a frame, you should change it to 'DENY'.

X_FRAME_OPTIONS is set to SAMEORIGIN because token renewal happens in an invisible iframe.

Should I just remove the middleware?

edmorley · 2018-01-25T13:50:09Z

We need the middleware, otherwise the X-Frame-Options header wouldn't be set at all (which is different to being set to SAMEORIGIN), which would reduce our score on https://observatory.mozilla.org/analyze.html?host=treeherder.mozilla.org (there were mentions a while back that all Mozilla sites had to get at least a 'B') - and presumably allow certain attack scenarios (though have likely they are I don't know).

However I agree that X_FRAME_OPTIONS = 'SAMEORIGIN' is necessary here/safe for us (and the HTTP observatory is fine with it too) - so lets silence that Django check warning by adding it to SILENCED_SYSTEM_CHECKS, here:

treeherder/treeherder/config/settings.py

Lines 465 to 469 in 247e1b9

    
           SILENCED_SYSTEM_CHECKS = [ 
        
               # We can't set CSRF_COOKIE_HTTPONLY to True since the requests to the API 
        
               # made using Angular's `httpProvider` require access to the cookie. 
        
               'security.W017', 
        
           ]

edmorley · 2018-01-25T22:29:34Z

Many thanks for rebasing! This is now deployed to prototype (I had to push as a new branch on the upstream repo, since Heroku can't deploy from forks that aren't connected to the account):
https://treeherder-prototype.herokuapp.com/

I think I've found one small bug - STR:

Open the URL above and log in
Open a new tab to the same URL

-> In the second tab the login link top right has reverted to "login/register" (side note: I wonder if we should get rid of the pre-existing word "register"?) rather than showing me logged in. Returning to the 1st tab shows that to be logged out now too.

helfi92 · 2018-01-25T23:15:36Z

@edmorley Weird, I can't seem to reproduce. Could you try clearing your cache and trying again?

edmorley · 2018-01-25T23:19:46Z

Reproduces with a clean cache. Once it didn't reproduce, but 3-4 other times it has (the time it didn't I'd left it longer before opening the new tab).

helfi92 · 2018-01-25T23:54:04Z

@djmitche Could you check #3144 (comment) and see if you can reproduce? I can't seem to replicate on my machine.

djmitche · 2018-01-26T16:51:01Z

I can't seem to reproduce. I opened a dozen or so tabs with the same URL. When they first load, it says "Login/Register", but as the page fills out that changes to my name.

Reason explained in the comment

It turns out that window.dispatchEvent or window.postMessage don't work properly when the window sending the message has been refreshed or redirected. Therefore, it is not really reliable. localStorage ftw!

Previously, whenever perfherder added something to localStorage, angular-local-storage added a default 'ls' prefix to each entry, making things messy. I noticed that some entries were even duplicated. For example, if you login in treeherder.mozilla.org/ and then open a new tab at treeherder.mozilla.org/perf.html, then you will find the following duplicated entries: * (ls, treeherder).user * (ls, treehderr).taskcluster.credentials This commit removes the duplicated entries through the removal of the prefixes. In the long term, I see angular-local-storage being completely removed and simply rely on the browser of localStorage. This should fix the issue of not being able to login with /perf.html. I previously did not take that ls prefix into consideration.

Previously, the way to update tc credentials is when there is renewal or when a user logs in. This commit will update tc credentials on page load in case something change (e.g., user's scopes)

Previously, taskcluster-client-web was used to get taskcluster credentials but taskcluster-client was then used to setup the client. I believe this was causing some issues with expiration since the agent creates credentials that are only good for a couple of minutes. To fix this issue, this commit uses taskcluster-client-web to handle everything. This commit brings other benefits, namely: - taskcluster-client is no longer a dependency. - thTaskcluster is now written in plain JS. This should drive the transition from angular to react in the right direction.

Also, removed ctrl.userLoggingIn since it's redundant. Login callback is on a different page now.

edmorley · 2018-02-07T10:24:08Z

Well done on figuring out the last bug! Is this now ready to have the sheriffs take another look?

To prevent pyup/Renovate PRs being opened as soon as this lands. (Which would also then cause conflicts if they landed and this PR later needed reverting.) Changes: https://github.com/Legrandin/pycryptodome/blob/master/Changelog.rst https://github.com/auth0/auth0.js/blob/master/CHANGELOG.md taskcluster/taskcluster-client-web@v5.0.1...v5.1.0

helfi92 · 2018-02-07T13:35:20Z

I sent an email yesterday to the sheriffs but forgot to cc you. Just did now :)

We no longer use Hawk directly, but had kept it as a dependency to work around Neutrino 4's broken module resolution: #3144 (comment)

Since the name/description references the pre-auth0 implementation that was removed in #3144. The `test_user` fixture has also been removed, since it is not required for the test to run (the error referenced in the comment no longer occurs).

helfi92 self-assigned this Jan 17, 2018

helfi92 force-pushed the th-auth0 branch 4 times, most recently from 68c466b to f623f12 Compare January 18, 2018 14:54

helfi92 requested review from djmitche and edmorley January 18, 2018 15:10

edmorley suggested changes Jan 18, 2018

View reviewed changes

djmitche suggested changes Jan 18, 2018

View reviewed changes

helfi92 force-pushed the th-auth0 branch 2 times, most recently from c292198 to 3132307 Compare January 23, 2018 17:21

helfi92 requested review from djmitche and edmorley January 23, 2018 17:26

djmitche suggested changes Jan 23, 2018

View reviewed changes

djmitche approved these changes Jan 24, 2018

View reviewed changes

treeherder/auth/backends.py

"use": key["use"],

"n": key["n"],

"e": key["e"]

}

Copy link
Copy Markdown

Contributor

djmitche Jan 24, 2018

Choose a reason for hiding this comment

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

OK, this is much better now :)

edmorley reviewed Jan 24, 2018

View reviewed changes

helfi92 requested a review from edmorley January 25, 2018 14:23

helfi92 force-pushed the th-auth0 branch 2 times, most recently from 5553119 to 1213774 Compare January 25, 2018 20:47

edmorley temporarily deployed to treeherder-prototype January 25, 2018 22:10 Inactive

helfi92 added 17 commits February 6, 2018 13:34

Fix tests

20394d1

Silence X_FRAME_OPTIONS check

6ade55f

Reason explained in the comment

Remove linting issues

7c5841a

Update backend packages

7dbf624

Remove trailing whitespace and use trailing comma

58eab52

Remove extra 'None' from default get()

6cf144d

Switch indentation of JS & JSON to 2 spaces (new files)

82f5579

Close possibility of 'open' file descriptor

91abfc8

Add pycryptodome python wheel hash

33a8c5f

Add css to loading text, add comments, remove 'use strict'

3a6283c

Remove tilde from auth0-js package.json version

3a3dc9d

Rely on localStorage for treeherder.taskcluster.credentials

8be7f85

It turns out that window.dispatchEvent or window.postMessage don't work properly when the window sending the message has been refreshed or redirected. Therefore, it is not really reliable. localStorage ftw!

Update tc credentials on page load if logged in

18f537f

Previously, the way to update tc credentials is when there is renewal or when a user logs in. This commit will update tc credentials on page load in case something change (e.g., user's scopes)

Remove unwanted error message in thTaskclusterErrors

cd2c225

Fix tc credentials issue

6f675a5

Also, removed ctrl.userLoggingIn since it's redundant. Login callback is on a different page now.

helfi92 force-pushed the th-auth0 branch from c419add to 6f675a5 Compare February 6, 2018 18:39

helfi92 temporarily deployed to treeherder-prototype February 6, 2018 18:39 Inactive

edmorley added 2 commits February 7, 2018 11:55

Fix yarn.lock merge

1a7a2c5

helfi92 merged commit e1b0168 into mozilla:master Feb 7, 2018

edmorley mentioned this pull request Feb 10, 2018

support AJAX-y sites mozilla/mozilla-django-oidc#133

Closed

edmorley added a commit that referenced this pull request May 29, 2018

Bug 1465041 - Remove direct hawk dependency workaround

e7fab09

We no longer use Hawk directly, but had kept it as a dependency to work around Neutrino 4's broken module resolution: #3144 (comment)

edmorley mentioned this pull request May 29, 2018

Bug 1465041 - Restore default webpack module resolution config #3589

Merged

edmorley added a commit that referenced this pull request May 29, 2018

Bug 1465041 - Remove direct hawk dependency workaround

af892c7

We no longer use Hawk directly, but had kept it as a dependency to work around Neutrino 4's broken module resolution: #3144 (comment)


		const authApp = angular.module('auth', ['LocalStorageModule', 'auth0.auth0', 'react']);

		authApp.config(['angularAuth0Provider', 'localStorageServiceProvider', '$locationProvider',

Conversation

helfi92 commented Jan 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rough summary of the changes

Front end

Credential expiration

Credential renewal

Migration

Uh oh!

helfi92 commented Jan 18, 2018

Uh oh!

edmorley left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edmorley Jan 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edmorley Jan 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

helfi92 Jan 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edmorley Jan 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edmorley Jan 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

djmitche left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

helfi92 commented Jan 17, 2018 •

edited

Loading

edmorley Jan 18, 2018 •

edited

Loading

edmorley Jan 18, 2018 •

edited

Loading

helfi92 Jan 24, 2018 •

edited

Loading

edmorley Jan 18, 2018 •

edited

Loading

edmorley Jan 18, 2018 •

edited

Loading