Implement org membership based auth #147

MobiusCraftFlip · 2023-06-12T17:08:50Z

No description provided.

magnalite

I have had a quick glance and most of these changes look good! There is however a fundamental concern with this approach so I haven't done a full review as it may require some substantial changes.

wally-registry-backend/src/auth.rs

magnalite

This is a great start! Thanks for getting the ball rolling on this it has been a great help. While going through this PR I noticed some other changes I'd like to make with our existing auth approach so I'll be adding those changes and taking this PR to the finish line. Please don't push any changes!

I've kept the comments I was going to leave to give you some context into the changes I'm making. I recommend also looking at the other changes I make in my commits to see further ways some of the logic here can be improved.

magnalite · 2023-06-14T01:05:28Z

src/commands/login.rs

+        .send()?;
+
+    let device_code_response = device_code_response.json::<DeviceCodeResponse>()?;


Suggested change

.send()?;

let device_code_response = device_code_response.json::<DeviceCodeResponse>()?;

.send()?

.json::<DeviceCodeResponse>()?;

magnalite · 2023-06-14T01:16:15Z

wally-registry-backend/src/auth.rs

@@ -80,11 +96,13 @@ async fn verify_github_token(request: &Request<'_>) -> Outcome<WriteAccess, Erro
    };

    let client = Client::new();
+
+    // The user is in no orgs we can see so we cannot get their userinfo from that.


This comment is confusing and could do with some extra context into why we are doing this.

Suggested change

// The user is in no orgs we can see so we cannot get their userinfo from that.

// Users already logged in may not have given us read:org permission so we

// need to still support a basic read:user check.

// See: https://github.com/UpliftGames/wally/pull/147

// TODO: Eventually we can transition to only using org level oauth

magnalite · 2023-06-14T01:23:51Z

wally-registry-backend/src/auth.rs

+        Ok(github_info) => {
+            return Outcome::Success(WriteAccess::Github(GithubWriteAccessInfo {
+                user: github_info,
+                token: token,
+            }));
+        }


We can use an implicit return here as well field init shorthand for token. Make sure you have rust-analyzer set to give you these hints.

Suggested change

Ok(github_info) => {

return Outcome::Success(WriteAccess::Github(GithubWriteAccessInfo {

user: github_info,

token: token,

}));

}

Ok(github_info) => Outcome::Success(WriteAccess::Github(GithubWriteAccessInfo {

user: github_info,

token,

})),

magnalite · 2023-06-14T01:47:49Z

wally-registry-backend/src/auth.rs

+        .await;
+
+    let github_org_info = match org_response {
+        Err(err) => {
+            return Err(format_err!(err).status(Status::InternalServerError));
+        }
+        Ok(response) => response.json::<Vec<GithubOrgInfo>>().await,
+    };
+
+    match github_org_info {
+        Ok(github_org_info) => match github_org_info.get(0) {
+            Some(_) => Ok(github_org_info
+                .iter()
+                .map(|x| x.organization.login.to_lowercase())
+                .collect::<Vec<_>>()),
+            None => Ok(vec![]),
+        },
+        Err(err) => Err(format_err!("Github auth failed: {}", err).status(Status::Unauthorized)),


As we are returning a result we can use anyhows context trait and ? to simplify this error handling. I'm also not sure exactly what happened at the end here when collecting the user's orgs.

Suggested change

.await;

let github_org_info = match org_response {

Err(err) => {

return Err(format_err!(err).status(Status::InternalServerError));

}

Ok(response) => response.json::<Vec<GithubOrgInfo>>().await,

};

match github_org_info {

Ok(github_org_info) => match github_org_info.get(0) {

Some(_) => Ok(github_org_info

.iter()

.map(|x| x.organization.login.to_lowercase())

.collect::<Vec<_>>()),

None => Ok(vec![]),

},

Err(err) => Err(format_err!("Github auth failed: {}", err).status(Status::Unauthorized)),

.await

.context("Github org membership request failed")?;

let github_org_info = org_response

.json::<Vec<GithubOrgInfo>>()

.await

.context("Failed to parse github org membership")?;

let orgs: Vec<_> = github_org_info

.iter()

.map(|org_info| org_info.organization.login.to_lowercase())

.collect();

Ok(orgs)

magnalite · 2023-06-14T04:31:58Z

Note to self... inform users that they may need to login again to get the new oauth scopes. I accidentally deleted the warning :(

Also we should probably ensure org scopes are claimed by the org user id so in the event an org is renamed they don't lose ownership.

moo1210 · 2023-10-04T00:23:10Z

Just thought I'd check in on this PR's status, as it doesn't appear to have any active reviews, I think it's a great feature to have.

* Nightly workflow

* Refactored TempProject into its own module and started on tests * Created test projects with a diamond-graph This will allow us to test the update tool in all expected situations - One dependency added/removed - One dependency updated - Transitive dependencies changed It's also a nice addition for testing any other commands as well. * Added to primary-registry Metadata and zipped up * Built the update subcommand. * Lockfiles need to be written! * Fixed misspelling of dependency_changes * Reverted test_registry arg back to invisible * Changed target_packages to package_specs Clarity? * Downgrading was added as a concept * Added lockfile method to view packages as packageIds * Installs the new packages now, forgor. * No longer throws if missing lockfile It will generate a new one instead based on the manifest. * Clarity on the filtering process for try_to_use * Added needed imports -- they got stashed by accident * Made the clippy happy and fixed grammar mistakes * Delete root from registry They're not packages to be consumed. * Add new tests, almost done with them all so far. * Added the rest of snapshots missing * Testing now works, yay! * Deleted testing code by mistake and forgot snap They say to run your tests before push.. This is why. * Appleasing the formatter Import in the wrong order... Extra newline at the end... * Made it look... *pretty* and cleaned messes * another blood sarciface for rust fmt * Doing final cleanups per comments upstream * The gods demand it, another formatter sacrifice Co-authored-by: magnalite <jonny@uplift.games> * The coolness must be toned down. * A little silly mistake indeed Seemingly forgot to filter out packages to update for try_to_use. instead it kept them and got rid of everything else. --------- Co-authored-by: magnalite <jonny@uplift.games>

* Initial copy of upload action * Pivot to svenstaro/upload-release-action * Finalise nightly text

MobiusCraftFlip added 2 commits June 12, 2023 17:54

add org-membership publish

ff713a3

Merge branch 'UpliftGames:main' into org-login

04c702b

magnalite requested changes Jun 12, 2023

View reviewed changes

wally-registry-backend/src/auth.rs Show resolved Hide resolved

MobiusCraftFlip added 3 commits June 13, 2023 12:42

handle no user:org permissions

7f631f9

please the fmt god

22db5e3

please the fmt godsv

1c9a104

MobiusCraftFlip requested a review from magnalite June 13, 2023 12:49

magnalite reviewed Jun 14, 2023

View reviewed changes

Clarify logic and misc upkeep changes

49fee58

magnalite and others added 8 commits October 15, 2023 21:52

Nightly build workflow (UpliftGames#148)

3d3d047

* Nightly workflow

Update README.md

6d434c8

Fix nightly builds (UpliftGames#152)

6f89079

* Initial copy of upload action * Pivot to svenstaro/upload-release-action * Finalise nightly text

Fix downloaded file name having no type extension (UpliftGames#155)

0d4a3d3

Update CONTRIBUTING.md

9fb0bd0

Add status badge to README.md

1b9f99f

Update status badge in README.md

4434c1f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement org membership based auth #147

Implement org membership based auth #147

MobiusCraftFlip commented Jun 12, 2023

magnalite left a comment

magnalite left a comment

magnalite Jun 14, 2023

magnalite Jun 14, 2023

magnalite Jun 14, 2023

magnalite Jun 14, 2023 •

edited

Loading

magnalite commented Jun 14, 2023 •

edited

Loading

moo1210 commented Oct 4, 2023

		.send()?;

		let device_code_response = device_code_response.json::<DeviceCodeResponse>()?;

-    // The user is in no orgs we can see so we cannot get their userinfo from that.
+    // Users already logged in may not have given us read:org permission so we
+    // need to still support a basic read:user check.
+    // See: https://github.com/UpliftGames/wally/pull/147
+    // TODO: Eventually we can transition to only using org level oauth

Implement org membership based auth #147

Are you sure you want to change the base?

Implement org membership based auth #147

Conversation

MobiusCraftFlip commented Jun 12, 2023

magnalite left a comment

Choose a reason for hiding this comment

magnalite left a comment

Choose a reason for hiding this comment

magnalite Jun 14, 2023

Choose a reason for hiding this comment

magnalite Jun 14, 2023

Choose a reason for hiding this comment

magnalite Jun 14, 2023

Choose a reason for hiding this comment

magnalite Jun 14, 2023 • edited Loading

Choose a reason for hiding this comment

magnalite commented Jun 14, 2023 • edited Loading

moo1210 commented Oct 4, 2023

magnalite Jun 14, 2023 •

edited

Loading

magnalite commented Jun 14, 2023 •

edited

Loading