This is more consistent with the `uses_omniauth?` we already have, and
it also will help for the next change, where I want a `uses_password?`
method (and using the name `password?` breaks some of Devise's
validation code).
Ahh right, in development `User` and `AuthUser` will have the same ID,
but that got messed up early on for us in production DTI 😅
Here, we switch the form to reference the `User` instead of the
`AuthUser` (to get the ID right), then we also change how we compare
the IDs, because `User#to_param` appends extra text onto the ID after
the number!
Motivation is that I wanna add NeoPass stuff to here! But also like,
it's looked bad for a long time, let's clean it up!! (I just used the
Devise default without any styling at all lol)
Huh, I was writing up an API inventory doc to send to TNT, and was
gonna explain why we proxy these assets… but turns out we don't need to
anymore! Nice!
This is a bit fragile if they ever change their headers, so I'll
mention that in the doc, but for now, yeah sure let's save the planet
some computational effort!
Previously, the way we loaded the image hash for a given pet was to
navigate to `https://pets.neopets.com/cpn/<pet_name>/1/1.png`, but
*not* follow the redirect, and extract the image hash from the URL
where it redirected us to.
In this change, we refactor to use the AMFPHP RPC `PetService.getPet`
instead. I don't think it had this data last time I looked at it, but
now it does! Much prefer to use an actual RPC than our weird hacky
thing!
(We might also be able to use this call for other stuff, like
auto-labeling gender & mood for pet states, maybe?? That's in this data
too! We used to load petlookups for this, long long ago, before the
petlookup captchas got added.)
I guess this was like, we had some call site that was handling loading
the viewer data itself, and didn't want to have to reload it?
But whatever, not used now, let's simplify! We can rebuild this easily
if we need it again.
Locale is the big one that's not really relevant anymore (I don't want
to be loading non-English item names anymore, now that we've simplified
to only support English like TNT has!), but there was also `item_scope`
and stuff.
The timeout option is technically not used in any call sites, but I
think that one's useful to leave around; timeout stuff is important,
and I don't want to rewrite it sometime if we need it again!
Just a small thing, I guess when I was a kid I did a weird thing where
I attached `origin_pet` to `PetType`, then upon saving `PetType` I
loaded the image hash for the pet to save as the pet type's new image
hash.
I guess this does have the nice property of not bothering to load that
stuff until we need it? But whatever, I'm moving this into `Pet` both
to simplify the relationship between the models, and to prepare for
another potential refactor: using `PetService.getPet` for this instead!
Ahh, I had assumed the `uid` provided by NeoPass would be the user's
Neopets username, but in hindsight that was never gonna work out since
NeoPass doesn't think of things in terms of usernames at all!
For now, we create 100% random NeoPass usernames, of the form
"neopass-shoyru-5812" or similar. This will be an important fallback
anyway, because it's possible to have a NeoPass with *no* Neopets.com
account attached.
But hopefully we'll be able to work with TNT to request the user's main
Neopets account's username somehow, to use that as the default when
possible!
Hacky and inconvenient, but it works!
I want this primarily to enable me to live-debug what info we're
getting back in the auth token. In production right now, the flow with
NeoPass succeeds, but we fail to create the account, and my production
error logs say it's because the username field is too long. I had hoped
it would just be the Neopets username, but now that I've poked at
NeoPass itself a bit, I'm realizing it won't be that simple.
So, we'll use this to investigate!
Ahh okay, the NeoPass spec says to pass the `client_id` and
`client_secret` as POST form data when exchanging the code for the
access token, but the default behavior of our client is to pass it as
an `Authorization` header instead.
In this change, we set an option to change that behavior, and also add
a lot of comments about this and the other options!
Wowie, it's starting to happen! :3
When you run this in production, though, you get back the auth failure
message, and the OmniAuth logs say the server returned the following:
> invalid_client: Client authentication failed (e.g., unknown client,
> no client authentication included, or unsupported authentication
> method). The OAuth 2.0 Client supports client authentication method
> 'client_secret_post', but method 'client_secret_basic' was requested.
> You must configure the OAuth 2.0 client's
> 'token_endpoint_auth_method' value to accept 'client_secret_basic'.
I'll add a fix for this in the next commit, with some explanations as
to why!
Ah beans, I didn't notice when doing my Turbo fixes in
40804c1543, that I had accidentally
prevented Chakra from applying some of its usual global styles! This
caused some minor visual regressions in various parts of the app, e.g.,
the default border color for the search field in the wardrobe app
became way darker.
Here, instead of copy-pasting the styles and missing some parts, we
scope the global styles a bit more carefully: we first use
`extendTheme` with the same code as Impress 2020 to get a good
`globalStyles` function that includes Chakra's defaults, *then* delete
the key from the theme.
Then, in `ScopedCSSReset`, we use code similar to Chakra's own global
style application code: call the `globalStyles` function with the
current theme and color mode, use Chakra's `css` function to convert
values like `green.800` to CSS values, prepend our scoping rule onto
the selectors, and drop it into our Emotion CSS.
Tbh I was worried because when I first noticed this issue while on my
trip, I saw the black item search box border, and was like "ah dang,
did I destroy all the color in the app by breaking the part where
Chakra defines its CSS color variables??" And no, that's not actually
what happened, a lot of the app still had its colors!
So this was less pressing than I had thought, but still good to get
fixed!
Commit 07617fa34f was an emergency commit
while I was on a trip, away from my usual workstation! I was able to
write the migration and run it against production, but I didn't have
the dev server fully set up, so I wasn't able to run the migration in
development, which is when Rails updates `schema.rb`.
Now I'm home and can run it, easy peasy! `rails db:migrate`
I don't link to this in the footer or anything yet, but TNT asked for
it as part of the NeoPass setup, so I currently just redirect to the
outdated Impress 2020 privacy policy. (It's outdated in the sense that
we share *less* data with our third party services now, e.g. we moved
our analytics and error tracking onto our own machines!)
Ahh wow, we're finally at the point of having enough outfits to need to
increase this ID field!
I got an alert this morning that queries were failing with the MySQL error
"id out of range". I went and found that creating a new outfit still works if
it's empty, but fails once you try to add an item to it. So, I assume this is
the failing column!
Note that I'm writing this from my emergency Steam Deck workstation, and that
I didn't quite get MySQL set up on it yet (I just yolo ran this in
production), so we don't have the corresponding changes to `db/schema.rb` yet.
This might require another commit from my normal workstation later to
complete this change!
Ah right, I went and checked the Devise source code, and the default
implementation for `password_required?` is a bit trickier than I
expected:
```ruby
def password_required?
!persisted? || !password.nil? || !password_confirmation.nil?
end
```
Looks like `super` does a good enough job here, though! (I'm actually
kinda surprised, I wasn't sure how Ruby's `super` rules worked, and
this isn't a subclass thing—or maybe it is, maybe the `devise` method
adds a mixin? Idk! But it does what I expect, so, great!)
So now, we require the password if 1) Devise doesn't see a UI reason
not to, *and* 2) the user isn't using OmniAuth (i.e. NeoPass).
This had caused a bug where it was impossible to use the Settings page
*without* changing your password! (The form says it's okay to leave it
blank, which stopped being true! But now it's fixed!)
Whew, exciting! Still done nothing against the live NeoPass server, but
we've got this fully working with the development server, it seems!
Wowie!!
This is all still hidden behind secret flags, so it's fine to deploy
live. (And it's not actually a problem if someone gets past to the
endpoints behind it, because we haven't actually set up real
credentials for our NeoPass client yet, so authentication will fail!)
Okay time to lie down lol.
Okay, `sub` seems to be a pretty standard place for user identifiers.
Let's start with that assumption! I override the `oauth2-mock-server`'s
default of `johndoe` with `theneopetsteam`, just to be cute :3
Right, I didn't totally connect the dots that there's some OpenID
features in the mix here for how we expect to identify the user once
they authenticate. It requires looking up the provider's public key,
and validating the JWT they sent us. This gem does all that for us!
I don't actually know what a real NeoPass `id_token` looks like yet?
But I'll fill in some placeholder stuff for now, and use that for
initializing the account!
I'm starting to work with the OpenID Connect stuff in NeoPass, and the
library I'm using for discovery doesn't seem to want to do it over a
plain `http://` connection. (I dug into the source files, and it just
actually is hardcoded to only work with HTTPS, as far as I can tell?)
So, I've added logic to `neopass-server` to try to make an HTTPS
certificate with the `mkcert` utility (if installed), which is a tool
that installs a root certificate authority on your local machine, then
helps you create certificates via that authority that will work only on
your local machine.
I think I'll also be able to remove the "main" server in front of the
backing server, because the library I'm using now will be able to
"discover" the auth and token endpoints, so it won't matter that our
local one uses different URLs than live NeoPass does? We'll find out!
I also remove `neopass-server` from the `Procfile`, because I think
it's a bit rude to have it auto-try to run `mkcert`. We could like,
make the process just a no-op in that case? But I think I'd prefer to
just run `neopass-server` by hand when I want it, for simplicity.
Hey nice, now we can actually get a round-trip auth success! This gets
us to the `Devise::OmniauthCallbacksController#neopass` success method,
so now we need to add our logic to actually login/signup!
In this change, we wire up a new NeoPass OAuth2 strategy for OmniAuth,
and hook up the "Log in with NeoPass" button to use it!
The authentication currently fails with `invalid_credentials`, and
shows the `owo` response we hardcoded into the NeoPass server's token
response. We need to finally follow up on the little `TODO` written in
there!
If you pass `?neopass=1` (or a secret value in production), you can see
the "Log in with NeoPass" button, which currently takes you to
OmniAuth's "developer" login page, where you can specify a name and
email and be redirected back. (All placeholder UI!)
We're gonna strip the whole developer strategy out pretty fast and
replace it with one that uses our NeoPass test server. This is just me
checking my understanding of the wiring!
This is setting us up for NeoPass, but first we're just gonna try stuff
with the "developer" strategy that's built in for testing, rather than
using the NeoPass dev server!
Oh right, all these cute overrides should be scoped to the page!
I guess we skipped this because we had pulled this out into a
separate stylesheet. Curiously learning more about how Turbo handles
this kind of thing, like that it doesn't *unload* stylesheets that
*leave* the page when you navigate!
I noticed an issue where Turbo-loading between the Your Items page and
the homepage would clobber each other's copy of jQuery, breaking things
sometimes. e.g. go to Your Items, then go to home, then go to Your
Items, and the page's JS fails because `$.fn.live` isn't defined.
I briefly tested the homepage and it didn't seem to actually depend on
any features from the later version of jQuery? At least not that I
noticed! So I'll just downgrade for consistency. (I also tried
upgrading the Your Items page, but there's too much usage of
`$.fn.live`, which is replaced with a notably different syntax in
jQuery 2.0+.)
First one, Turbo reasonably yelled at us in the JS console that we
should put its script tag in the `head` rather than the `body`, because
it re-executes scripts in the `body` and we don't want to spin up Turbo
multiple times!
I also removed some scripts that aren't relevant anymore, fixed a bug
in `outfits/new.js` where failing to load a donation pet would cause
the preview thing to not work when you type (I think this might've
already been an issue?), reworked `item_header.js` to just run once in
the `head`, and split scripts into `:javascripts` (run once in `head`)
vs `:javascripts_body` (run every page load in `body`).
Finally getting around to this because, with Turbo in play, it applies
to subsequent *pages* too, oops! Previously we at least had the blast
radius of this known issue constrained to the item page itself lol :p
Got some questions in Discord about account unlinking, and seeing
people look ahead to other potential integrations. Want to clarify that
unlinking will work here (barring any surprises!), and that there's no
data sharing _just_ yet!
Turbo expects this to exist, for features we don't use! I didn't bother
to check if Turbo has a way to turn this off too, I just said fine lol
Turbo worked in development without this because it only loads files as
needed, whereas in production it blocked the app from starting up. But
you can see the error in development by running `rails zeitwerk:check`,
which attempts to preload everything to make sure it all works, and you
get this:
```
NameError: uninitialized constant ActionCable (NameError)
class Turbo::StreamsChannel < ActionCable::Channel::Base
```
Fixed now!
Someone requested this in Discord, and I figured why not! I'm still
planning to move stuff away from Impress 2020 over time, I just figure
may as well have them more linked while this is still The Reality
This doesn't really matter, I just didn't realize the `.html` part was
optional, and I guess I omitted it here without realizing? But let's
add it for consistency.
The modeling code is slow! I think in production it's being cached, and
tbh I though I had development mode caching turned on over here, but
it's quite evidently _not_ doing it if so, so. Okay! Skip for now.
