CBG-801: Auto-generated OIDC callback URL should include provider when non-default #4549

sarathkumarsivan · 2020-04-08T02:54:49Z

No description provided.

…n non-default

coveralls · 2020-04-08T03:04:04Z

Coverage increased (+0.05%) to 61.243% when pulling de66f86 on CBG-801 into 1a92d84 on master.

rest/oidc_api_test.go

rest/oidc_api.go

rest/oidc_api_test.go

rest/oidc_api.go

…n non-default - Added error codes returned by failures to add parameters to callback URL. - Changes to to skip returning an error against empty provider name since it is invalid to have an empty provider name in addCallbackURLQueryParam function. - Removed license headers from rest/oidc_api_test.go - Removed parallel test execution since our our XML parser for Jenkins test results doesn't support parallel tests. - Added changes in TestAddCallbackURLQueryParam to simplify the code if we change inputURL to string and run the URL parsing inside the test loop. - Refactored requestParamProvider to oidcAuthRequestURI; this parameter was named requestParamProvider since it its being used not only in callback URL but also in /{db}/_oidc request, called by clients to initiate the OIDC Authorization Code flow. - Added changes to check for escaped version of provider appended to the callback URL. - Added changes to append ?name=value instead of &name=value to the redirect url when the redirect_uri itself has no query parameters set. It would be best to parse redirect_uri using url.Parse and set a query parameter using the strongly typed Query/Value methods.

sarathkumarsivan

Added error codes returned by failures to add parameters to callback URL.
Changes to to skip returning an error against empty provider name since it is invalid to have an empty provider name in addCallbackURLQueryParam function.
Removed license headers from rest/oidc_api_test.go
Removed parallel test execution since our our XML parser for Jenkins test results doesn't support parallel tests.
Added changes in TestAddCallbackURLQueryParam to simplify the code if we change inputURL to string and run the URL parsing inside the test loop.
Refactored requestParamProvider to oidcAuthRequestURI; this parameter was named requestParamProvider since it its being used not only in callback URL but also in /{db}/_oidc request, called by clients to initiate the OIDC Authorization Code flow.
Added changes to check for escaped version of provider appended to the callback URL.
Added changes to append ?name=value instead of &name=value to the redirect url when the redirect_uri itself has no query parameters set. It would be best to parse redirect_uri using url.Parse and set a query parameter using the strongly typed Query/Value methods.

rest/oidc_api.go

rest/oidc_api_test.go

rest/oidc_api.go

…n non-default - oidcAuthRedirectURI refactoring

…n non-default - Reuse oidcAuthRedirectURI variables in TestAddCallbackURLQueryParam tests

bbrks

A few comments, but 2 of those are minor.

rest/oidc_api_test.go

rest/oidc_api.go

sarathkumarsivan

Removed oidc_api_test.go since it became obsolete after the fix is implemented.
Modified getOIDCCallbackURL method signature to receive provider name and a flag to determine if the current provider is default provider, when building the callbackURL check for default provider. If the current provider is default, add provider to the callback URL and skip adding provider otherwise.
Added changes in database.go to use AddURLQueryParam directly when adding providers during initial setup. This would handle the situation better when the user explicitly specify the callbackURL with provider against a non default provider during setup.
Added unit tests for AddURLQueryParam in oidc_test.go
The constant OIDCAuthProvider has been exported to reuse during initial provider setup.
Modified method signature at declaration.

rest/oidc_api.go

rest/oidc_api_test.go

This speeds up full integration test runs significantly, by moving the bucket setup/teardown into an async worker which gets buckets ready in the background while tests are running using a different bucket. ## Usage: ``` export SG_TEST_BACKING_STORE=Couchbase export SG_TEST_COUCHBASE_SERVER_URL=couchbase://localhost go test -v -p=1 -count=1 ./... ``` ### Configuration #### Existing - `SG_TEST_BACKING_STORE` (default `Walrus`) - Can be set to `Couchbase` to enable integration tests - `SG_TEST_COUCHBASE_SERVER_URL` (default `http://localhost:8091`) - The connection string to connect with Couchbase Server #### New - `SG_TEST_USERNAME` (default `"Administrator"`) - The username to use when connecting to the test server - `SG_TEST_PASSWORD` (default `"password"`) - The password to use when connecting to the test server - `SG_TEST_BUCKET_POOL_SIZE` (default `3`) - Specify how many buckets to create and use. - `SG_TEST_BUCKET_QUOTA_MB` (default `150`) - Specify how many MB each bucket's RAM quota should be - `SG_TEST_BACKING_STORE_RECREATE` (default `false`) - Drops any existing test buckets, and creates new ones before continuing to add them to the pool. If this is not set, previous buckets are just readied as normal. - `SG_TEST_BUCKET_POOL_PRESERVE` (default `false`) - In the event of a failing test, the bucket used is not emptied and put back into the pool. This may result in exhaustion of buckets if more tests fail than the pool size. - `SG_TEST_BUCKET_POOL_DEBUG` (default `false`) - If "true", verbose bucket pool logging is turned on, to see what is happening to each bucket whilst running. - `SG_TEST_DISABLE_GSI` - Always set to true until CBG-818 --- * CBG-625 - Add bucket pooling test framework. * Simplify LoggingBucket log code, and include bucket name in log context * Prevent test failures when using walrus buckets * Check for TestBucket pointer in AsGoCBBucket * Fix WaitGroup data race by incrementing counter before sending bucket over bucketReadierQueue * Prevent data race with multiple concurrent cluster.Authenticate() calls by using a pre-authed cluster reference * Fix data race around initialisation of bucketReadierWaitGroup * Add getTestKeyNamespace to force unique doc IDs for xattr tests (avoids KV tombstone issues) * Increase timeout on retry loop for WaitForNumDocsViaChanges * Bump gocb/gocbcore for cluster ops pre-bucket open * Add SG_TEST_USE_VIEWS flag, but force it to be true all the time until 6.5.1 * Fix TestQuerySequencesStatsN1ql bucket type/view check * Use TestsUseViews in RestTester setup * Avoid vet catching unreachable code * Use TestsUseViews for ServerContext test * rename TestsUseViews to TestsDisableGSI and use in PostUpgrade * Handle View and Query stats in TestTombstoneCompaction * Bump bucket pool size from 100MB to 150MB * Parameterise bucket quota * Improve comments, rename things to be TBP scoped, rearrange to tidy * More cleanup * Tweak values in TestAddRawTimeoutRetry to avoid exceeding 150MB bucket quota, whilst still reproducing issue * Add retry loop around UpsertDesignDocument to handle sporadic Erlang 500 errors * Bump sg-bucket/walrus revisions * Handle non-nil empty error from gocb's GetBuckets for 401 status code * Add SG_TEST_USERNAME/SG_TEST_PASSWORD env vars * Add benchmark for GetCallersName * Skip printStats if no buckets opened (e.g. benchmarks) * Remove unused GetTestBucketSpec * Remove alignment chars from test logging * Add comments for unused functions * Drop default test pool size to 3 * Use t.Name() from getTestKeyNamespace * Simplify document keys in single-doc tests * Remove unnesesary query result Close * Clean up TestBucketPool naming/API * Move primary index creation out into test-only code * Make mockClaims expire far into the future * Fix missed InitializeIndex paramter removal * Refer to global TestBucketPool from db TestMain * Unskip TestPostUpgradeIndexesSimple

Switch to compareAndSwap when toggling compactRunning on channelCacheImpl

…n non-default Removed oidc_api_test.go since it became obsolete after the fix is implemented. Modified getOIDCCallbackURL method signature to receive provider name and a flag to determine if the current provider is default provider, when building the callbackURL check for default provider. If the current provider is default, add provider to the callback URL and skip adding provider otherwise. Added changes in database.go to use AddURLQueryParam directly when adding providers during initial setup. This would handle the situation better when the user explicitly specify the callbackURL with provider against a non default provider during setup. Added unit tests for AddURLQueryParam in oidc_test.go The constant OIDCAuthProvider has been exported to reuse during initial provider setup. Modified method signature at declaration.

sarathkumarsivan · 2020-04-10T08:17:31Z

Closing this PR due to other commits merged to this while rebasing. Will open a clean PR.

CBG-801: Auto-generated OIDC callback URL should include provider whe…

9b40efb

…n non-default

sarathkumarsivan assigned bbrks Apr 8, 2020

sarathkumarsivan requested a review from bbrks April 8, 2020 07:41

bbrks requested changes Apr 8, 2020

View reviewed changes

sarathkumarsivan commented Apr 8, 2020

View reviewed changes

sarathkumarsivan requested a review from bbrks April 8, 2020 22:34

sarathkumarsivan added 2 commits April 8, 2020 15:37

CBG-801: Auto-generated OIDC callback URL should include provider whe…

64ed761

…n non-default - oidcAuthRedirectURI refactoring

CBG-801: Auto-generated OIDC callback URL should include provider whe…

6cca62a

…n non-default - Reuse oidcAuthRedirectURI variables in TestAddCallbackURLQueryParam tests

bbrks requested changes Apr 9, 2020

View reviewed changes

rest/oidc_api_test.go Show resolved Hide resolved

rest/oidc_api.go Show resolved Hide resolved

rest/oidc_api.go Show resolved Hide resolved

sarathkumarsivan commented Apr 10, 2020

View reviewed changes

rest/oidc_api.go Show resolved Hide resolved

rest/oidc_api.go Show resolved Hide resolved

rest/oidc_api_test.go Show resolved Hide resolved

sarathkumarsivan force-pushed the CBG-801 branch from 1345814 to 6cca62a Compare April 10, 2020 07:30

bbrks and others added 4 commits April 10, 2020 01:07

CBG-763 Avoid race in cache compaction start (#4548)

7fd5f07

Switch to compareAndSwap when toggling compactRunning on channelCacheImpl

Bump go-fleecedelta revision for null delta fix (#4550)

262f69f

sarathkumarsivan closed this Apr 10, 2020

sarathkumarsivan deleted the CBG-801 branch April 10, 2020 08:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CBG-801: Auto-generated OIDC callback URL should include provider when non-default #4549

CBG-801: Auto-generated OIDC callback URL should include provider when non-default #4549

sarathkumarsivan commented Apr 8, 2020

coveralls commented Apr 8, 2020 •

edited

Loading

sarathkumarsivan left a comment

bbrks left a comment •

edited

Loading

sarathkumarsivan left a comment

sarathkumarsivan commented Apr 10, 2020

CBG-801: Auto-generated OIDC callback URL should include provider when non-default #4549

CBG-801: Auto-generated OIDC callback URL should include provider when non-default #4549

Conversation

sarathkumarsivan commented Apr 8, 2020

coveralls commented Apr 8, 2020 • edited Loading

sarathkumarsivan left a comment

Choose a reason for hiding this comment

bbrks left a comment • edited Loading

Choose a reason for hiding this comment

sarathkumarsivan left a comment

Choose a reason for hiding this comment

sarathkumarsivan commented Apr 10, 2020

coveralls commented Apr 8, 2020 •

edited

Loading

bbrks left a comment •

edited

Loading