Skip to content

Commit e4cb33e

Browse files
committed
Auto merge of #1690 - sgrif:sg-read-only-login, r=jtgeibel
Allow existing users to sign in while READ_ONLY_MODE=1 Currently whenever a user signs in we attempt to update their profile (display name, avatar, email, etc). If we're in read only mode, we can't do that. However, preventing them from signing in entirely seems unfortunate. If we get a read-only error, we will now try to look for an existing user, returning that user if found (or any errors that occurred loading it), and continue to return `ReadOnlyMode` if we couldn't find an existing user. A user will have to sign out and back in again after maintenance has completed if they want to update their information. Ideally we'd display a warning informing them of this fact, but we don't currently have code in place to support that, and with < 3 hours remaining until maintenance time, there's not enough time to write it. I'd also love to add some explicit tests for this, but none of our existing tests handle GitHub oauth, and again, there's not enough time for me to figure out what our recordings need to look like for that. If we're not comfortable shipping this in this imperfect state (no warning, no tests), then we should just close this, and folks won't be able to log in during the maintenance period. That's not the end of the world. However, I've tested this locally to make sure it behaves properly (with #1689 as well to make sure we display the right error), so I'd like to ship this to minimize disruption during the DB maintenance
2 parents 67d178a + 27c5236 commit e4cb33e

File tree

1 file changed

+19
-3
lines changed

1 file changed

+19
-3
lines changed

src/controllers/user/session.rs

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@ use rand::distributions::Alphanumeric;
66
use rand::{thread_rng, Rng};
77

88
use crate::models::{NewUser, User};
9+
use crate::schema::users;
10+
use crate::util::errors::{CargoError, ReadOnlyMode};
911

1012
/// Handles the `GET /authorize_url` route.
1113
///
@@ -111,16 +113,30 @@ struct GithubUser {
111113
}
112114

113115
impl GithubUser {
114-
fn save_to_database(&self, access_token: &str, conn: &PgConnection) -> QueryResult<User> {
115-
Ok(NewUser::new(
116+
fn save_to_database(&self, access_token: &str, conn: &PgConnection) -> CargoResult<User> {
117+
NewUser::new(
116118
self.id,
117119
&self.login,
118120
self.email.as_ref().map(|s| &s[..]),
119121
self.name.as_ref().map(|s| &s[..]),
120122
self.avatar_url.as_ref().map(|s| &s[..]),
121123
access_token,
122124
)
123-
.create_or_update(conn)?)
125+
.create_or_update(conn)
126+
.map_err(Into::into)
127+
.or_else(|e: Box<dyn CargoError>| {
128+
// If we're in read only mode, we can't update their details
129+
// just look for an existing user
130+
if e.is::<ReadOnlyMode>() {
131+
users::table
132+
.filter(users::gh_id.eq(self.id))
133+
.first(conn)
134+
.optional()?
135+
.ok_or(e)
136+
} else {
137+
Err(e)
138+
}
139+
})
124140
}
125141
}
126142

0 commit comments

Comments
 (0)