[gold] Fix logging in w/o google.com addresses
There were 2 issues at play here:
1) We weren't authenticating against the right server (we were
targeting skia.org instead of the correct gold instance). This
meant the wrong list of allowed users/domains was being used.
2) Specifically for gmail addresses, we didn't normalize them
(i.e. remove dots and stuff after the plus sign).
This addresses both of those issues and applies eslint to
login.js (since I was there trying to deduce what went wrong).
Bug: skia:10062
Change-Id: I03ec8232c3891ff26e2987406bcd7a40d928f2d0
Reviewed-on: https://skia-review.googlesource.com/c/buildbot/+/308601
Reviewed-by: Leandro Lovisolo <lovisolo@google.com>
diff --git a/go/login/login.go b/go/login/login.go
index ce4e984..dcb06ec 100644
--- a/go/login/login.go
+++ b/go/login/login.go
@@ -31,7 +31,6 @@
"crypto/sha256"
"encoding/base64"
"encoding/json"
- "errors"
"fmt"
"io"
"io/ioutil"
@@ -295,7 +294,7 @@
func getSession(r *http.Request) (*Session, error) {
cookie, err := r.Cookie(COOKIE_NAME)
if err != nil {
- return nil, err
+ return nil, skerr.Wrap(err)
}
var s Session
if cookie != nil && len(cookie.String()) > 20 {
@@ -306,10 +305,10 @@
}
if err := secureCookie.Decode(COOKIE_NAME, cookie.Value, &s); err != nil {
- return nil, err
+ return nil, skerr.Wrap(err)
}
if s.AuthScope != strings.Join(oauthConfig.Scopes, " ") {
- return nil, fmt.Errorf("Stored auth scope differs from expected (%v vs %s)", oauthConfig.Scopes, s.AuthScope)
+ return nil, skerr.Fmt("Stored auth scope differs from expected (%v vs %s)", oauthConfig.Scopes, s.AuthScope)
}
return &s, nil
}
@@ -319,13 +318,18 @@
func LoggedInAs(r *http.Request) string {
var email string
if s, err := getSession(r); err == nil {
+ sklog.Infof("Is in getSession %#v", s)
email = s.Email
- } else if e, err := ViaBearerToken(r); err == nil {
- email = e
+ } else {
+ sklog.Infof("Could not get email from session %s", err)
+ if e, err := ViaBearerToken(r); err == nil {
+ sklog.Infof("Got email via bearer token %s", e)
+ email = e
+ } else {
+ sklog.Infof("Could not get email from bearer token %s", err)
+ }
}
if isAuthorized(email) {
- // TODO(stephana): Uncomment the following line when Debugf is different from Infof.
- // sklog.Debugf("User %s is on the allowlist", email)
return email
}
@@ -564,15 +568,35 @@
func isAuthorized(email string) bool {
parts := strings.Split(email, "@")
if len(parts) != 2 {
+ sklog.Errorf("Email %s was not in 2 parts: %s", email)
return false
}
+
+ user, domain := parts[0], parts[1]
+ if domain == "gmail.com" {
+ user = normalizeGmailAddress(user)
+ }
+ normalizedEmail := user + "@" + domain
+
if viewAllow != nil {
- return viewAllow.Member(email)
+ return viewAllow.Member(normalizedEmail)
}
- if len(activeUserDomainAllowList) > 0 && !activeUserDomainAllowList[parts[1]] && !activeUserEmailAllowList[email] {
- return false
+
+ if len(activeUserDomainAllowList) == 0 {
+ return true // if the list is empty, everybody is allowed
}
- return true
+ return activeUserDomainAllowList[domain] || activeUserEmailAllowList[normalizedEmail]
+}
+
+// normalizeGmailAddress removes periods and text after a plus sign.
+// See https://stackoverflow.com/a/15499627 for more.
+func normalizeGmailAddress(user string) string {
+ user = strings.ReplaceAll(user, ".", "")
+ plusIdx := strings.Index(user, "+")
+ if plusIdx >= 0 {
+ return user[:plusIdx]
+ }
+ return user
}
// StatusHandler returns the login status of the user as JSON that looks like:
@@ -810,12 +834,12 @@
func ViaBearerToken(r *http.Request) (string, error) {
tok := r.Header.Get("Authorization")
if tok == "" {
- return "", errors.New("User is not authenticated.")
+ return "", skerr.Fmt("User is not authenticated. No Authorization header.")
}
tok = strings.TrimPrefix(tok, "Bearer ")
tokenInfo, err := ValidateBearerToken(tok)
if err != nil {
- return "", err
+ return "", skerr.Wrap(err)
}
return tokenInfo.Email, nil
}
diff --git a/go/login/login_test.go b/go/login/login_test.go
index 7b57708..6095588 100644
--- a/go/login/login_test.go
+++ b/go/login/login_test.go
@@ -147,3 +147,26 @@
assert.False(t, isAuthorized("fred@example.com"))
assert.False(t, isAuthorized("evil@proj.iam.gserviceaccount.com"))
}
+
+func TestIsAuthorized_Gmail(t *testing.T) {
+ unittest.SmallTest(t)
+ once.Do(loginInit)
+ setActiveAllowLists("google.com example@gmail.com")
+
+ assert.True(t, isAuthorized("example@gmail.com"))
+ assert.True(t, isAuthorized("ex.amp.le@gmail.com"))
+ assert.True(t, isAuthorized("example+somethi.ng@gmail.com"))
+ assert.True(t, isAuthorized("ex.amp.le+something@gmail.com"))
+
+ assert.False(t, isAuthorized("fred@gmail.com"))
+ assert.False(t, isAuthorized("example@g.mail.com"))
+}
+
+func TestNormalizeGmailAddress(t *testing.T) {
+ unittest.SmallTest(t)
+ assert.Equal(t, "example", normalizeGmailAddress(".ex.ampl.e."))
+ assert.Equal(t, "example", normalizeGmailAddress("exa.mple"))
+ assert.Equal(t, "example", normalizeGmailAddress("example+"))
+ assert.Equal(t, "example", normalizeGmailAddress("example+decoration+more+plus"))
+ assert.Equal(t, "example", normalizeGmailAddress("examp.le+.dec."))
+}
diff --git a/golden/modules/gold-scaffold-sk/gold-scaffold-sk.js b/golden/modules/gold-scaffold-sk/gold-scaffold-sk.js
index 5e1c1d4..ee3f416 100644
--- a/golden/modules/gold-scaffold-sk/gold-scaffold-sk.js
+++ b/golden/modules/gold-scaffold-sk/gold-scaffold-sk.js
@@ -43,7 +43,7 @@
</div>
<div class=spacer></div>
<!-- TODO(kjlubick) last commit -->
- <login-sk ?testing_offline=${ele.testingOffline}></login-sk>
+ <login-sk ?testing_offline=${ele.testingOffline} .loginHost=${window.location.host}></login-sk>
</header>
<aside>
diff --git a/infra-sk/modules/login-sk/login-sk.js b/infra-sk/modules/login-sk/login-sk.js
index 3835dbb..535c310 100644
--- a/infra-sk/modules/login-sk/login-sk.js
+++ b/infra-sk/modules/login-sk/login-sk.js
@@ -15,29 +15,29 @@
*
* <p>
*/
-import { define } from 'elements-sk/define'
+import { define } from 'elements-sk/define';
import { errorMessage } from 'elements-sk/errorMessage';
import { LoginTo } from '../login';
define('login-sk', class extends HTMLElement {
connectedCallback() {
- this.innerHTML = `<span class=email>Loading...</span><a class=logInOut></a>`;
+ this.innerHTML = '<span class=email>Loading...</span><a class=logInOut></a>';
const host = this.loginHost ? this.loginHost : 'skia.org';
if (this.testingOffline) {
- this.querySelector('.email').textContent = "test@example.com";
+ this.querySelector('.email').textContent = 'test@example.com';
const logInOut = this.querySelector('.logInOut');
- logInOut.href = `https://${host}/logout/?redirect=` + encodeURIComponent(document.location);
+ logInOut.href = `https://${host}/logout/?redirect=${encodeURIComponent(document.location)}`;
logInOut.textContent = 'Logout';
} else {
LoginTo(`https://${host}/loginstatus/`).then((status) => {
this.querySelector('.email').textContent = status.Email;
- let logInOut = this.querySelector('.logInOut');
+ const logInOut = this.querySelector('.logInOut');
if (!status.Email) {
- logInOut.href = status.LoginURL;
- logInOut.textContent = 'Login';
+ logInOut.href = status.LoginURL;
+ logInOut.textContent = 'Login';
} else {
- logInOut.href = `https://${host}/logout/?redirect=` + encodeURIComponent(document.location);
- logInOut.textContent = 'Logout';
+ logInOut.href = `https://${host}/logout/?redirect=${encodeURIComponent(document.location)}`;
+ logInOut.textContent = 'Logout';
}
}).catch(errorMessage);
}
@@ -46,6 +46,7 @@
/** @prop testingOffline {boolean} Reflects the testing_offline attribute for ease of use.
*/
get testingOffline() { return this.hasAttribute('testing_offline'); }
+
set testingOffline(val) {
if (val) {
this.setAttribute('testing_offline', '');
@@ -57,6 +58,7 @@
/** @prop loginHost {string} Which host should be used for login URLs.
*/
get loginHost() { return this.getAttribute('login_host'); }
+
set loginHost(val) {
if (val) {
this.setAttribute('login_host', val);
diff --git a/infra-sk/modules/login.js b/infra-sk/modules/login.js
index b5ea59e..fd6417c 100644
--- a/infra-sk/modules/login.js
+++ b/infra-sk/modules/login.js
@@ -13,13 +13,13 @@
*
* The Email will be the empty string if the user is not logged in.
*/
-export var Login = fetch('https://skia.org/loginstatus/', {
+export const Login = fetch('https://skia.org/loginstatus/', {
credentials: 'include',
-}).then(res => {
+}).then((res) => {
if (res.ok) {
- return res.json()
+ return res.json();
}
- throw new Error('Problem reading /loginstatus/:' + res.statusText);
+ throw new Error(`Problem reading /loginstatus/:${res.statusText}`);
});
/**
@@ -36,14 +36,14 @@
*
* The Email will be the empty string if the user is not logged in.
*/
-export var LoginTo = function(loginStatusURL) {
+export const LoginTo = function(loginStatusURL) {
return fetch(loginStatusURL, {
credentials: 'include',
- }).then(res => {
+ }).then((res) => {
if (res.ok) {
- return res.json()
+ return res.json();
}
- throw new Error('Problem reading /loginstatus/:' + res.statusText);
+ throw new Error(`Problem reading /loginstatus/:${res.statusText}`);
});
};
