Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,6 @@ compile_commands.json
CTestTestfile.cmake
build

src/app_credentials.h
lib/all_servers_dirname.h
src/crypto/secrets.h
.vscode
2 changes: 1 addition & 1 deletion CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ include(${CMAKE_ROOT}/Modules/ExternalProject.cmake)
ExternalProject_Add(rapidjson
PREFIX thirdparty
GIT_REPOSITORY https://github.com/Tencent/rapidjson.git
GIT_TAG v1.1.0
GIT_TAG master
STEP_TARGETS download
CONFIGURE_COMMAND true
BUILD_COMMAND true
Expand Down
15 changes: 12 additions & 3 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,18 @@ The scopes should be `user_info user_courses user_scores user_submit`. To test b

![Screen Shot 2023-02-12 at 14 55 39](https://user-images.githubusercontent.com/25730111/218333852-f739cc46-bcb7-44d6-9209-6b049bfbb31c.png)

Then, create file `src/app_credentials.h` by making a copy of `src/app_credentials.h.template`, and enter the generated `client_id` and `client_secret` into the predefined fields in the file, as well as the Autolab server domain and `redirect_uri` (same as the one entered into the Autolab Oauth2 manager).

![Screen Shot 2023-02-12 at 14 58 41](https://user-images.githubusercontent.com/25730111/218334013-f4c2efb5-d98e-4595-bc3b-2fc747f8a299.png)
Create file "src/crypto/secrets.h" by making a copy of "src/crypto/secrets.h.template", changing the secrest to strings of your choosing.

Then, create file `lib/all_servers_dirname.h` by making a copy of `lib/all_servers_dirname.h.template`, decide where you want to store info text files
for each Autolab server and enter the absolute path to that directory into the
`all_servers_dirname` field in the file.

Inside of this directory, insert files that follow the format as can be seen in
`lib/server_info_example.txt`. While this can be populated manually, you can
also go back to Manage API Applications and click "Download Config" to download
the file. A daemon can populate the courses field with course names according
to the Autolab server. Make sure that there are no duplicate courses or server
names (the automatic config sets this to be equal to the URL).
Comment on lines +34 to +45
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Fix typos and phrasing in setup instructions.

There are a couple of user-facing text issues: "secrest" should be "secrets", and "Inside of this directory" should be "Inside this directory".

✏️ Suggested doc edits
-Create file "src/crypto/secrets.h" by making a copy of "src/crypto/secrets.h.template", changing the secrest to strings of your choosing.
+Create file "src/crypto/secrets.h" by making a copy of "src/crypto/secrets.h.template", changing the secrets to strings of your choosing.

-Inside of this directory, insert files that follow the format as can be seen in
+Inside this directory, insert files that follow the format as can be seen in
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
Create file "src/crypto/secrets.h" by making a copy of "src/crypto/secrets.h.template", changing the secrest to strings of your choosing.
Then, create file `lib/all_servers_dirname.h` by making a copy of `lib/all_servers_dirname.h.template`, decide where you want to store info text files
for each Autolab server and enter the absolute path to that directory into the
`all_servers_dirname` field in the file.
Inside of this directory, insert files that follow the format as can be seen in
`lib/server_info_example.txt`. While this can be populated manually, you can
also go back to Manage API Applications and click "Download Config" to download
the file. A daemon can populate the courses field with course names according
to the Autolab server. Make sure that there are no duplicate courses or server
names (the automatic config sets this to be equal to the URL).
Create file "src/crypto/secrets.h" by making a copy of "src/crypto/secrets.h.template", changing the secrets to strings of your choosing.
Then, create file `lib/all_servers_dirname.h` by making a copy of `lib/all_servers_dirname.h.template`, decide where you want to store info text files
for each Autolab server and enter the absolute path to that directory into the
`all_servers_dirname` field in the file.
Inside this directory, insert files that follow the format as can be seen in
`lib/server_info_example.txt`. While this can be populated manually, you can
also go back to Manage API Applications and click "Download Config" to download
the file. A daemon can populate the courses field with course names according
to the Autolab server. Make sure that there are no duplicate courses or server
names (the automatic config sets this to be equal to the URL).
🧰 Tools
🪛 LanguageTool

[grammar] ~34-~34: Ensure spelling is correct
Context: ...rypto/secrets.h.template", changing the secrest to strings of your choosing. Then, cre...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)


[style] ~39-~39: This phrase is redundant. Consider using “Inside”.
Context: ...ll_servers_dirname` field in the file. Inside of this directory, insert files that follo...

(OUTSIDE_OF)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@README.md` around lines 34 - 45, The README contains typos and awkward
phrasing: change "secrest" to "secrets" and replace "Inside of this directory"
with "Inside this directory"; update the instructions that reference the
template filenames (src/crypto/secrets.h.template and
lib/all_servers_dirname.h.template), the all_servers_dirname field, and the
example file lib/server_info_example.txt to use the corrected words so the setup
steps read clearly and correctly.


You should then after building autolab-cli, be able to run `autolab setup`, and successfully authorize the CLI with your Autolab deployment.

Expand Down
6 changes: 6 additions & 0 deletions include/autolab/autolab.h
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,12 @@ struct EnrollmentOption {

/* exceptions */

class InvalidInputException: public std::exception {
const char * what() const noexcept override {
return "Invalid input from user.";
}
};

// Indicates an error that occurred in HTTP operations.
class HttpException: public std::exception {
private:
Expand Down
14 changes: 7 additions & 7 deletions include/autolab/client.h
Original file line number Diff line number Diff line change
Expand Up @@ -23,17 +23,17 @@ class Client {

public:
/* setup-related */
Client(std::string domain, std::string client_id, std::string client_secret,
std::string redirect_uri, void (*new_token_callback)(std::string, std::string));
void set_tokens(std::string access_token, std::string refresh_token);
Client();

/* oauth-related */
void device_flow_init(std::string &user_code, std::string &verification_uri);
int device_flow_authorize(size_t timeout);
void set_auth_info_list(std::vector<AuthInfo> auth_info_list);
bool has_auth_for_server(const std::string& server_name);
void device_flow_init(std::string &user_code, std::string &verification_uri, std::string& device_code, const ServerInfo& server_info);
int device_flow_authorize(size_t timeout, const std::string& device_code, const ServerInfo& server_info);

/* resource-related */
void get_user_info(User &user);
void get_courses(std::vector<Course> &courses);
void get_user_info(User &user, const ServerInfo& server_info__);
void get_courses(std::vector<Course> &courses); // should get all courses across all servers
void get_assessments(std::vector<Assessment> &asmts, const std::string &course_name);
void get_assessment_details(DetailedAssessment &dasmt, const std::string &course_name, const std::string &asmt_name);
void get_problems(std::vector<Problem> &probs, const std::string &course_name, const std::string &asmt_name);
Expand Down
56 changes: 56 additions & 0 deletions include/autolab/multi_server.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
#ifndef _MULTI_SERVER_H_
#define _MULTI_SERVER_H_

#include <string>
#include <vector>

namespace Autolab {
struct ServerInfo {
std::string server_name;
std::string base_uri; // domain of the autolab service
std::string client_id;
std::string client_secret;
std::string redirect_uri;
std::vector<std::string> courses;
};


class AllServers {
public:
std::vector<ServerInfo> m_server_list;

AllServers(const std::string& all_servers_filename); // parses the JSON file to populate itself

std::vector<std::string> get_all_courses() const;
std::vector<std::string> get_all_server_names() const;

// Returns "" if there is no such course
ServerInfo get_server_from_course(const std::string& course_name) const;
ServerInfo get_server_from_name(const std::string& server_name) const;
Comment on lines +27 to +29
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Misleading comment: return type is ServerInfo, not "".

The comment says "Returns "" if there is no such course" but the function returns ServerInfo. Looking at the implementation in multi_server.cpp, the function actually throws an exception when the course is not found. Update the comment to reflect the actual behavior.

Suggested fix
-    // Returns "" if there is no such course
-    ServerInfo get_server_from_course(const std::string& course_name) const;
-    ServerInfo get_server_from_name(const std::string& server_name) const;
+    // Throws InvalidInputException if the course is not found
+    ServerInfo get_server_from_course(const std::string& course_name) const;
+    // Throws InvalidInputException if the server name is not found
+    ServerInfo get_server_from_name(const std::string& server_name) const;
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@include/autolab/multi_server.h` around lines 27 - 29, The comment for
get_server_from_course (and similarly get_server_from_name) is incorrect: these
functions return ServerInfo and, per the implementation in multi_server.cpp,
throw an exception when the requested server/course is not found; update the
comment to state that they return a ServerInfo on success and throw an exception
(specify exception type if known) when no matching course/server exists so the
declaration matches the actual behavior.

};

struct AuthInfo {
std::string server_name;
bool exists;
std::string access_token;
std::string refresh_token;
};

class AllAuthInfo {
public:
std::vector<AuthInfo> m_auth_info_list;
bool has_auth_for_server(const std::string& server_name);
bool set_tokens_for_server(const std::string& server_name,
const std::string& access_token, const std::string& refresh_token);

// Returns "" if it does not exist
std::string get_access_token_from_server(const std::string& server_name);
std::string get_refresh_token_from_server(const std::string& server_name);
Comment on lines +46 to +48
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Misleading comment: these functions throw or return empty string.

Similar to get_server_from_course, update comments to reflect that get_access_token_from_server throws on failure (per implementation), while get_refresh_token_from_server returns empty string.

Suggested fix
-    // Returns "" if it does not exist
-    std::string get_access_token_from_server(const std::string& server_name);
+    // Throws std::runtime_error if it does not exist
+    std::string get_access_token_from_server(const std::string& server_name);
+    // Returns "" if it does not exist
     std::string get_refresh_token_from_server(const std::string& server_name);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Returns "" if it does not exist
std::string get_access_token_from_server(const std::string& server_name);
std::string get_refresh_token_from_server(const std::string& server_name);
// Throws std::runtime_error if it does not exist
std::string get_access_token_from_server(const std::string& server_name);
// Returns "" if it does not exist
std::string get_refresh_token_from_server(const std::string& server_name);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@include/autolab/multi_server.h` around lines 46 - 48, Update the misleading
comment above the token accessors: change the comment for
get_access_token_from_server to state it throws on failure (instead of returning
""), and update the comment for get_refresh_token_from_server to explicitly
state it returns an empty string when not found; reference the functions
get_access_token_from_server(const std::string& server_name) and
get_refresh_token_from_server(const std::string& server_name) so callers know
the different error behaviors.

};

}

extern Autolab::AllServers g_all_servers;


#endif /* _MULTI_SERVER_H_ */
52 changes: 19 additions & 33 deletions include/autolab/raw_client.h
Original file line number Diff line number Diff line change
Expand Up @@ -19,26 +19,21 @@
#include <rapidjson/document.h>

#include "autolab/autolab.h"
#include "autolab/multi_server.h"

namespace Autolab {

class RawClient {
public:
RawClient(const std::string &domain, const std::string &id,
const std::string &st, const std::string &ru,
void (*tk_cb)(std::string, std::string));

// setters and getters
void set_tokens(std::string at, std::string rt);
const std::string get_access_token() { return access_token; }
const std::string get_refresh_token() { return refresh_token; }
void set_new_tokens_callback(void (*cb)(std::string, std::string)) {
new_tokens_callback = cb;
}
RawClient();


/* oauth-related */
void device_flow_init(std::string &user_code, std::string &verification_uri);
int device_flow_authorize(size_t timeout);
void set_auth_info_list(std::vector<AuthInfo> auth_info_list);
bool has_auth_for_server(const std::string& server_name);
void device_flow_init(std::string &user_code, std::string &verification_uri, std::string& device_code, const ServerInfo& server_info);
int device_flow_authorize(size_t timeout, const std::string& device_code,
const ServerInfo& server_info);

// keeps track of state and config for the current request.
struct request_state {
Expand Down Expand Up @@ -75,8 +70,8 @@ class RawClient {
typedef std::vector<std::pair<std::string, std::string>> Params;

/* REST interface methods */
void get_user_info(rapidjson::Document &result);
void get_courses(rapidjson::Document &result);
void get_user_info(rapidjson::Document &result, const ServerInfo& server_info__);
void get_courses(rapidjson::Document &result, const ServerInfo& server_info);
void get_assessments(rapidjson::Document &result, const std::string &course_name);
void get_assessment_details(rapidjson::Document &result, const std::string &course_name, const std::string &asmt_name);
void get_problems(rapidjson::Document &result, const std::string &course_name, const std::string &asmt_name);
Expand All @@ -89,15 +84,12 @@ class RawClient {
void crud_enrollment(rapidjson::Document &result, const std::string &course_name, std::string email, Params &in_params, CrudAction action);

private:
// domain of the autolab service
std::string base_uri;

// initializes curl interface. Must be called before anything else.
static int curl_ready;
static int init_curl();

// tokens-related
void (*new_tokens_callback)(std::string, std::string);

enum HttpMethod {GET, POST, PUT, DELETE};
HttpMethod crud_to_http(CrudAction action);
Expand Down Expand Up @@ -129,33 +121,27 @@ class RawClient {
// private instance vars
int api_version;

std::string client_id;
std::string client_secret;
std::string redirect_uri;
std::string access_token;
std::string refresh_token;
std::string device_flow_device_code;
std::string device_flow_user_code;
AllAuthInfo all_auth_info;

// perform HTTP request and return result, default method is GET.
long raw_request(request_state *rstate, path_segments &path, param_list &params, HttpMethod method);
long raw_request_optional_refresh(request_state *rstate, path_segments &path, param_list &params, HttpMethod method, bool refresh);
long make_request(rapidjson::Document &response, path_segments &path, param_list &params, HttpMethod method, bool refresh,
long raw_request(request_state *rstate, path_segments &path, param_list &params, const ServerInfo& server_info__, HttpMethod method);
long raw_request_optional_refresh(request_state *rstate, path_segments &path, param_list &params, const ServerInfo& server_info__, HttpMethod method, bool refresh);
long make_request(rapidjson::Document &response, path_segments &path, param_list &params, const ServerInfo& server_info__, HttpMethod method, bool refresh,
const std::string &download_dir, const std::string &suggested_filename, const std::string &upload_filename);

void clear_device_flow_strings();

bool save_tokens_from_response(rapidjson::Document &response);
bool get_token_from_authorization_code(std::string authorization_code);
bool perform_token_refresh();
bool save_tokens_from_response(rapidjson::Document &response, const std::string& server_name);
bool get_token_from_authorization_code(std::string authorization_code, const ServerInfo& server_name);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Misleading parameter name: server_name has type ServerInfo.

The parameter is named server_name but its type is ServerInfo, which is confusing. Rename to server_info for clarity.

Suggested fix
-  bool get_token_from_authorization_code(std::string authorization_code, const ServerInfo& server_name);
+  bool get_token_from_authorization_code(std::string authorization_code, const ServerInfo& server_info);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
bool get_token_from_authorization_code(std::string authorization_code, const ServerInfo& server_name);
bool get_token_from_authorization_code(std::string authorization_code, const ServerInfo& server_info);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@include/autolab/raw_client.h` at line 135, Rename the misleading parameter
name server_name to server_info in the declaration of
get_token_from_authorization_code(std::string authorization_code, const
ServerInfo& server_info) and update the corresponding definition/implementation
and all call sites to use server_info; ensure any documentation/comments and
variable usages inside the get_token_from_authorization_code function that
referenced server_name are updated to the new identifier to match the ServerInfo
type.

bool perform_token_refresh(const ServerInfo& server_info);

bool document_has_error(request_state *rstate, const std::string &error_msg);
void init_regular_path(path_segments &path);
void init_regular_params(param_list &params);
void init_regular_params(param_list &params, const ServerInfo& server_info__);
void init_oauth_token_path(path_segments &path);
void init_device_flow_init_path(path_segments &path);
void init_device_flow_authorize_path(path_segments &path);
void update_access_token_in_params(param_list &params);
void update_access_token_in_params(param_list &params, const ServerInfo& server_info__);
};

}
Expand Down
10 changes: 10 additions & 0 deletions lib/all_servers_dirname.h.template
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
#ifndef _ALL_SERVERS_DIRNAME_H_
#define _ALL_SERVERS_DIRNAME_H_

#include <string>

/* Fill this in with where all the server text files are stored. Make sure it
is an absolute path. */
std::string all_servers_dirname = "/path/to/all_servers/";

#endif /* _ALL_SERVERS_DIRNAME_H_ */
Comment on lines +1 to +10
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Static initialization order fiasco risk.

Per context snippet at lib/autolab/multi_server.cpp:205, g_all_servers is constructed at global scope using all_servers_dirname. Since both are globals in different translation units, there's no guaranteed initialization order—all_servers_dirname may not be initialized when g_all_servers's constructor runs.

Consider using a function that returns a reference to a local static (lazy initialization) to guarantee order:

inline const std::string& get_all_servers_dirname() {
    static std::string dirname = "/path/to/servers/";
    return dirname;
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@lib/all_servers_dirname.h.template` around lines 1 - 10, all_servers_dirname
is a global std::string that can be used before it's initialized by
g_all_servers in another TU, causing static initialization order problems;
replace the global variable with an accessor function (e.g.,
get_all_servers_dirname()) that returns a const std::string& initialized as a
local static (lazy init) and update all call sites (including the g_all_servers
construction in multi_server.cpp) to use get_all_servers_dirname() instead of
the global variable to ensure safe initialization order.

2 changes: 1 addition & 1 deletion lib/autolab/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
add_library(autolab
json_helpers.cpp utility.cpp client.cpp raw_client.cpp)
json_helpers.cpp utility.cpp client.cpp raw_client.cpp multi_server.cpp)

add_dependencies(autolab rapidjson-download)

Expand Down
70 changes: 41 additions & 29 deletions lib/autolab/client.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -13,22 +13,26 @@

namespace Autolab {

Client::Client(std::string domain, std::string client_id,
std::string client_secret, std::string redirect_uri,
void (*new_token_callback)(std::string, std::string))
: raw_client(domain, client_id, client_secret, redirect_uri, new_token_callback) {}
Client::Client()
: raw_client() {}

void Client::set_tokens(std::string access_token, std::string refresh_token) {
raw_client.set_tokens(access_token, refresh_token);
}

/* oauth-related */
void Client::device_flow_init(std::string &user_code, std::string &verification_uri) {
raw_client.device_flow_init(user_code, verification_uri);
void Client::set_auth_info_list(std::vector<AuthInfo> auth_info_list) {
raw_client.set_auth_info_list(auth_info_list);
}


bool Client::has_auth_for_server(const std::string& server_name) {
return raw_client.has_auth_for_server(server_name);
}

void Client::device_flow_init(std::string &user_code, std::string &verification_uri, std::string& device_code, const ServerInfo& server_info) {
raw_client.device_flow_init(user_code, verification_uri, device_code, server_info);
}

int Client::device_flow_authorize(size_t timeout) {
return raw_client.device_flow_authorize(timeout);
int Client::device_flow_authorize(size_t timeout, const std::string& device_code, const ServerInfo& server_info) {
return raw_client.device_flow_authorize(timeout, device_code, server_info);
}

/* custom utility */
Expand Down Expand Up @@ -89,9 +93,9 @@ void enrollment_from_json(Enrollment &enrollment, rapidjson::Value &enrollment_j
}

/* resource-related */
void Client::get_user_info(User &user) {
void Client::get_user_info(User &user, const ServerInfo& server_info) {
rapidjson::Document user_info_doc;
raw_client.get_user_info(user_info_doc);
raw_client.get_user_info(user_info_doc, server_info);
check_for_error_response(user_info_doc);

require_is_object(user_info_doc);
Expand All @@ -100,22 +104,30 @@ void Client::get_user_info(User &user) {
}

void Client::get_courses(std::vector<Course> &courses) {
rapidjson::Document courses_doc;
raw_client.get_courses(courses_doc);
check_for_error_response(courses_doc);

require_is_array(courses_doc);
for (auto &c_doc : courses_doc.GetArray()) {
Course course;
course.name = get_string_force(c_doc, "name");
course.display_name = get_string(c_doc, "display_name");
course.semester = get_string(c_doc, "semester");
course.late_slack = get_int(c_doc, "late_slack", 0);
course.grace_days = get_int(c_doc, "grace_days", 0);
course.auth_level = Utility::string_to_authorization_level(
get_string_force(c_doc, "auth_level"));

courses.push_back(course);
std::vector<std::string> all_servers = g_all_servers.get_all_server_names();

for (const auto& server_name : all_servers) {
if (!raw_client.has_auth_for_server(server_name)) continue;
ServerInfo server_info = g_all_servers.get_server_from_name(server_name);

rapidjson::Document courses_doc;
raw_client.get_courses(courses_doc, server_info);
check_for_error_response(courses_doc);


require_is_array(courses_doc);
for (auto &c_doc : courses_doc.GetArray()) {
Course course;
course.name = get_string_force(c_doc, "name");
course.display_name = get_string(c_doc, "display_name");
course.semester = get_string(c_doc, "semester");
course.late_slack = get_int(c_doc, "late_slack", 0);
course.grace_days = get_int(c_doc, "grace_days", 0);
course.auth_level = Utility::string_to_authorization_level(
get_string_force(c_doc, "auth_level"));

courses.push_back(course);
}
}
}

Expand Down
Loading
Loading