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

User subsystems: update identity #4172

Draft
wants to merge 12 commits into
base: develop
Choose a base branch
from

Conversation

fisx
Copy link
Contributor

@fisx fisx commented Jul 26, 2024

WPB-8881

Checklist

  • Add a new entry in an appropriate subdirectory of changelog.d
  • Read and follow the PR guidelines

@echoes-hq echoes-hq bot added the echoes: technical-roadmap/throughput Changes intended at preserving our ability to evolve the software safely and effectively label Jul 26, 2024
@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Jul 26, 2024
@fisx fisx force-pushed the WPB-8881/user-subsystem-update-identity branch from 963936a to a610321 Compare July 26, 2024 19:17
@fisx
Copy link
Contributor Author

fisx commented Jul 26, 2024

a610321 is interesting:

validateTokens ::
  (ZAuth.TokenPair u a) =>
  List1 (ZAuth.Token u) ->
  Maybe (ZAuth.Token a) ->
  ExceptT ZAuth.Failure (AppT r) (UserId, Cookie (ZAuth.Token u))
validateTokens uts at = do
  tokens <- forM uts $ \ut -> lift $ runExceptT (validateToken ut at)
  getFirstSuccessOrFirstFail tokens
  where
    -- FUTUREWORK: There is surely a better way to do this
    getFirstSuccessOrFirstFail ::
      (Monad m) =>
      List1 (Either ZAuth.Failure (UserId, Cookie (ZAuth.Token u))) ->
      ExceptT ZAuth.Failure m (UserId, Cookie (ZAuth.Token u))
    getFirstSuccessOrFirstFail tks = case (lefts $ NE.toList $ List1.toNonEmpty tks, rights $ NE.toList $ List1.toNonEmpty tks) of
      (_, suc : _) -> pure suc
      (e : _, _) -> throwE e
      _ -> throwE ZAuth.Invalid -- Impossible

turned into this:

validateTokens ::
  forall u a v.
  (v ~ (UserId, Cookie (ZAuth.Token u))) =>
  (ZAuthWrapper.TokenPair u a) =>
  List1 (ZAuth.Token u) ->
  Maybe (ZAuth.Token a) ->
  Either ZAuthWrapper.Failure v
validateTokens uts at = ((`validateToken` at) `mapM` uts) & second List1.head

but

@fisx
Copy link
Contributor Author

fisx commented Jul 26, 2024

also i'm a little proud of my commit discipline today! :)

@fisx fisx force-pushed the WPB-8881/user-subsystem-update-identity branch 4 times, most recently from c65ec85 to 9168390 Compare August 5, 2024 13:54
@akshaymankar akshaymankar force-pushed the WPB-8881/user-subsystem-update-identity branch from 9168390 to 1201c40 Compare August 6, 2024 08:01
--
-- TODO: Make this a proper subsystem and use polysemy for error handling rather than either?
-- TODO: put every instance X of Authorize into a module Wire.Authorize.*. all those modules should re-export authorize.
data Authorized op val = Authorized {runAuthorized :: val}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should this be called AuthorizedFor?

-- You should have received a copy of the GNU Affero General Public License along
-- with this program. If not, see <https://www.gnu.org/licenses/>.

module Wire.Authorize
Copy link
Contributor Author

Choose a reason for hiding this comment

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

seems like all the extra authorization logic in changing email addresses is redunandant. we could just check for the z-user header, no? (thanks @akshaymankar)

| -- | The key/code was valid but already recently activated.
ActivationPass

-- | Outcome of an email address the procedure.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
-- | Outcome of an email address the procedure.
-- | Outcome of an email address activation.

@pcapriotti pcapriotti force-pushed the WPB-8881/user-subsystem-update-identity branch from ec93eab to 64f5a37 Compare October 4, 2024 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
echoes: technical-roadmap/throughput Changes intended at preserving our ability to evolve the software safely and effectively ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants