Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add atomic callbacks for changes/validations throughout AshAuthentication #677

Open
zachdaniel opened this issue May 12, 2024 · 3 comments

Comments

@zachdaniel
Copy link
Collaborator

Let's review the various changes and validations in AshAuthentication to give them atomic callbacks. In some cases, the atomic callbacks may be not doable because of security or not worthwhile to be done atomically. Thats not an issue, but we should document those cases/discuss them.

@jimsynz
Copy link
Collaborator

jimsynz commented May 14, 2024

Changes:

  1. AshAuthentication.GenerateTokenChange - generates a JWT, stores it in the token resource and returns it in the metadata.
  2. AshAuthentication.AddOn.Confirmation.ConfirmChange - verifies a JWT and then performs arbitrary changes to the user resource.
  3. AshAuthentication.AddOn.Confirmation.ConfirmationHookChange - inhibits changes to the user resource and generates a JWT (with side effect of writing to the token resource).
  4. AshAuthentication.Strategy.OAuth2.IdentityChange - upserts a record in the user identity resource as a side effect of user creation.
  5. AshAuthentication.Strategy.Password.HashPasswordChange - performs password hashing and updates the hashed_password attribute. Theoretically possible to perform password hashing in the datalayer but pgcrypt doesn't support any modern password hashing functions.
  6. AshAuthentication.TokenResource.RevokeTokenChange, AshAuthentication.TokenResource.StoreConfirmationChangesChange, AshAuthentication.TokenResource.StoreTokenChange - all three of these decode a token and use it to upsert a record in the token resource. They may be a good refactoring opportunity.
  7. AshAuthentication.UserIdentity.UpsertIdentityChange - decodes a "token info" OAuth2 response and uses it to upsert a record in the user identity resource.

Validations:

  1. AshAuthentication.Strategy.Password.PasswordConfirmationValidation - can likely be made atomic - it just checks that one attribute equals another.
  2. AshAuthentication.Strategy.Password.PasswordValidation - takes a password argument, hashes it and checks it against the hashed password field. See my thoughts on AshAuthentication.Strategy.Password.HashPasswordChange for more.
  3. AshAuthentication.Strategy.Password.ResetTokenValidation - decodes a token and verifies it against the strategy's resettable configuration.

@zachdaniel
Copy link
Collaborator Author

zachdaniel commented May 14, 2024

Awesome 🔥

  1. can be made atomic by adding after_batch
  2. I think could theoretically be made atomic, but not entirely convinced it's worth it. Might be a good exercise
  3. same as # 2
  4. Same as # 1
  5. This one would be a good candidate for being done with atomics & lazy/1 to hash the password, or using the before_atomic hook that I mention below (which doesn't exist yet). We definitely shouldn't try to encrypt the password itself in the db because then we're transmitting clear text passwords to the db anyway. But since it's a literal value coming in, this is a good candidate for atomics.
  6. makes sense. These are making me feel like we're kind of missing a phase with atomics, specifically before_atomic. Because in all of these cases, we want to do some "do-only-once" type work that depends purely on input and would set attributes atomically. before_atomic would basically just be the exact same as the atomic callback, but we expect that, at this point, any reasons why it can't be atomic will have been sussed out, so no {:not_atomic return value. And then atomic/3 would just return :ok
  7. same as # 2

Validations

  1. Yeah, this one could just be made atomic by making atomic/3 just call validate/3 since it only works on arguments.
  2. Same thought as # 5 above
  3. Same as # 1 here, since its only validating arguments it should be good.

Not expecting anyone to jump on these, but as users are using some of these changes like the HashPasswordChange and PasswordConfirmationValidation etc. in their actions, it would be nice to have the most common ones implemented with atomics, so that those users don't have to add require_atomic? false (and because if an action can be atomic its a net benefit for concurrency safety/performance)

@jimsynz
Copy link
Collaborator

jimsynz commented Sep 12, 2024

Looks like some of this work is being done as part of #782

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants