Skip to content

Comments

[SQSERVICES-1770] Register OAuth App (1/n)#2882

Closed
battermann wants to merge 10 commits intodevelopfrom
SQSERVICES-1770-be-oatuh-internal-endpoint-to-register-o-auth-app
Closed

[SQSERVICES-1770] Register OAuth App (1/n)#2882
battermann wants to merge 10 commits intodevelopfrom
SQSERVICES-1770-be-oatuh-internal-endpoint-to-register-o-auth-app

Conversation

@battermann
Copy link
Contributor

@battermann battermann commented Nov 29, 2022

https://wearezeta.atlassian.net/browse/SQSERVICES-1770

Todos

  • Everything is implemented in a single new module. We should refactor this and extract common generic behavior that could be a standalone OAuth2 library and put the rest into places where it makes more sense, like e.g. wire-api. However, refactoring should probably be done in a single branch that contains all the subsequent OAuth PRs. Also, maybe define new effects for secret generation?

Checklist

  • Add a new entry in an appropriate subdirectory of changelog.d
  • Read and follow the PR guidelines
  • Run make git-add-cassandra-schema to update the cassandra schema documentation

@battermann battermann temporarily deployed to cachix November 29, 2022 16:19 Inactive
@battermann battermann temporarily deployed to cachix November 29, 2022 16:19 Inactive
@battermann battermann temporarily deployed to cachix November 30, 2022 13:02 Inactive
@battermann battermann temporarily deployed to cachix November 30, 2022 13:02 Inactive
@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Nov 30, 2022
@battermann battermann temporarily deployed to cachix November 30, 2022 14:32 Inactive
@battermann battermann temporarily deployed to cachix November 30, 2022 14:32 Inactive
@battermann battermann temporarily deployed to cachix November 30, 2022 15:11 Inactive
@battermann battermann temporarily deployed to cachix November 30, 2022 15:11 Inactive
@battermann battermann temporarily deployed to cachix November 30, 2022 15:34 Inactive
@battermann battermann temporarily deployed to cachix November 30, 2022 15:34 Inactive
@battermann battermann requested review from fisx and jschaul November 30, 2022 15:39
@battermann battermann marked this pull request as ready for review November 30, 2022 15:39
@battermann battermann temporarily deployed to cachix November 30, 2022 15:45 Inactive
@battermann battermann temporarily deployed to cachix November 30, 2022 15:45 Inactive
@battermann battermann temporarily deployed to cachix December 1, 2022 09:44 Inactive
@battermann battermann temporarily deployed to cachix December 1, 2022 09:44 Inactive
@battermann battermann temporarily deployed to cachix December 2, 2022 09:47 Inactive
@battermann battermann temporarily deployed to cachix December 2, 2022 09:47 Inactive
@battermann battermann force-pushed the SQSERVICES-1770-be-oatuh-internal-endpoint-to-register-o-auth-app branch from 390921e to b897080 Compare December 5, 2022 16:14
@battermann battermann temporarily deployed to cachix December 5, 2022 16:14 Inactive
@battermann battermann temporarily deployed to cachix December 5, 2022 16:15 Inactive
@battermann battermann changed the title [SQSERVICES-1770] Register OAuth App [SQSERVICES-1770] Register OAuth App (1/n) Dec 8, 2022
@battermann battermann temporarily deployed to cachix December 8, 2022 11:34 — with GitHub Actions Inactive
@battermann battermann temporarily deployed to cachix December 8, 2022 11:34 — with GitHub Actions Inactive
@battermann battermann force-pushed the SQSERVICES-1770-be-oatuh-internal-endpoint-to-register-o-auth-app branch from 468d7f6 to 942f1fa Compare December 22, 2022 15:30
@battermann battermann temporarily deployed to cachix December 22, 2022 15:30 — with GitHub Actions Inactive
@battermann battermann temporarily deployed to cachix December 22, 2022 15:30 — with GitHub Actions Inactive
@battermann battermann temporarily deployed to cachix December 22, 2022 15:44 — with GitHub Actions Inactive
@battermann battermann temporarily deployed to cachix December 22, 2022 15:44 — with GitHub Actions Inactive
envs:
- all
disable_zauth: true
- path: /oauth/clients/([^/]*)$
Copy link
Contributor

Choose a reason for hiding this comment

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

probably should add that part to the changelog entry as well.

import Test.QuickCheck.Instances ()

data IdTag = A | C | I | U | P | S | T | STo
data IdTag = A | C | I | U | P | S | T | STo | OAuthClient
Copy link
Contributor

Choose a reason for hiding this comment

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

i like how the names get exponentially longer over time. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

is there a reason you're not just using a new type OAuthClientId. it's fine to reuse Id, just wondering.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Id provides us with a lot of helpful utility functions, that's all.

CREATE TABLE IF NOT EXISTS oauth_client
( id uuid PRIMARY KEY
, name text
, redirect_uri blob
Copy link
Contributor

Choose a reason for hiding this comment

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

text? (same for secrect)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe, I did this according to other table columns with the same Haskell types.

Copy link
Contributor

Choose a reason for hiding this comment

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

then we should probably keep it as is for consistency.

]
r =>
ServerT (BrigIRoutes.API :<|> IOAuthAPI) (Handler r)
servantSitemap = brigInternalAPI :<|> internalOauthAPI
Copy link
Contributor

Choose a reason for hiding this comment

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

why not add internalOAuthAPI to the list in brigInternalAPI? i would argue it's just one more thing next to mls, teams, user, auth, ...

same for public api.

:<|> Team.servantAPI
:<|> systemSettingsAPI
ServerT (BrigAPI :<|> OAuthAPI) (Handler r)
servantSitemap = brigAPI :<|> oauthAPI
Copy link
Contributor

Choose a reason for hiding this comment

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

see comment for internal api above.

versionApi = API.Version.tests mg brigOpts b
mlsApi = MLS.tests mg b brigOpts

let oauthAPI = API.OAuth.tests mg b brigOpts
Copy link
Contributor

Choose a reason for hiding this comment

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

add this line to the let block above?

@fisx fisx mentioned this pull request Jan 16, 2023
2 tasks
@battermann
Copy link
Contributor Author

battermann commented Mar 9, 2023

Obsolete because of #2989

@battermann battermann closed this Mar 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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.

3 participants